Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Document's refCount is no longer reliably correct #12396

Closed
marcelgerber opened this issue May 2, 2016 · 10 comments
Closed

Document's refCount is no longer reliably correct #12396

marcelgerber opened this issue May 2, 2016 · 10 comments
Milestone

Comments

@marcelgerber
Copy link
Contributor

marcelgerber commented May 2, 2016

  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 ffa298c, part of #11820

cc @swmitra @nethip @petetnt

@marcelgerber
Copy link
Contributor Author

It all boils down to this check:

if (!editor || editor._paneId !== pane.id) {

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
Copy link
Collaborator

petetnt commented May 2, 2016

Nice job debugging the cause @marcelgerber 👍

@swmitra
Copy link
Collaborator

swmitra commented May 3, 2016

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

@nethip
Copy link
Contributor

nethip commented May 3, 2016

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

@swmitra
Copy link
Collaborator

swmitra commented May 5, 2016

@marcelgerber @petetnt This problem should be resolved by #12405.

@marcelgerber
Copy link
Contributor Author

Resolved with #12405. Thanks @swmitra.
Closing.

@marcelgerber marcelgerber added this to the Release 1.7 milestone May 19, 2016
@marcelgerber marcelgerber reopened this May 19, 2016
@marcelgerber
Copy link
Contributor Author

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 7990632 (cc @swmitra), the test passed before this commit.

@swmitra
Copy link
Collaborator

swmitra commented May 20, 2016

@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
Copy link
Contributor

nethip commented May 20, 2016

@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
Copy link
Collaborator

swmitra commented May 20, 2016

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants