Brackets: Document's refCount is no longer reliably correct

[MarcelGerber]

1. Open SpecRunnerUtils.js
2. In the developer tools console, type DocumentManager = require("document/DocumentManager")and DocumentManager.getAllOpenDocuments() (supposed to only output one document - SpecRunnerUtils; otherwise, restart Brackets)
3. In line 403, put your cursor on _notifyActiveEditorChanged and press Ctrl + E to invoke an inline editor
4. -> DocumentManager.getAllOpenDocuments() now includes a second document - EditorManager - with a _refCount of 3
5. In the upper-left corner of the inline editor, click the file name to transform it into a full editor
6. -> DocumentManager.getAllOpenDocuments()'s entry for EditorManager has a _refCount of 5 now
7. Press Ctrl + W to close the full editor
8. -> DocumentManager.getAllOpenDocuments()'s entry for EditorManager has a _refCount of 4 now
9. Close the inline editor
10. -> DocumentManager.getAllOpenDocuments()'s entry for EditorManager has a _refCount of 1 now
11. Retry these steps with git checked out to ebd613b

Result:
On current master, the _refCount of that document, where notably no editor exists any longer, is still 1, which means this document is not gonna get destroyed

Expected:
As it is on ebd613b, _refCount should go down to 0, which means the document will self-destroy

This is the reason for some of the current test failures. I know it makes the UrlCodeHints tests fail, but I suppose it has broader impact.

This is caused by ffa298c78f95c70f98bd905cbba8e2f1fb4a457f, part of #11820

cc @swmitra @nethip @petetnt

[MarcelGerber]

It all boils down to this check: https://github.com/adobe/brackets/blob/76f3f993512a16a7f28b935f0cc07e6131863d83/src/editor/EditorManager.js#L548
As the pane id is not set, a new full editor with the same document is being created, which increases the refCount; but that refCount is not decreased to 0 afterwards

[petetnt]

Nice job debugging the cause @MarcelGerber :+1:

[swmitra]

Great job @MarcelGerber :+1:
I will try to have a look at it today...

[nethip]

Good job @MarcelGerber. Let us target this for 1.7.

[swmitra]

@MarcelGerber @petetnt This problem should be resolved by https://github.com/adobe/brackets/pull/12405.

[MarcelGerber]

Resolved with #12405. Thanks @swmitra.
Closing.

[MarcelGerber]

I am deeply sorry, but I have to reopen this.
As I just ran the unit tests, I noticed the failure of EditorManager test should use an existing editor for a document when requested on same pane (with the additional message Error: Expected spy addView not to have been called.).
git bisect points to 7990632650cc67bf664800de433ab827f681f2e6 (cc @swmitra), the test passed before this commit.

[swmitra]

@MarcelGerber I just saw the error but really confused about the resolution of the problem. The expectation in our actual code is that when a document is getting opened explicitly in current active pane, we do call addView. I will try to debug our code in the actual scenario and see if the test itself needs to be updated.

Edit Can confirm, this particular test has to be updated as the created editor using _SpecRunnerUtils.createEditorInstance_ is never added to the pane , hence when we try to show it , we are going to call addView. One alternate is that we add the pane.addView call in _SpecRunnerUtils.createEditorInstance_ itself , but then again the test will fail if we don't modify it.

Following points are resolution options...

• Modify _showEditor fn by replacing this check _editor.$el.parent()[0] !== pane.$content[0]_ with _editor._paneId !== pane.id_ . That will ensure the test passes as _SpecRunnerUtils.createEditorInstance_ sets the paneId in it's current form. But then, instead of checking just the id, I would prefer checking the actual DOM object to verify whether this view is already present in the pane.
• Modify the test case as the failure is a false alarm because the test preconditions doesn't match the actual happenings in the code.

@MarcelGerber @petetnt @nethip What do you think guys ?

[nethip]

@swmitra I think if I understand it right, because the semantics have now changed, we need to update the test cases. But do analyze and see if it is going to effect extensions.

[swmitra]

12439 resolves the reported issue as well as 4 more failing tests in MainViewManager.

@MarcelGerber @nethip Can you guys please have a look at the PR.
Thanks in advance :+1: