Leak with <iframe> in <img>

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Core
DOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: mccr8, NeedInfo)

Tracking

(Blocks: 1 bug, {memory-leak, testcase})

Trunk
mozilla45
memory-leak, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44 fixed, firefox45 fixed, firefox-esr38 wontfix, b2g-v2.5 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8695232 [details]
testcase

Leaks nsGlobalWindow and nsDocument.

Updated

2 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 1

2 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

2 years ago
There's a missing reference to an about:blank outer window.
(Assignee)

Comment 3

2 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

2 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

2 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

2 years ago
Yeah, that fixes the leak for me.
Assignee: continuation → nobody
Keywords: regressionwindow-wanted
(Assignee)

Comment 7

2 years ago
It looks like that's a regression from bug 870021, which overrode DestroyContent.
Blocks: 870021
status-firefox42: --- → wontfix
status-firefox43: --- → affected
status-firefox44: --- → affected
status-firefox-esr38: --- → affected

Comment 8

2 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 9

2 years ago
Sure.
Assignee: nobody → continuation
(Assignee)

Comment 10

2 years ago
Created attachment 8695366 [details] [diff] [review]
HTMLImageElement should call its superclass's DestroyContent().

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

2 years ago
status-firefox43: affected → wontfix
status-firefox-esr38: affected → wontfix
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 13

2 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 17

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8bfc05c3e99
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: affected → fixed
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

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2da4256bbe6
status-firefox44: affected → fixed
You need to log in before you can comment on or make changes to this bug.