Closed
Bug 1230110
Opened 9 years ago
Closed 9 years ago
Leak with <iframe> in <img>
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: jruderman, Assigned: mccr8, NeedInfo)
References
Details
(Keywords: memory-leak, testcase, Whiteboard: [MemShrink])
Attachments
(2 files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
smaug
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
Leaks nsGlobalWindow and nsDocument.
Updated•9 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 1•9 years ago
|
||
That looks like a simple test case. It seems like it must be some kind of regression.
Assignee: nobody → continuation
Keywords: regressionwindow-wanted
Assignee | ||
Comment 2•9 years ago
|
||
There's a missing reference to an about:blank outer window.
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
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.
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.
Assignee | ||
Comment 6•9 years ago
|
||
Yeah, that fixes the leak for me.
Assignee: continuation → nobody
Keywords: regressionwindow-wanted
Assignee | ||
Comment 7•9 years ago
|
||
It looks like that's a regression from bug 870021, which overrode DestroyContent.
Blocks: srcset
status-firefox42:
--- → wontfix
status-firefox43:
--- → affected
status-firefox44:
--- → affected
status-firefox-esr38:
--- → affected
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.)
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.
Attachment #8695366 -
Flags: review?(bugs)
Assignee | ||
Updated•9 years ago
|
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
Attachment #8695366 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 12•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5e9f307ea4a
Keywords: checkin-needed
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
Attachment #8695366 -
Flags: approval-mozilla-aurora?
Comment 14•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d8d05aa4ee
Keywords: checkin-needed
Comment 15•9 years ago
|
||
bugherder landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9d8d05aa4ee
Comment 17•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b8bfc05c3e99
Status: NEW → RESOLVED
Closed: 9 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+
Comment 20•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2da4256bbe6
Comment 21•9 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f2da4256bbe6
status-b2g-v2.5:
--- → fixed
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•