Intermittent PID 12769 | Assertion failure: aTransactionId == 1 || aTransactionId > LastPendingTransactionId(), at /builds/worker/workspace/build/src/gfx/layers/wr/WebRenderBridgeParent.cpp:1269

RESOLVED FIXED in Firefox 60

Status

()

defect
P5
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: intermittent-bug-filer, Assigned: kats)

Tracking

(Blocks 1 bug, {intermittent-failure})

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 disabled, firefox59 disabled, firefox60 fixed)

Details

Attachments

(1 attachment)

Filed by: kgupta [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=159861895&repo=try

https://queue.taskcluster.net/v1/task/SydPAQ6LTJmL7NVQ4z7F2w/runs/0/artifacts/public/logs/live_backing.log

This happened on a try push where I tried turning on the web-platform-tests
I can reproduce this locally, investigating.
Assignee: nobody → bugmail
Based on my investigation so far I think the problem is that if we go back/forward in the bfcache, then we end up reusing a prescontext and refreshdriver from the bfcache. This means we can go from a refresh driver with a high transaction id value to a refresh driver with a lower transaction id value (that is still nonzero), and that causes the assertion failure because the next transaction that the compositor sees will be > 1 but also less than the last transaction id it received.

The test that's hitting this crash does back/forward navigation [1] and when that navigation happens I see a refresh driver with a nonzero transaction id getting installed at [2].

I don't know if we can rely on the transaction ids increasing monotonically given this. Or maybe we should reset the transaction id counter when we pull a prescontext out of the bfcache.

[1] https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/testing/web-platform/tests/service-workers/service-worker/navigate-window.https.html#117,119
[2] https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/layout/painting/nsDisplayList.cpp#2485
Or we can try to use the isFirstPaint flag instead of aTransactionId==1 as the check. That seems to fix it locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a3b6e9e54e765f0fc4b66f314f9d55403859df1
There's a debug M-e10s-5 failure in the above try push that is failing the modified assertion. Looking into it.
Ugh, looks like we sometimes do a paint with the paint suppression turned on? We send a paint after going through [1] with mIsFirstPaint=true and mPaintingSuppressed=true, so the isFirstPaint flag doesn't get to the compositor side. This seems like another bug but I don't want to mess with that right now. I have different idea about how to fix this that is probably going to be more robust.

[1] https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/layout/base/PresShell.cpp#6393
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #6)
> I have different idea about how to fix this that is probably
> going to be more robust.

This idea was to make the transaction id a class which stored the presShellId as well as the transaction id. Then a changing presShellId would indicate that the refresh driver changed and the compositor could use that to do it's special magic.

But then I discovered that since we implemented WebRenderLayerManager, the ClientLayerManager::SetTransactionIdAllocator implementation has grown (in bug 1283947) and it looks like it handles exactly this kind of problem. So I'm copying over that code and let's see how it does:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a42562d78cf286e4d9c74a7adeeda6f98220430a
That's looking better. I think it might fix some of the mochitests as well. Kicked off another try push with some of the skipped mochitests re-enabled to see if they are still failing:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a18532c149a0aebfbfbd8cc6932d854d357291e2
Comment on attachment 8949833 [details]
Bug 1434996 - Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager.

https://reviewboard.mozilla.org/r/219144/#review224958

Good catch!
Attachment #8949833 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c0a2f744ef5c
Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager. r=sotaro
https://hg.mozilla.org/mozilla-central/rev/c0a2f744ef5c
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.