:Felipe Gomes (needinfo for replies!) Reporter
	Description • 13 years ago

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

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 1 • 13 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

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 2 • 13 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.

	Dietrich Ayala (:dietrich)
	Comment 3 • 13 years ago

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

	Benjamin Smedberg
	Comment 4 • 13 years ago

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

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 5 • 13 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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 6 • 13 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.

	Dão Gottwald [:dao]
	Comment 7 • 13 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?

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 13 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."

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 9 • 13 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.

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 10 • 11 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

	:Felipe Gomes (needinfo for replies!) Reporter
	Comment 11 • 6 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.