671101 - docShell in framescripts leaks

Status: RESOLVED WORKSFORME

(Core :: DOM: Navigation, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

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).

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: trigger it

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

Comment 2

[:Felipe Gomes (needinfo for replies!)]

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

[Dietrich Ayala (:dietrich)]

Does this mean all frame scripts leak docshells, or only some?

Comment 4

[Benjamin Smedberg]

I suspect this is because docshells are not cycle-collected. So for the moment, don't do that, just write an accessor?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

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

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 7

[Dão Gottwald [:dao]]

(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

[Olli Pettay [:smaug][[e-mail]]]

(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."

Comment 9

[:Felipe Gomes (needinfo for replies!)]

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.

Comment 10

[:Felipe Gomes (needinfo for replies!)]

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

Comment 11

[:Felipe Gomes (needinfo for replies!)]

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.