Jesse Ruderman Reporter
	Description • 9 years ago

Leaks nsGlobalWindow and nsDocument.

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 9 years ago

That looks like a simple test case. It seems like it must be some kind of regression.

	Andrew McCreight [:mccr8] Assignee
	Comment 2 • 9 years ago

There's a missing reference to an about:blank outer window.

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 9 years ago

0x11f85a400 [nsGlobalWindow #2147483652 outer about:blank]

    Root 0x11f85a400 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x11f85ac00 [nsGlobalWindow #2147483653 inner about:blank] --[mOuterWindow]--> 0x11f85a400
       0x11f84d880 [nsJSContext] --[mGlobalObjectRef]--> 0x11f85a400

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 9 years ago

I think I know what this is.
HTMLImageElement::DestroyContent() is missing to call its parent class' DestroyContent.
The missing edge is probably iframe->frameloader->docshell->outerwindow.

	Andrew McCreight [:mccr8] Assignee
	Comment 5 • 9 years ago

Alright. It seems like it isn't a recent regression, as I seem to get the issue in the October 6th Nightly.

	Andrew McCreight [:mccr8] Assignee
	Comment 6 • 9 years ago

Yeah, that fixes the leak for me.

	Andrew McCreight [:mccr8] Assignee
	Comment 7 • 9 years ago

It looks like that's a regression from bug 870021, which overrode DestroyContent.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 8 • 9 years ago

hey, you have the patch ready there. want to upload it? I didn't test it, just had the idea that it probably should fix this :)

(Other DestroyContent implementations look ok.)

	Andrew McCreight [:mccr8] Assignee
	Comment 9 • 9 years ago

Sure.

	Andrew McCreight [:mccr8] Assignee
	Comment 10 • 9 years ago

I verified that the test leaks without the change and does not leak with it.

I'll probably nominate this for Aurora but not beta, as we're kind of late in beta and we've gone so long with this leak it hopefully doesn't happen much in the wild.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 9 years ago

Comment on attachment 8695366 [details] [diff] [review]
HTMLImageElement should call its superclass's DestroyContent().

...and this is indeed rather odd use of HTML

	Andrew McCreight [:mccr8] Assignee
	Comment 12 • 9 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5e9f307ea4a

	Andrew McCreight [:mccr8] Assignee
	Comment 13 • 9 years ago

Comment on attachment 8695366 [details] [diff] [review]
HTMLImageElement should call its superclass's DestroyContent().

Approval Request Comment
[Feature/regressing bug #]: bug 870021, which landed in Fx 32
[User impact if declined]: memory leaks, though it isn't clear how much this happens on actual web pages
[Describe test coverage new/current, TreeHerder]: There's a test for the leak, plus this code is used a lot in normal testing.
[Risks and why]: low. This is very similar to the implementation of other HTML elements.
[String/UUID change made/needed]: none

	Pulsebot
	Comment 14 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d8d05aa4ee

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 9 years ago
bugherder landing

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d8d05aa4ee

	Pulsebot
	Comment 16 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8bfc05c3e99

	Carsten Book [:Tomcat]
	Comment 17 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b8bfc05c3e99

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 18 • 9 years ago

Jesse, could you please confirm this leak is fixed? Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 19 • 9 years ago

Comment on attachment 8695366 [details] [diff] [review]
HTMLImageElement should call its superclass's DestroyContent().

Memleak fixes are good (however small it might be). This has been on Nightly for a few days now, let's uplift to Aurora44.

	Carsten Book [:Tomcat]
	Comment 20 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2da4256bbe6

	Carsten Book [:Tomcat]
	Comment 21 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f2da4256bbe6