Closed Bug 1431211 Opened 2 years ago Closed 2 years ago

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


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




Tracking Status
firefox59 --- fixed


(Reporter: Gankra, Assigned: lsalzman)




(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
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 to check that this solves our problem?
Pushed by
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
> to check that this
> solves our problem?

The leak is gone now, yes:
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.