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

Status: RESOLVED FIXED

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

Description

[Treeherder Bug Filer]

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

Comment 1

[Intermittent Failures Robot]

1 failures in 735 pushes (0.001 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 1

Platform breakdown:
* linux64-qr: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1434996&startday=2018-01-29&endday=2018-02-04&tree=all

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I can reproduce this locally, investigating.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

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

Comment 5

[Kartikaya Gupta (email:[e-mail])]

There's a debug M-e10s-5 failure in the above try push that is failing the modified assertion. Looking into it.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) 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

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1434996 - Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager.

Changing the transaction id allocator is now handled better in
ClientLayerManager than before in that it resets the transaction id on
the new allocator to match the old allocator and avoid discontinuities
in the transaction ids. We should apply this behaviour to
WebRenderLayerManager as well, because WebRenderLayerManager was
assuming that any time the allocator changed it would automatically
start the transaction id at 1, which is not the case. In particular,
when navigating to something in the bfcache, we can reuse a pre-existing
refresh driver which might have a transaction id already greater than 1.

Review commit: https://reviewboard.mozilla.org/r/219144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/219144/

Comment 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8949833 [details]
Bug 1434996 - Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager.

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

Good catch!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0a2f744ef5c
Update WebRenderLayerManager::SetTransactionIdAllocator to match ClientLayerManager. r=sotaro

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

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