Closed
Bug 1351384
Opened 8 years ago
Closed 8 years ago
WebRenderLayerManager getting created in non-WR builds, causes crashes
Categories
(Core :: Graphics: WebRender, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox-esr52 | --- | unaffected |
firefox53 | --- | unaffected |
firefox54 | --- | fixed |
firefox55 | --- | fixed |
People
(Reporter: kats, Assigned: kats)
References
Details
(Keywords: regression, Whiteboard: [gfx-noted])
Crash Data
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
dvander
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details |
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)
Assignee | ||
Comment 1•8 years ago
|
||
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
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Crash Signature: [@ mozilla::layers::PWebRenderBridgeChild::SendClearCachedResources ]
Assignee | ||
Comment 4•8 years ago
|
||
Also https://treeherder.mozilla.org/#/jobs?repo=try&revision=536c4de70b423847a8c6a27d0bbc89cd6f304523 for the linux64-qr jobs
Comment hidden (mozreview-request) |
Comment 6•8 years ago
|
||
mozreview-review |
Comment on attachment 8852186 [details]
Bug 1351384 - Stop using the CompositorOptions to check if WebRender is enabled.
https://reviewboard.mozilla.org/r/124424/#review126982
Attachment #8852186 -
Flags: review?(dvander) → review+
Assignee | ||
Updated•8 years ago
|
Blocks: 1333122
status-firefox53:
--- → unaffected
status-firefox54:
--- → affected
status-firefox55:
--- → affected
Keywords: regression
Summary: WebRenderLayerManager getting created in non-WR builds → WebRenderLayerManager getting created in non-WR builds, causes crashes
Version: Other Branch → 54 Branch
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/106a5b0e636b
Stop using the CompositorOptions to check if WebRender is enabled. r=dvander
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
Please request Aurora approval on this when you get a chance.
Assignee | ||
Comment 12•8 years ago
|
||
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+).
Flags: needinfo?(bugmail)
Assignee | ||
Comment 13•8 years ago
|
||
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
Attachment #8852186 -
Flags: approval-mozilla-aurora?
Comment 14•8 years ago
|
||
Comment on attachment 8852186 [details]
Bug 1351384 - Stop using the CompositorOptions to check if WebRender is enabled.
safer webrender check, aurora54+
Attachment #8852186 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•8 years ago
|
||
bugherder uplift |
You need to log in
before you can comment on or make changes to this bug.
Description
•