Closed Bug 671101 Opened 13 years ago Closed 6 years ago

docShell in framescripts leaks

Categories

(Core :: DOM: Navigation, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: Felipe, Unassigned)

References

(Blocks 1 open bug)

Details

(Whiteboard: [e10s])

Attachments

(1 file)

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).
Attached patch trigger itSplinter Review
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
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.
Does this mean all frame scripts leak docshells, or only some?
I suspect this is because docshells are not cycle-collected. So for the moment, don't do that, just write an accessor?
Blocks: 681392
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
Assignee: nobody → Olli.Pettay
(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.
Assignee: Olli.Pettay → nobody
(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?
(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."
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.
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)
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: 6 years ago
Flags: needinfo?(felipc)
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.