Frequent leaks in /css/css-fonts/font-display/ WPTs

RESOLVED FIXED in Firefox 66

Status

()

defect
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: mccr8, Assigned: emilio)

Tracking

(Blocks 1 bug, {memory-leak})

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(4 attachments)

Reporter

Description

5 months ago

As seen in bug 1480676, there are frequent leaks in the /css/css-fonts/font-display/ web platform tests. We should fix the leak. I'll try to narrow down what the cause is.

Reporter

Comment 1

5 months ago

The leaks look like generic DOM leaks. I'm not sure if this is the right component for the DOM-ish part of CSS.

Reporter

Comment 2

5 months ago

I managed to reproduce in a run with a CC log. It looks like there's a FontFaceSet keeping things alive.

0x7f1474f60da0 [FontFaceSet ]
--[mRuleFaces[i].mFontFace]--> 0x7f1471ddaf20 [FontFace]
--[mParent]--> 0x7f1471aef000 [nsGlobalWindowInner # 6442450953 inner http://web-platform.test:8000/css/css-fonts/font-display/font-display.html]

Root 0x7f1474f60da0 is a ref counted object with 4 unknown edge(s).
known edges:
   0x7f1471dda7a0 [FontFace] --[mFontFaceSet]--> 0x7f1474f60da0
   [ ... lots more like the previous line ... ]
   0x7f1474f60da0 [FontFaceSet ] --[mUserFontSet->mFontFaceSet]--> 0x7f1474f60da0
   0x7f1471bcd9a0 [Loader] --[mozilla::css::Loader.mObservers]--> 0x7f1474f60da0
   0x7f1471dc4000 [Document normal (xhtml) http://web-platform.test:8000/css/css-fonts/font-display/font-display.html] --[mFontFaceSet]--> 0x7f1474f60da0
Reporter

Comment 3

5 months ago

I did some digging around using DMD heap scan mode. In the run I managed to reproduce the leak in, there were 14 unknown references to a single FontFace, that were causing the FontFace to leak. These 14 unknown references look like they are nsFontFaceLoaders.

I picked one of the nsFontFaceLoaders and tried to figure out what was holding it alive. It looks like an nsStreamLoader is holding it alive. That in turn looks like it is being held alive by an nsCORSListenerProxy which is being held alive by an HttpChannelChild. (There are 15 of those leaking in this log.) I didn't look any further to figure out why the channel child was alive, but I could if that would be useful.

Given that all of this is being held alive by an HTTP channel, maybe the issue lies in networking code? I'm not sure what else would be responsible for making these channels go away.

Emilio, any thoughts?

Flags: needinfo?(emilio)
Assignee

Comment 4

5 months ago

The loader also keeps alive the channel:

https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/layout/style/nsFontFaceLoader.h#59

Which is only nulled out on an error path:

https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/layout/style/FontFaceSet.cpp#637

I don't know if Necko accidentally leaves a reference around to a listener in an error path or something, but looks like we should be able to drop the reference at least on Cance() and StopRequest().

Flags: needinfo?(emilio)
Reporter

Comment 5

5 months ago

Hmm, yeah I do see a reference from an nsFontFaceLoader. Good catch.

Reporter

Comment 6

5 months ago

(In reply to Emilio Cobos Álvarez (:emilio) from comment #4)

I don't know if Necko accidentally leaves a reference around to a listener in an error path or something, but looks like we should be able to drop the reference at least on Cance() and StopRequest().

I'll try that and see if it helps, thanks.

Assignee

Comment 7

5 months ago
Doesn't look like the HTTP channel breaks the reference cycle at all, so do it
when we know we're done with it.
Assignee

Comment 8

5 months ago

Comment on attachment 9036459 [details]
Bug 1519918 - Drop nsFontFaceLoader::mChannel more often. r=jfkthame

Does this help?

Attachment #9036459 - Flags: feedback?(continuation)
Assignee

Comment 9

5 months ago

Ah, sorry, was too fast maybe? Happy to remove the patch if you planned on fixing it, I just read comment 6 :)

Reporter

Comment 10

5 months ago

No, it is better that you fix it. I don't know my way around this channel stuff. I'll try your patch.

Assignee: continuation → emilio
Reporter

Comment 11

5 months ago

Comment on attachment 9036459 [details]
Bug 1519918 - Drop nsFontFaceLoader::mChannel more often. r=jfkthame

I still hit the leak, after only 5 tries.

Attachment #9036459 - Flags: feedback?(continuation) → feedback-
Reporter

Comment 12

5 months ago

I don't know if this helps at all, but the leaking CSSRuleList is being created during css/css-fonts/font-display/font-display.html. In fact, that is the only test in this directory that seems to create a CSSRuleList.

Assignee

Comment 13

5 months ago

Buhh, cancellation of the loaders happens on destruction of the FontFaceSet:

https://searchfox.org/mozilla-central/rev/b29663c6c9c61b0bf29e8add490cbd6bad293a67/layout/style/FontFaceSet.cpp#176

Which is kept alive by the loader, via the FontFace. Do you see a live FontFaceSet? That could explain it.

Flags: needinfo?(continuation)
Reporter

Comment 14

5 months ago
Flags: needinfo?(continuation)
Reporter

Comment 15

5 months ago

Here's the full output from the XPCOM leak checking. It does include a FontFaceSet.

Reporter

Comment 16

5 months ago

As seen in comment 2 and comment 3, the leaking object is a FontFaceSet, which is being held alive by nsFontFaceLoaders.

Reporter

Comment 17

5 months ago

(By "leaking", I mean it is the object in the cycle collector graph is that is holding other things alive.)

Assignee

Comment 18

5 months ago
And start tracking the loader ASAP to be on the safe side.

Both sides already take care of cleaning up pointers on destruction.
Reporter

Comment 19

5 months ago

With this patch, and an additional patch that changes the type of nsFontFaceLoader::mFontFaceSet to a raw pointer instead of a RefPtr, I wasn't able to reproduce the leak in 30 tries.

Your patch should also delete

  testing/web-platform/meta/css/css-fonts/font-display/__dir__.ini

which contains the white listing. (I changed this file in bug 1519912 so that might make rebasing weird until that merges around.)

Reporter

Comment 20

5 months ago

I ran this with the weak reference patch, but not the loader mChannel patch. I still get a leak, but it is smaller. Only 22896 bytes. Emilio said we should file a separate bug on Necko.

Reporter

Updated

5 months ago
Attachment #9036478 - Attachment is patch: false
Assignee

Updated

5 months ago
See Also: → 1520062
Reporter

Comment 21

5 months ago

I'll file a bug about updating the whitelisting and assign it to myself. No need to block this on that. If I do figure something out first, it can just be rolled into the patch here.

Whiteboard: [MemShrink]
Reporter

Updated

5 months ago
Blocks: 1520064
Priority: -- → P3

Comment 22

5 months ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c05fb631121
Drop nsFontFaceLoader::mChannel more often. r=jfkthame

Comment 23

5 months ago
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9570ad88e6d
Make nsFontFaceLoader::mFontFaceSet a weak reference. r=heycam

Comment 24

5 months ago
bugherder
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66

Too late for 65 at this point. Emilio, is this severe enough that we should consider it for the ESR 60.6 release?

Flags: needinfo?(emilio)
Assignee

Comment 26

5 months ago

Unsure, 301 Andrew :)

Flags: needinfo?(emilio) → needinfo?(continuation)
Reporter

Comment 27

5 months ago

There's no evidence that it is actually affecting anybody, so I think it is okay to not backport it.

Flags: needinfo?(continuation)
Reporter

Updated

5 months ago
Blocks: 1517316
Depends on: 1524246
Depends on: 1523181
Depends on: 1526216
You need to log in before you can comment on or make changes to this bug.