Closed Bug 1326421 Opened 3 years ago Closed 3 years ago

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

Categories

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

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox53 --- affected

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 2 obsolete files)

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.
Assignee: nobody → bugmail
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
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.
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
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.
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.
Note that these patches are dependent on the patches in bug 1330037, after being rebased onto the graphics branch.
Comment on attachment 8826303 [details]
Bug 1326421 - Add a compositor option for WebRender being enabled or not.

https://reviewboard.mozilla.org/r/104254/#review104988
Attachment #8826303 - Flags: review?(dvander) → review+
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
Attachment #8826304 - Flags: review?(dvander) → review+
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
Attachment #8826305 - Flags: review?(dvander) → review+
Comment on attachment 8826306 [details]
Bug 1326421 - Clean up remaining uses of gfxPrefs::WebRenderEnabled.

https://reviewboard.mozilla.org/r/104260/#review104996
Attachment #8826306 - Flags: review?(dvander) → review+
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=kgupta@mozilla.com&fromchange=f1d2491ac715c7020454407d5dda0d96615516a6&group_state=expanded
Pushed by kgupta@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Backed out in https://hg.mozilla.org/projects/graphics/rev/384dac8cee40d43595b757da776f524ad0241217 for test failures.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Pushed by kgupta@mozilla.com:
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
For clarity I'm going to close this bug and move the remaining patches to a new bug.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Summary: Allow WR widgets to coexist with non-WR widgets → Allow WR widgets to coexist with non-WR widgets (part 1)
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.
Attachment #8826304 - Attachment is obsolete: true
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.
Attachment #8826306 - Attachment is obsolete: true
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.