Closed
Bug 997315
Opened 10 years ago
Closed 10 years ago
Since bug 992244 BrowserTabActors and BrowserAddonActors are being disconnected prematurely
Categories
(DevTools :: Debugger, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: mossop, Assigned: mossop)
References
Details
Attachments
(1 file, 1 obsolete file)
3.37 KB,
patch
|
past
:
review+
|
Details | Diff | 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 1•10 years ago
|
||
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+
Updated•10 years ago
|
Assignee: nobody → dtownsend+bugmail
Status: NEW → ASSIGNED
Component: Developer Tools → Developer Tools: Debugger
Priority: -- → P2
Comment 2•10 years ago
|
||
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?
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/db31631ce10c
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/db31631ce10c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•