Crash in mozilla::layers::ClipManager::PushOverrideForASR

RESOLVED FIXED in Firefox 64

Status

()

defect
P2
critical
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: jseward, Assigned: aosmond)

Tracking

(Blocks 2 bugs, {crash})

unspecified
mozilla65
Unspecified
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64 fixed, firefox65+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

6 months ago
This bug was filed from the Socorro interface and is
report bp-9c63eb52-8c44-40c9-97d6-b64d30181026.
=============================================================

This is topcrash #4 in the Windows nightly 20181025220044, with
6 crashes in 3 different installs.

Top 10 frames of crashing thread:

0 xul.dll void mozilla::layers::ClipManager::PushOverrideForASR gfx/layers/wr/ClipManager.cpp:102
1 xul.dll void mozilla::layers::ClipManager::BeginList gfx/layers/wr/ClipManager.cpp:61
2 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1379
3 xul.dll nsDisplayTransform::CreateWebRenderCommands layout/painting/nsDisplayList.cpp:8865
4 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1456
5 xul.dll nsDisplayOwnLayer::CreateWebRenderCommands layout/painting/nsDisplayList.cpp:7082
6 xul.dll nsDisplayFixedPosition::CreateWebRenderCommands layout/painting/nsDisplayList.cpp:7512
7 xul.dll mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList gfx/layers/wr/WebRenderCommandBuilder.cpp:1456
8 xul.dll mozilla::layers::WebRenderCommandBuilder::BuildWebRenderCommands gfx/layers/wr/WebRenderCommandBuilder.cpp:1320
9 xul.dll mozilla::layers::WebRenderLayerManager::EndTransactionWithoutLayer gfx/layers/wr/WebRenderLayerManager.cpp:300

=============================================================
(Reporter)

Updated

6 months ago
Flags: needinfo?(aosmond)
(Assignee)

Updated

6 months ago
(Assignee)

Updated

6 months ago
Component: Graphics: Layers → Graphics: WebRender
Priority: -- → P2
(Assignee)

Comment 1

6 months ago
ClipManager::PushOverrideForASR assumes we will always get a scroll ID. Presumably this is because we call ClipManager::BeginItem for the display item that triggered the override, which would have run:

https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/gfx/layers/wr/ClipManager.cpp#222

Which would have ensured we have a scroll ID. This suggests that we hit one of these cases:

https://searchfox.org/mozilla-central/rev/d5fc6f3d4746279a7c6fd4f7a98b1bc5d05d6b01/gfx/layers/wr/ClipManager.cpp#282,297,303
(Assignee)

Comment 2

6 months ago
This is a consequence of fixing bug 1471671. We got a little further along, that's all.
(Assignee)

Updated

6 months ago
See Also: → 1471671
(Assignee)

Comment 3

6 months ago
This suggests to me, then, that we should have a method in ClipManager that checks if there is a scroll ID for an ASR's view ID, and if not, repeat with its parent, until we either find a scroll ID, or we have no parent and use the null scroll ID. This would mirror the behaviour in DefineScrollLayers.
(Assignee)

Updated

6 months ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Flags: needinfo?(aosmond)
(Assignee)

Comment 5

6 months ago
In ClipManager::DefineScrollLayers, we don't always create a scroll ID
for each ASR. We may fail to get the scroll metadata, or the it may not
be scrollable, in which case we should use the scroll ID of its ancestor
(or the root scroll ID if there is no ancestor). This should fix a crash
where we simply assumed the leaf of an ASR tree will always have a valid
scroll ID.

Comment 6

6 months ago
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34035754be84
Ensure we are consistent in how we define and use scroll IDs with WebRender. r=kats

Comment 7

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/34035754be84
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(Assignee)

Comment 8

6 months ago
Comment on attachment 9022099 [details]
Bug 1502454 - Ensure we are consistent in how we define and use scroll IDs with WebRender.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1471671

User impact if declined: May occasionally crash the content process if using WebRender.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Bug 1471671

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We were crashing before, now the theoretical worst case would be the scrolling on the page is flaky. Only affects WebRender.

String changes made/needed: N/A
Attachment #9022099 - Flags: approval-mozilla-beta?
Comment on attachment 9022099 [details]
Bug 1502454 - Ensure we are consistent in how we define and use scroll IDs with WebRender.

webrender crash fix, approved for 64.0b8
Attachment #9022099 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.