1187068 - Leak with document.fonts, document.open()

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

PCOM_MEM_LEAK_LOG=2 shows this leaking nsGlobalWindow, nsDocument.

I didn't know my fuzzer could even test document.open() this way!

Comment 1

[Andrew McCreight [:mccr8]]

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.

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

I don't know how to write a test for this. A crash test hangs, presumably because the loading never finishes.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Tell the cycle collector about nsContentSink::mCSSLoader.

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

Comment 5

[Nicholas Nethercote [inactive]]

heycam: review ping!  It's a two-line change :)

Comment 6

[Cameron McCormack (:heycam)]

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.

Comment 7

[Jesse Ruderman]

(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)

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 10

[Andrew McCreight [:mccr8]]

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

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Jesse, could you please verify this specific memleak is fixed in the latest Nightly build? Thanks.

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd207343fc32