1326421 - Allow WR widgets to coexist with non-WR widgets (part 1)

Status: RESOLVED FIXED

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

Description

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

Right now we have the compile-time webrender flag and guards (#ifdef MOZ_ENABLE_WEBRENDER) and the runtime pref (gfx.webrender.enabled). However we will probably need this to be even more granular, and allow individual widgets to have webrender while other widgets are non-webrender. This will allow, for example, doing printing and/or tooltips without webrender while main content windows do have webrender.

We did something similar for APZ, where nsIWidget has a AsyncPanZoomEnabled() function that can be used to determine if a particular widget as APZ enabled.

Comment 1

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

I guess the tricky part is propagating the flag to PuppetWidget/TabChild. In the APZ scenario we seem to have a simplifying assumption [1] which probably won't fly for webrender.

[1] http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/dom/ipc/TabChild.cpp#392

Comment 2

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

This includes 4 patches from bug 1330037 plus an extra 5 patches for this bug:

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

Comment 3

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

Although stuff is looking green on try, I can reproduce an error with the following STR in my local linux build:

1. Start the browser (with gfx.webrender.enabled=true).
2. Open a content tab and load some page. This will create a content process and TabChild with webrender enabled
3. Open about:config in a new tab and set gfx.webrender.enabled=false
4. Open a new window and load some page.

At this point TabChild::RecvSetDocShellIsActive is called even though the TabChild has no layer manager yet (nor a layers id). The RecvSetDocShellIsActive function calls GetLayerManager()->SetLayerObserverEpoch, and the call to GetLayerManager() tries to create a layer manager. However at this point CompositorOptions hasn't been set yet, so it asserts (debug builds) or creates a WebRenderLayerManager (opt builds) and immediately crashes because SetLayerObserverEpoch tries to dereference a null WRBridge().

I need to spend a bit more time wrapping my head around the TabChild creation process, because it seems very unintuitive that we can start creating a layer manager in the tabchild without having any idea what it's going to be attached to on the other side.

Comment 4

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

I guess it should be safe to just not call SetLayerObserverEpoch in that scenario, where the graphics stack hasn't been set up yet. That seems to fix the crash for me locally. Try push with that fixed:

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

Comment 5

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

Still one outstanding issue: it looks like on Windows, opening a non-WR window in the GPU process causes WR windows in the UI process to not paint properly. If I disable the GPU process globally it works fine. This isn't a showstopper but I can investigate while I'm waiting for the first set of patches to land.

Comment 6

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

I suspect the GPU process problem is that the ImageBridge from the content process gets created with the parent side in the GPU process, and so when the WebRenderBridgeParent in the UI process tries to get the images it doesn't have them. Since wiring that up seems more complicated than just getting WR to work in the GPU process (there's already a working patch in bug 1323799) I'll do that.

Comment 7

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

Attachment: Bug 1326421 - Add a compositor option for WebRender being enabled or not.

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

Comment 8

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

Attachment: Bug 1326421 - Have content processes create WebRenderLayerManagers if and only if the compositor being used is WebRender.

Also guard against a scenario where RecvDocShellIsActive gets called before
we have the layers id for the tab. Because we need to have the compositor
options (which requires knowing the layers id) before we can create the layer
manager, we need to avoid calling GetLayerManager() before we have the options.

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

Comment 9

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

Attachment: Bug 1326421 - Update GL context creation code to use compositor-specific WebRender flag instead of a global pref.

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

Comment 10

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

Attachment: Bug 1326421 - Clean up remaining uses of gfxPrefs::WebRenderEnabled.

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

Comment 11

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

Note that these patches are dependent on the patches in bug 1330037, after being rebased onto the graphics branch.

Comment 12

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

Comment on attachment 8826303 [details]
Bug 1326421 - Add a compositor option for WebRender being enabled or not.

https://reviewboard.mozilla.org/r/104254/#review104988

Comment 13

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

Comment on attachment 8826304 [details]
Bug 1326421 - Have content processes create WebRenderLayerManagers if and only if the compositor being used is WebRender.

https://reviewboard.mozilla.org/r/104256/#review104990

Comment 14

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

Comment on attachment 8826305 [details]
Bug 1326421 - Update GL context creation code to use compositor-specific WebRender flag instead of a global pref.

https://reviewboard.mozilla.org/r/104258/#review104992

Comment 15

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

Comment on attachment 8826306 [details]
Bug 1326421 - Clean up remaining uses of gfxPrefs::WebRenderEnabled.

https://reviewboard.mozilla.org/r/104260/#review104996

Comment 16

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

I had to rebase this on top of Jerry's patches in bug 1329574. It was mostly straightforward, except in GLContextProviderWGL.cpp, where he added a use of gfxPrefs::WebRenderEnabled in CreateDummyWindow and I'm not familiar enough with that yet to figure out how to wire it up with a CompositorOptions object. I'll tackle that next but for now I just turned it into an #ifdef.

Try push with the rebased patches:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=f1d2491ac715c7020454407d5dda0d96615516a6&group_state=expanded

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/c3a651f4965a
Add a compositor option for WebRender being enabled or not. r=dvander
https://hg.mozilla.org/projects/graphics/rev/ea5671926fa7
Have content processes create WebRenderLayerManagers if and only if the compositor being used is WebRender. r=dvander
https://hg.mozilla.org/projects/graphics/rev/c647de564cb7
Update GL context creation code to use compositor-specific WebRender flag instead of a global pref. r=dvander
https://hg.mozilla.org/projects/graphics/rev/c5944d059846
Clean up remaining uses of gfxPrefs::WebRenderEnabled. r=dvander

Comment 18

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

Backed out in https://hg.mozilla.org/projects/graphics/rev/384dac8cee40d43595b757da776f524ad0241217 for test failures.

Comment 19

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

I can reland parts 1 and 3 pretty safely, I think. Part 2 is the one that was causing the crashes and I'm going to need to rejigger some of the TabParent code (on m-c) to make it work, I think. Part 4 will depend on part 2 so that will have to wait as well.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/ea5c2bcbb0a3
Add a compositor option for WebRender being enabled or not. r=dvander
https://hg.mozilla.org/projects/graphics/rev/cf63a84c0c18
Update GL context creation code to use compositor-specific WebRender flag instead of a global pref. r=dvander

Comment 22

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

For clarity I'm going to close this bug and move the remaining patches to a new bug.

Comment 23

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

Comment on attachment 8826304 [details]
Bug 1326421 - Have content processes create WebRenderLayerManagers if and only if the compositor being used is WebRender.

This patch has been replaced by part 1 in bug 1333122.

Comment 24

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

Comment on attachment 8826306 [details]
Bug 1326421 - Clean up remaining uses of gfxPrefs::WebRenderEnabled.

This patch has been replaced by parts 2-5 in bug 1333122.