1691309 - Missing glyph atlas image keys incorrectly used

Status: RESOLVED FIXED

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

Description

[Andrew Osmond [:aosmond] (he/him)]

The missing glyph code stores only the handle of an image key, and not the namespace. Since the namespace can change and invalidate old keys, this means the missing glyph code can end up using invalid handles with the current namespace. This might either refer to invalid images, or a different image entirely. It must keep the entire key in memory.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1691309 - Manage image key lifetimes properly for the missing glyph atlas.

The resource namespace associated with a WebRenderBridgeChild can change
over time, e.g. due to a tab being moved between windows. We need to
recreate any resource keys as a result of this. The missing glyph atlas
code assumed the namespace was constant, and this could cause issues by
referencing invalid image keys, or even another unrelated image. This
patch makes it properly track the entire image key, not just the handle,
so that it can manage namespace changes properly.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

The bug could cause crashes with the given signature, so I'll mark it as such, although I don't know if it is responsible for the outstanding crashes.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e958c1008b99
Manage image key lifetimes properly for the missing glyph atlas. r=jrmuizel

Comment 4

[Sandor Molnar[:smolnar]]

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

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

This could be related to crashes such as bug 1683070 and bug 1521095 where the image key seems to have gone missing.

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 9201686 [details]
Bug 1691309 - Manage image key lifetimes properly for the missing glyph atlas.

Beta/Release Uplift Approval Request

• User impact if declined: May crash if using WebRender
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Without the patch, we could see seemingly unrelated crashes since it could cause unrelated image keys to be removed and/or used in rendering. The change is straightforward such that we just track the whole the image key instead of the unique piece. Verifying in nightly is difficult due to the low crash rate volume.
• String changes made/needed:

Comment 7

[Pascal Chevrel:pascalc]

Comment on attachment 9201686 [details]
Bug 1691309 - Manage image key lifetimes properly for the missing glyph atlas.

We don't crash with this signature on 86 beta and even on release we only have a handful of crashes. The other related crashers (1521095 and 1645841) are also of very low volume. Given were we are in the beta cycle and the fact that the regressor was in Firefox 63, I think we should go on the safer side and let it ride the 87 train. Don't hesitate to contact me if you think that this patch should go into 86 and I missed something, thanks!

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Pascal Chevrel:pascalc from comment #7)

Comment on attachment 9201686 [details]
Bug 1691309 - Manage image key lifetimes properly for the missing glyph atlas.

We don't crash with this signature on 86 beta and even on release we only have a handful of crashes. The other related crashers (1521095 and 1645841) are also of very low volume. Given were we are in the beta cycle and the fact that the regressor was in Firefox 63, I think we should go on the safer side and let it ride the 87 train. Don't hesitate to contact me if you think that this patch should go into 86 and I missed something, thanks!

I believe this could manifest as many different signatures, although I agree it is unlikely it is a high volume issue.