Leak with document.fonts, document.open()

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: jruderman, Assigned: mccr8, NeedInfo)

Tracking

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

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed, firefox42 fixed, firefox43 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments)

Posted file testcase
PCOM_MEM_LEAK_LOG=2 shows this leaking nsGlobalWindow, nsDocument.

I didn't know my fuzzer could even test document.open() this way!
Using CC logs, I determined that a css::Loader was keeping alive the window. Then, using DMD heap scanner logs, I determined that an nsContentSink was holding alive the Loader, via the mCSSLoader field, which should be cycle collected. I could have skipped the second step and found it via code inspection, but for some reason I forgot that nsContentSink is cycle collected.
Assignee: nobody → continuation
Whiteboard: [MemShrink]
It looks like this was an incomplete cycle collectification of Loader in bug 1031967, and then the actual leak was probably caused by bug 1028497, which the CCing was needed for it seems. It would be nice if we had some static analysis of CC methods.
Blocks: 1028497
I don't know how to write a test for this. A crash test hangs, presumably because the loading never finishes.
heycam: review ping!  It's a two-line change :)
Whiteboard: [MemShrink] → [MemShrink:P2]
Comment on attachment 8648412 [details] [diff] [review]
Tell the cycle collector about nsContentSink::mCSSLoader.

Review of attachment 8648412 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for overlooking this.
Attachment #8648412 - Flags: review?(cam) → review+
Keywords: checkin-needed
(In reply to Andrew McCreight [:mccr8] from comment #3)
> I don't know how to write a test for this. A crash test hangs, presumably
> because the loading never finishes.

Either:

A) Make a new file that loads the testcase as an iframe. Make the new page navigate away after boom().
B) Follow the document.open with a document.close() at some point.

Hopefully one of those still reproduces the bug (without the patch)
https://hg.mozilla.org/mozilla-central/rev/ca84cf3f4cfe
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment on attachment 8648412 [details] [diff] [review]
Tell the cycle collector about nsContentSink::mCSSLoader.

Approval Request Comment
[Feature/regressing bug #]: bug 1028497
[User impact if declined]: Potential for bad leaks, though I haven't seen any evidence they are happening in the real world.
[Describe test coverage new/current, TreeHerder]: This feature has some existing tests.
[Risks and why]: Very low. It just tells the cycle collector about one more field.
[String/UUID change made/needed]: none
Attachment #8648412 - Flags: approval-mozilla-beta?
Attachment #8648412 - Flags: approval-mozilla-aurora?
Jesse, could you please verify this specific memleak is fixed in the latest Nightly build? Thanks.
Flags: needinfo?(jruderman)
Comment on attachment 8648412 [details] [diff] [review]
Tell the cycle collector about nsContentSink::mCSSLoader.

Approving for uplift to Aurora42 and Beta41 as the fix seems simple and the patch has been in Nightly for a few days already.
Attachment #8648412 - Flags: approval-mozilla-beta?
Attachment #8648412 - Flags: approval-mozilla-beta+
Attachment #8648412 - Flags: approval-mozilla-aurora?
Attachment #8648412 - Flags: approval-mozilla-aurora+
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.