Closed Bug 1431211 Opened 2 years ago Closed 2 years ago

blob images: fonts not removed from cache (leak) on bridge shutdown

Categories

(Core :: Graphics: WebRender, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: Gankra, Assigned: lsalzman)

References

Details

Attachments

(1 file)

This appears to be the only remaining blocker for turning on blob-images on our CI.

This is very easy to reproduce. Just launch a browser session, go to a website, and close the window. At least on linux, UnscaledFontFontconfigs will be leaked.

This appears to be because they're stored in a static variable: the sFontDataTable.

Entries are only removed from this table by calling DeleteFontData, which webrender calls when it recieves the wr_resource_updates_delete_font message. This is normally called by WebRenderBridgeChild::RemoveExpiredFontKeys.

I *believe* the issue is that we're killing the BridgeChild before it has a chance to run RemoveExpiredFontKeys -- or rather the font keys aren't expiring before the last time we check.

I also believe it's not sufficient to just add a static cleanup of this cache, as this seems to be a legitimate leak that could grow in an unbounded manner if WRBridgeChild's are being created and deleted on a regular basis.
This just crawls the sFontDataTable and discards any font keys that are in the WR API namespace that is just about to be destroyed. By this point we know no other things in the WR instance can render the key, so it is safe to just discard the keys before they would otherwise leak.
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8943501 - Flags: review?(a.beingessner)
Comment on attachment 8943501 [details] [diff] [review]
clean up WR blob image renderer resources on API destruction

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

I'm not very familiar with this part of the codebase, but this seems reasonable from what I've read.
Attachment #8943501 - Flags: review?(a.beingessner) → review+
Might want to do a try build like the one in https://bugzilla.mozilla.org/show_bug.cgi?id=1362115#c8 to check that this solves our problem?
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/294d12c7062b
clean up WR blob image renderer resources on API destruction. r=gankro
(In reply to Alexis Beingessner [:Gankro] from comment #3)
> Might want to do a try build like the one in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1362115#c8 to check that this
> solves our problem?

The leak is gone now, yes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0427966c52e94b61ed74e1b4cf899dc7818ead34
https://hg.mozilla.org/mozilla-central/rev/294d12c7062b
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1431787
You need to log in before you can comment on or make changes to this bug.