Closed Bug 1691309 Opened 9 months ago Closed 9 months ago

Missing glyph atlas image keys incorrectly used

Categories

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

defect

Tracking

()

RESOLVED FIXED
87 Branch
Tracking Status
firefox-esr78 --- wontfix
firefox85 --- wontfix
firefox86 --- wontfix
firefox87 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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.

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.

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.

Crash Signature: [@ core::option::expect_failed | webrender::resource_cache::ResourceCache::get_cached_image ]
Keywords: crash, regression
Pushed by aosmond@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e958c1008b99
Manage image key lifetimes properly for the missing glyph atlas. r=jrmuizel
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

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

See Also: → 1683070, 1521095

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:
Attachment #9201686 - Flags: approval-mozilla-beta?
See Also: → 1645841

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!

Attachment #9201686 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

(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.

You need to log in before you can comment on or make changes to this bug.