Closed Bug 1230110 Opened 4 years ago Closed 4 years ago

Leak with <iframe> in <img>

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 --- fixed
firefox45 --- fixed
firefox-esr38 --- wontfix
b2g-v2.5 --- fixed

People

(Reporter: jruderman, Assigned: mccr8, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase, Whiteboard: [MemShrink])

Attachments

(2 files)

Attached file testcase
Leaks nsGlobalWindow and nsDocument.
Whiteboard: [MemShrink]
That looks like a simple test case. It seems like it must be some kind of regression.
Assignee: nobody → continuation
There's a missing reference to an about:blank outer window.
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
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.
Alright. It seems like it isn't a recent regression, as I seem to get the issue in the October 6th Nightly.
Yeah, that fixes the leak for me.
Assignee: continuation → nobody
It looks like that's a regression from bug 870021, which overrode DestroyContent.
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.)
Sure.
Assignee: nobody → continuation
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.
Attachment #8695366 - Flags: review?(bugs)
Comment on attachment 8695366 [details] [diff] [review]
HTMLImageElement should call its superclass's DestroyContent().

...and this is indeed rather odd use of HTML
Attachment #8695366 - Flags: review?(bugs) → review+
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
Attachment #8695366 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/b8bfc05c3e99
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Jesse, could you please confirm this leak is fixed? Thanks!
Flags: needinfo?(jruderman)
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.
Attachment #8695366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.