Closed Bug 1452845 Opened 2 years ago Closed 2 years ago

Ensure all tests pass with async scene building enabled


(Core :: Graphics: WebRender, enhancement, P1)

Other Branch



Tracking Status
firefox62 --- fixed


(Reporter: kats, Assigned: kats)


(Blocks 2 open bugs)


(Whiteboard: [gfx-noted])


(2 files)

At least one that we need to do (as noted by Botond in bug 1449982 comment 75) is to fix the comment/assertion in UpdateZoomConstraints at [1]. With async scene building enabled, the content process can send a UpdateZoomConstraints message over PAPZCTreeManager, which arrives in APZCTreeManagerParent on the compositor thread, which then calls APZCTreeManager::UpdateZoomConstraints on the compositor thread. Since the compositor thread != the updater thread when async scene building is enabled, it means that codepath can be taken in the GPU process as well, not just in the UI process. So the assertion is not valid any more, and the comment needs updating.

A recent try push with it enabled has much orange:

There's a lot of reftest wonkiness, presumably because the async scene build screws with "sync composites". And a bunch of assertion failures from code that I added which I'll need to investigate.
(In reply to Kartikaya Gupta ( from comment #1)
> There's a lot of reftest wonkiness, presumably because the async scene build
> screws with "sync composites".

Most likely we'll need to flush any inflight async scene builds before [1] to fix this.


This should hopefully fix the assertion failures and leaks (I was seeing leaks locally). Reftests are still going to be busted.
That try push didn't actually enable async scene building, this one does:
No longer blocks: 1454430
Depends on: 1454430
Depends on: 1455302
Depends on: 1455387
Depends on: 1455715
Depends on: 1457448
Depends on: 1458259
Attachment #8972619 - Attachment is obsolete: true
The WR update in bug 1459935 fixes a bunch of async-scene-building crashes. The patches from bug 1457466 expose some other issues that I have WIPs for. Try push with everything so far is at
Depends on: 1460988
Depends on: 1417784
Depends on: 1464086
Finally I have something I think is correct, and looks green (so far):

Will clean it up and post patches.
No longer depends on: 1464086
Comment on attachment 8972619 [details]
Bug 1452845 - Fix up snapshotting implementation with async-scene-building enabled.
Attachment #8972619 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8980411 [details]
Bug 1452845 - Turn on async scene building by default.
Attachment #8980411 - Flags: review?(jmuizelaar) → review+
Was your one-week estimate just assuming you would run into a lot more trouble?
Yeah, my one-week estimate was on the assumption that I would actually need to fix bug 1464086. Turns out we don't need to in order to turn this on. That being said it looks like there's still some orange that might be related on the try push. Either I didn't fix all the problems or I screwed up something during patch cleanup.
Here's the most recent try push:

This includes the patches from bug 1417784. There's a few issues turning up in this push that aren't pre-existing intermittents. One is the R3 failures in webgl-color-test, which I suspect will be fixed by bug 1463313. Then there's the M2 test_fullscreen-api-race.html failure which I need to investigate. And there's a solitary M5 test_mousecapture.xhtml failure which might just be random but that I'm not convinced about yet. has the patches from bug 1463313 and it does seem to address the webgl-color-test failures.
Depends on: 1463313
Depends on: 1464908
Depends on: 1465078 has all the inflight stuff. Looking decent so far. I'll look into the tp5o_webext timeout in g5.
After retriggering g5 a bunch it didn't seem to persist. I also did a logging push at and it was all green. So I'll assume it's good for now, and deal with it if it turns up as a high-frequency after landing these patches.
Pushed by
Fix up snapshotting implementation with async-scene-building enabled. r=botond,jrmuizel
Turn on async scene building by default. r=jrmuizel
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Pulsebot from comment #24)
> Pushed by
> Fix up snapshotting implementation with async-scene-building enabled.
> r=botond,jrmuizel

I don't recall reviewing this :)
I never even flagged you for review on this patch! I guess it's a good thing we're decommissioning MozReview...
Oh, I see what happened, it's from comment 6 wherein I accidentally submitted the patch from bug 1458259 to this bug. MozReview decided to carry the r+ to the new and totally different patch.
Depends on: 1465784
Depends on: 1467429
Depends on: 1465658
Depends on: 1471497
You need to log in before you can comment on or make changes to this bug.