1691065 - Discard invalid resource update transactions due to namespace changes

Status: RESOLVED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Bug 1691065 - Discard invalid resource update transactions due to namespace changes.

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.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d96ca6e90aa2
Discard invalid resource update transactions due to namespace changes. r=jrmuizel

Comment 3

[Alexandru Michis [:malexandru]]

Backed out changeset d96ca6e90aa2 (Bug 1691065) for causing Bug 1691125.

Backout link: https://hg.mozilla.org/integration/autoland/rev/1e0238293dea9dd83644ff46dca31c9046339dcb

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception%2Csuccess%2Cretry%2Crunning%2Cpending%2Crunnable&fromchange=c648cec12f77877c954d3fa15a6b013d63f2fa2e&searchStr=os%2Cx%2C10.14%2Cwebrender%2Cdebug%2Cmochitests%2Ctest-macosx1014-64-qr%2Fdebug-mochitest-browser-chrome-e10s%2Cbc2&tochange=2bce4860770888cdd16fae24ff0dc1b33299b378&selectedTaskRun=ZQKjEabvR6u3bdrqQxT6bQ.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=329044017&repo=autoland&lineNumber=107173

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f0e9e8ac401c
Discard invalid resource update transactions due to namespace changes. r=jrmuizel

Comment 5

[Sandor Molnar[:smolnar]]

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

Comment 6

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

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:

Comment 7

[Pascal Chevrel:pascalc]

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.

Comment 8

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/06dcac1cf7a4

Comment 9

[Julien Cristau [:jcristau]]

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.

Comment 10

[Robert Longson [:longsonr]]

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.

Comment 11

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

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.