Closed Bug 1434996 Opened 3 years ago Closed 3 years ago

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


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




Tracking Status
firefox-esr52 --- unaffected
firefox58 --- disabled
firefox59 --- disabled
firefox60 --- fixed


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


(Blocks 1 open bug)


(Keywords: intermittent-failure)


(1 file)

Filed by: kgupta [at]

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.

Or we can try to use the isFirstPaint flag instead of aTransactionId==1 as the check. That seems to fix it locally.
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.

(In reply to Kartikaya Gupta ( 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:
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:
Comment on attachment 8949833 [details]
Bug 1434996 - Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager.

Good catch!
Attachment #8949833 - Flags: review?(sotaro.ikeda.g) → review+
Pushed by
Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager. r=sotaro
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.