Closed
Bug 671101
Opened 14 years ago
Closed 7 years ago
docShell in framescripts leaks
Categories
(Core :: DOM: Navigation, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: Felipe, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [e10s])
Attachments
(1 file)
675 bytes,
patch
|
Details | Diff | Splinter Review |
I still need to figure out how to narrow it down, but I was using
const webNavigation = docShell.QueryInterface(Ci.nsIWebNavigation)
in a frame script and that causes the current browser-chrome tests to finish with 48 extra non gc'ed docshells (see bug 658738 comment 49).
Reporter | ||
Comment 1•14 years ago
|
||
Found one test that is able to trigger it and leak one extra docshell.. Note that running the full browser-chrome suite adds about 40.
To test: apply this patch, recompile browser/base, and run
TEST_PATH=browser/base/content/test/inspector/browser_inspector_treePanel_output.js make mochitest-browser-chrome
After the test completes, in the output summary, it says:
"
INFO TEST-START | Shutdown
Browser Chrome Test Summary
Passed: 12
Failed: 0
Todo: 0
*** End BrowserChrome Test Results ***
--DOCSHELL 095B99C8 == 7
--DOCSHELL 095B99C8 == 6
--DOCSHELL 095B99C8 == 5
.... (warnings removed)
"
7 docshells with this attached patch, 6 docshells without the patch
Reporter | ||
Comment 2•14 years ago
|
||
This content.js frame script is loaded here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1418
and this is all in-process.
Comment 3•14 years ago
|
||
Does this mean all frame scripts leak docshells, or only some?
Comment 4•14 years ago
|
||
I suspect this is because docshells are not cycle-collected. So for the moment, don't do that, just write an accessor?
Reporter | ||
Comment 5•14 years ago
|
||
reply to Dietrich Ayala (:dietrich) from comment #3)
> Does this mean all frame scripts leak docshells, or only some?
Only some, when they use a reference to a docshell, and it's also triggered by a number of tests (not all of them)
(In reply to Benjamin Smedberg [:bsmedberg] from comment #4)
> So for the moment, don't do that, just write an accessor?
In the bug where this was first found (bug 668307) we've already done that to land the code:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/content.js#43
Updated•14 years ago
|
Assignee: nobody → Olli.Pettay
Comment 6•14 years ago
|
||
(In reply to Felipe Gomes (:felipe) from comment #1)
...
> INFO TEST-START | Shutdown
> Browser Chrome Test Summary
> Passed: 12
> Failed: 0
> Todo: 0
>
> *** End BrowserChrome Test Results ***
> --DOCSHELL 095B99C8 == 7
> --DOCSHELL 095B99C8 == 6
> --DOCSHELL 095B99C8 == 5
> .... (warnings removed)
> "
>
> 7 docshells with this attached patch, 6 docshells without the patch
I can't actually reproduce this.
After *** End BrowserChrome Test Results *** I get 5 docshells, with or without the patch.
Updated•14 years ago
|
Assignee: Olli.Pettay → nobody
Comment 7•14 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #6)
> I can't actually reproduce this.
>
> After *** End BrowserChrome Test Results *** I get 5 docshells, with or
> without the patch.
Did you apply Felipe's patch?
Felipe, are you sure your patch triggers this?
Comment 8•14 years ago
|
||
(In reply to Dão Gottwald [:dao] from comment #7)
> (In reply to Olli Pettay [:smaug] from comment #6)
> > I can't actually reproduce this.
> >
> > After *** End BrowserChrome Test Results *** I get 5 docshells, with or
> > without the patch.
>
> Did you apply Felipe's patch?
I did say "with or without the patch."
Reporter | ||
Comment 9•14 years ago
|
||
This still happens, it just doesn't happen on the inspector tests that I pointed out on comment 0. A full mochitest-browser-chrome run adds 50 docshells with this patch. I'm going to try to find another test that causes the problem by itself.
Reporter | ||
Comment 10•12 years ago
|
||
A reminder comment about this bug is being removed in bug 911496, so I'm needinfo'ing myself to remind me to test this again
Flags: needinfo?(felipc)
Reporter | ||
Comment 11•7 years ago
|
||
docShell is used a lot in frame scripts these days, so I imagine it's fair to say it's not leaking anymore, otherwise we'd have seen it.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(felipc)
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•