1390731 - Crash in [@ mozilla::layers::WebRenderBridgeParent::GetRootCompositorBridgeParent ]

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Darkspirit]

Nightly 57 x64 20170815100349 @ Windows 10 with webrender+webrendest enabled

bp-16a6a97c-5d92-42a7-a17e-1afbc0170816

I closed some of the many bugzilla tabs and suddently my whole Nightly window became red. I pressed Ctrl+T to open a new tab then which seemed to fix it, but I don't know.

Comment 1

[Sotaro Ikeda [:sotaro]]

It seems that when WebRenderBridgeParent::GetRootCompositorBridgeParent() was called, LayerTreeId was already removed at CompositorBridgeParent.

Comment 2

[Sotaro Ikeda [:sotaro]]

LayerTreeId is removed like the following sequence when GPU process exists. The route is different than a route of calling WebRenderBridgeParent::Destroy().

RenderFrameParent::ActorDestroy(ActorDestroyReason why)
->GPUProcessManager::UnmapLayerTreeId()
->mGPUChild->SendRemoveLayerTreeIdMapping();
->GPUParent::RecvRemoveLayerTreeIdMapping()
->CompositorBridgeParent::DeallocateLayerTreeId();
->CompositorLoop()->PostTask(NewRunnableFunction(&EraseLayerState, aId));
->EraseLayerState(uint64_t aId)

Therefore there could be a case that WebRenderBridgeParent still is not called WebRenderBridgeParent::Destroy() after LayerTreeId removal.

Comment 3

[Sotaro Ikeda [:sotaro]]

There seems two way to address the problem.
[1] call WebRenderBridgeParent::Destroy() in EraseLayerState(uint64_t aId).
[2] Modify WebRenderBridgeParent::GetRootCompositorBridgeParent() as to handle CompositorBridgeParent::GetIndirectShadowTree() == nullptr.

Comment 4

[Sotaro Ikeda [:sotaro]]

[2] seems simpler to address the problem.

Comment 5

[Sotaro Ikeda [:sotaro]]

I changed my mind [1] seems more consistent for WebRenderBridgeParent and better.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Call WebRenderBridgeParent::Destroy() during removing LayerState

Comment 7

[Sotaro Ikeda [:sotaro]]

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

Comment 8

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

Hmm I kind of prefer solution [2]. WebRenderBridgeParent::GetRootCompositorBridgeParent already returns nullptr in some cases and all the call sites of that function gracefully handle the null return. So I think in the case that EraseLayerState is called early, GetRootCompositorBridgeParent should detect that instead of asserting there is a layer state, and return nullptr.

My reasoning for this is that WebRenderBridgeParent is kind of equivalent to LayerTransactionParent, and LayerTransactionParent doesn't do any special handling inside EraseLayerState so we shouldn't need to do it for WebRenderBridgeParent either. If we introduce code path differences like this then we might introduce new bugs, but if we stick to what we know works with LayerTransactionParent in Gecko then we should end up with similar behaviour.

What do you think?

Comment 9

[Sotaro Ikeda [:sotaro]]

Thansk. It is reasonable :)

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add pointer check to GetRootCompositorBridgeParent()

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add pointer check to GetRootCompositorBridgeParent()

Comment 12

[Sotaro Ikeda [:sotaro]]

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

Comment 13

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

Comment on attachment 8899655 [details] [diff] [review]
patch - Add pointer check to GetRootCompositorBridgeParent()

Review of attachment 8899655 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/408eeeae3d6c
Add pointer check to GetRootCompositorBridgeParent() r=kats

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/408eeeae3d6c