1351384 - WebRenderLayerManager getting created in non-WR builds, causes crashes

Status: RESOLVED FIXED

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

Description

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

Poking around crash-stats I see some crash reports in 54 which are in the WebRenderLayerManager code. 54 should not be exercising any WR code, so this is bad. It looks like the PuppetWidget is creating a WebRenderLayerManager for some reason.

Example report: https://crash-stats.mozilla.com/report/index/db225b76-dd3d-4974-862d-a15b02170328

Seems to be happening on all of (Windows, OS X, Linux)

Comment 1

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

I'm contemplating just ripping out all of the CompositorOptions stuff since the reason I added it (allowing non-WR windows to coexist with WR windows) is something we're not doing any more. However it is nice to have in some places - for example the original patch at [1] replaces a "bool aUseApz" in numerous places with the CompositorOptions which feels a little cleaner and more encapsulated.

Also whatever the fix is for this bug will need to be uplifted to Aurora, so I'd rather keep it simple for now rather than ripping out a lot of code.

[1] https://hg.mozilla.org/mozilla-central/rev/3d27c7cbcafa

Comment 2

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

Also, bug 1339266, which I thought was an unexplained regression from the CompositorOptions code, now has a plausible alternative explanation. I was thinking the existence of that bug might also warrant a full backout of the CompositorOptions stuff but I no longer think so.

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fd254c2e0d7122e76fbe501375400b28a420b3d

Comment 4

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

Also https://treeherder.mozilla.org/#/jobs?repo=try&revision=536c4de70b423847a8c6a27d0bbc89cd6f304523 for the linux64-qr jobs

Comment 5

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

Attachment: Bug 1351384 - Stop using the CompositorOptions to check if WebRender is enabled.

I suspect that the PuppetWidget is trying to create the layer manager after
it has been connected to a TabChild but before the TabChild has populated the
CompositorOptions. This results in the PuppetWidget effectively getting an
uninitialized value for the CompositorOptions, and so it sometimes randomly
creates a WebRenderLayerManager, later resulting in a crash.

It seems like exposing the potentially-uninitialized CompositorOptions from
TabChild like this is a bad idea, so I'm removing that API and using the more
reliable gfxVars in PuppetWidget. This is fine for WebRender purposes because
we no longer care to allow having WR compositors co-exist with non-WR
compositors.

We may eventually want to remove the CompositorOptions entirely, but for now
the rest of the usage of it seems fine.

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

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8852186 [details]
Bug 1351384 - Stop using the CompositorOptions to check if WebRender is enabled.

https://reviewboard.mozilla.org/r/124424/#review126982

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/106a5b0e636b
Stop using the CompositorOptions to check if WebRender is enabled. r=dvander

Comment 8

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

I may have landed this a little prematurely, I see a permafail on one of the xpcshell tests in the try push in comment 4. QR-only, so no need to backout, but I'll look into it.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/106a5b0e636b

Comment 10

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

Ah, so what's happening is that during the xpcshell tests, [1] is failing. So the cross-process GetCompositorOptions call returns an empty compositor options. Prior to this patch, that value resulted in the content process creating a non-webrender layer manager, but with this patch, we ignore the compositor options and use gfxVars instead. The gfxVars::UseWebRender is true (because this is a webrender-enabled build) and so we go down a different (technically more correct) codepath and crash. I need to figure out how to make it so we don't crash.

[1] http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/gfx/layers/ipc/CrossProcessCompositorBridgeParent.cpp#128

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 12

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

I guess I might as well. I wanted to fix bug 1351777 first but that shouldn't matter unless webrender is enabled (which can't happen on aurora+).

Comment 13

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

Comment on attachment 8852186 [details]
Bug 1351384 - Stop using the CompositorOptions to check if WebRender is enabled.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1333122
[User impact if declined]: crashes
[Is this code covered by automated tests?]: somewhat
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no, no STR
[List of other uplifts needed for the feature/fix]: none. In particular I realize I made this bug dependent on bug 1351777 but that will not need uplifting as it only manifests with webrender enabled.
[Is the change risky?]: no
[Why is the change risky/not risky?]: it switches to using a gfxVar for deciding the layer manager type, which is safer than checking the CompositorOptions which may not have been initialized. the fix is a speculative one but it's pretty safe
[String changes made/needed]: none

Comment 14

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8852186 [details]
Bug 1351384 - Stop using the CompositorOptions to check if WebRender is enabled.

safer webrender check, aurora54+

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1105ff76f42a