880862 - Leak with canvas fillText and a preserved wrapper on @font-face rule's style

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Leaks nsGlobalWindow and nsDocument.

Comment 1

[Andrew McCreight [:mccr8]]

probably my fault somehow

Comment 2

[Andrew McCreight [:mccr8]]

This is what is holding things alive:

0x12a53f690 [nsCSSFontFaceRule]
    --[Preserved wrapper]-> 0x128806180 [JS Object (Proxy)]
    --[type_proto]-> 0x118bd1af0 [JS Object (CSSStyleDeclarationPrototype)]
    --[getter]-> 0x1288135c0 [JS Object (Function)]
    --[parent]-> 0x118b34240 [JS Object (Window)]
    --[document]-> 0x118b3cf00 [JS Object (Proxy)]
    --[UnwrapDOMObject(obj)]-> 0x126770800 [nsDocument normal (xhtml) file:///Users/amccreight/mz/tests/880862.html]

    Root 0x12a53f690 is a ref counted object with 1 unknown edge(s).
    known edges:
       0x1297a7d70 [nsCSSStyleSheet] --[mOrderedRules[i]]-> 0x12a53f690
       0x128806180 [JS Object (Proxy)] --[UnwrapDOMObject(obj)]-> 0x12a53f690

So, something isn't reporting an edge to a nsCSSFontFaceRule to the CC.  I'll poke around and try to figure out what that might be.

Comment 3

[Andrew McCreight [:mccr8]]

The weak map doesn't seem to show up in the CC log (with additional logging added for weak maps...), so I think what is happening is that the weak map is just triggering wrapper preservation on document.styleSheets[0].cssRules[0].style, and then we don't have any way to trace through the preserved wrapper for whatever reason.

Note that replacing the second line in the test that does the weak map dance with this leaks in a similar looking way:
  document.styleSheets[0].cssRules[0].style.happy = "CLAP YOUR HANDS!";

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: test case without weakmaps

Comment 5

[Andrew McCreight [:mccr8]]

I'm still not really sure what is going on here, but this looks pretty weird to me, so I'm marking it s-s out of caution.

nsCSSFontFaceRule contains, as a field, not a pointer, a nsCSSFontFaceStyleDecl.  nsCSSFontFaceStyleDecl::ContainingRule() implies that the code doesn't want to rely on the fact that the Decl is stored at an offset of 0.  The Decl can be wrapper cached, but not the rule itself (as far as I can tell).

Hmm.  Ok, I see, refcount operations on the Decl are forwarded to the containing rule, and the QI contains similar magicks.  So that's how that works...

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: ref count log

Comment 7

[Andrew McCreight [:mccr8]]

I was worried that we were hitting some weird double wrapper situation, but all of the references from JS seem to be accounted for so this doesn't seem s-s to me after all.

After digging through the ref count logs, I think the extra reference is in nsPresContext::mUserFontSet, which is an array that includes strong references to nsCSSFontFaceRule.  nsPresContext isn't cycle collected, and with new DOM bindings, the nsCSSFontFaceRule now holds the document and window alive, so maybe whatever manages the lifetime of nsPresContext isn't clearing out this array or something?

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: annotated refcount log

This version of the ref count logs includes my analysis, such as it is, as comments.  The stacks involving mUserFontSet are particularly suspicious to me because they are the only ones that involve canvas.  Maybe this all bogus though, I don't know anything about layout.

Comment 9

[Andrew McCreight [:mccr8]]

bz figured out the problem here (lightly edited IRC transcript):

bz: I think that's our cycle
bz: canvas context to font group to font set to style rule to preserved wrapper to window to document to canvas dom node to canvas context
bz: So nsUserFontSet::Destroy
bz: maybe it should drop more refs....
bz: 93  nsTHashtable< nsPtrHashKey<nsFontFaceLoader> > mLoaders;
bz: 95  nsTArray<FontFaceRuleRecord>  mRules;
bz: It calls Cancel() on the mLoaders stuff
bz: but does nothing with mRules
bz: So maybe it should mRules.Clear()

...and that does indeed fix the leak in the test case.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: patch

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 761194 [details] [diff] [review]
patch

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

try run looks okay https://tbpl.mozilla.org/?tree=Try&rev=b3bb655d7107

Comment 12

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/510ecef8cfdd

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/510ecef8cfdd

Comment 14

[Andrew McCreight [:mccr8]]

This should get landed on 23, also.  The ship has sailed on 22, and ESR17 is almost dead so that doesn't matter.  I'll try to figure out if this should go on b2g18 or not.  This should be a very low risk patch, so maybe it wouldn't hurt.

Comment 15

[Jesse Ruderman]

Why is this worth uplifting?  I don't think it's common to manipulate styles through document.styleSheets[0].cssRules[0].  (But when manipulating styles, it's probably common to create expandos.)

Comment 16

[Andrew McCreight [:mccr8]]

I don't know anything about CSS.  If you say it isn't worth uploading, I won't bother.