1502454 - Crash in mozilla::layers::ClipManager::PushOverrideForASR

Status: RESOLVED FIXED

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

Description

[Julian Seward [:jseward]]

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

=============================================================

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

This is a consequence of fixing bug 1471671. We got a little further along, that's all.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Bug 1502454 - Ensure we are consistent in how we define and use scroll IDs with WebRender.

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

[Pulsebot]

Pushed by [e-mail]:
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

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/34035754be84

Comment 8

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 9

[Julien Cristau [:jcristau] (back August 18)]

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

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a7b21829921c