Closed Bug 997315 Opened 6 years ago Closed 6 years ago

Since bug 992244 BrowserTabActors and BrowserAddonActors are being disconnected prematurely

Categories

(DevTools :: Debugger, defect, P2)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
I think bug 992244 has introduced a bug with the tab and addon lists. In both cases multiple calls to getList will return the same actor if the tab/add-on hasn't changed. But they create a new pool for each call and destroy the previous one calling disconnect on all the previously returned actors, even though we are sending the same actors this time. I think just but not cleaning up the old actors this is fixed and I can do that here.
Attachment #8407695 - Flags: review?(past)
Comment on attachment 8407695 [details] [diff] [review]
patch

Review of attachment 8407695 [details] [diff] [review]:
-----------------------------------------------------------------

Argh, I thought about that, but then promptly forgot about it when all tests passed. Thanks!
Attachment #8407695 - Flags: review?(past) → review+
Assignee: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Debugger
Priority: -- → P2
That makes me think that we should have an xpcshell test to catch that sort of thing: listTabs and listAddons a couple of times and make sure that everything is still in place. test_add_actors.js is kind of similar. Do you think you could do it in this patch?
(In reply to Panos Astithas [:past] from comment #2)
> That makes me think that we should have an xpcshell test to catch that sort
> of thing: listTabs and listAddons a couple of times and make sure that
> everything is still in place. test_add_actors.js is kind of similar. Do you
> think you could do it in this patch?

The problem is we mock the tab and root actors in xpcshell tests. You will likely have to write a mochitest that acts like an xpcshell test (in that it is more of a unit test than an integration test) to properly test the listTabs and listAddons behavior.
Attached patch patchSplinter Review
I wrote the test and it passed without my patch applied. Digging deeper turns out that this isn't a bug at all since adding an actor to a pool removes it from any pool it's already registered in so destroying the old pool won't affect actors added to the new pool. Yay!

Might as well take the test anyway I guess?

http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/main.js#862
Attachment #8407695 - Attachment is obsolete: true
Attachment #8408388 - Flags: review?(past)
Comment on attachment 8408388 [details] [diff] [review]
patch

Review of attachment 8408388 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, better make sure we never regress this behavior.

::: browser/devtools/debugger/test/browser_dbg_listtabs-03.js
@@ +5,5 @@
> + * Make sure the listTabs request works as specified.
> + */
> +
> +const TAB1_URL = EXAMPLE_URL + "doc_empty-tab-01.html";
> +const TAB2_URL = EXAMPLE_URL + "doc_empty-tab-02.html";

The second tab is not used.
Attachment #8408388 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/db31631ce10c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.