[CLOSED] Document's refCount is no longer reliably correct

[core-ai-bot]

Issue by MarcelGerber
Monday May 02, 2016 at 19:50 GMT
Originally opened as adobe/brackets#12396

---

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

[core-ai-bot]

Comment by MarcelGerber
Monday May 02, 2016 at 19:56 GMT

---

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

[core-ai-bot]

Comment by petetnt
Monday May 02, 2016 at 19:58 GMT

---

Nice job debugging the cause@MarcelGerber 👍

[core-ai-bot]

Comment by swmitra
Tuesday May 03, 2016 at 03:14 GMT

---

Great job@MarcelGerber 👍
I will try to have a look at it today...

[core-ai-bot]

Comment by nethip
Tuesday May 03, 2016 at 05:52 GMT

---

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

[core-ai-bot]

Comment by swmitra
Thursday May 05, 2016 at 07:17 GMT

---

@MarcelGerber@petetnt This problem should be resolved by adobe/brackets#12405.

[core-ai-bot]

Comment by MarcelGerber
Thursday May 19, 2016 at 13:13 GMT

---

Resolved with #12405. Thanks@swmitra.
Closing.

[core-ai-bot]

Comment by MarcelGerber
Thursday May 19, 2016 at 22:19 GMT

---

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.

[core-ai-bot]

Comment by swmitra
Friday May 20, 2016 at 01:53 GMT

---

@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 ?

[core-ai-bot]

Comment by nethip
Friday May 20, 2016 at 05:20 GMT

---

@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.

[core-ai-bot]

Comment by swmitra
Friday May 20, 2016 at 08:12 GMT

---

#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 👍