Closed Bug 1691065 Opened 4 years ago Closed 4 years ago

Discard invalid resource update transactions due to namespace changes

Categories

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

defect

Tracking

()

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

People

(Reporter: aosmond, Assigned: aosmond)

References

(Blocks 1 open bug)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

One of the root causes of bug 1645841, we are processing invalid resource updates with stale keys. This causes crashes, notably in the blob update code but probably others.

When we change the namespace, we discard all cached resources and their
associated keys from the WebRender cache. As such if any transaction
comes in with the old namespace ID, we can safefully discard the entire
update, since we will need to recreate any that we are using anyways.

This patch also adds new asserts to ensure we never get old namespace
IDs for individual keys in a valid resource update. This should never
happen in practice.

Pushed by aosmond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d96ca6e90aa2 Discard invalid resource update transactions due to namespace changes. r=jrmuizel
Pushed by aosmond@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/f0e9e8ac401c Discard invalid resource update transactions due to namespace changes. r=jrmuizel
Flags: needinfo?(aosmond)
Crash Signature: [@ core::option::expect_failed | webrender_api::resources::ApiResources::update_blob_image ] [@ core::option::expect_failed | webrender::api_resources::ApiResources::update_blob_image ]
Keywords: crash
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 87 Branch

Comment on attachment 9201454 [details]
Bug 1691065 - Discard invalid resource update transactions due to namespace changes.

Beta/Release Uplift Approval Request

  • User impact if declined: May crash if they have 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): The crash rate is fairly low in nightly so it is hard to verify there. From my testing in CI, it did seem to fix an intermittent crash. The change itself is very straightforward -- we more consistently apply our checks for stale keys to ensure the system state is sane.
  • String changes made/needed:
Attachment #9201454 - Flags: approval-mozilla-beta?

Comment on attachment 9201454 [details]
Bug 1691065 - Discard invalid resource update transactions due to namespace changes.

Approved for 86 beta 8, the volume is low on desktop beta but noticable on Fenix betas, thanks.

Attachment #9201454 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Regressions: 1692736
Regressions: 1692916

One crash on 86.0b8 with this signature, bp-fb593b64-50d2-45a5-899d-ec4250210212
Rust panic with Attempt to update non-existent blob image

The volume is clearly lower than before the patch though, and no fenix crashes so far.

I'm concerned that without bug 1692736 we've not been executing all the code that we should. Need to keep an eye on things now that's landed.

I'm not too worried. It is only used for animated images and corner cases for fonts. It was executed a lot before my change regressed it.

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

Attachment

General

Created:
Updated:
Size: