Closed Bug 1390731 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Graphics: WebRender, defect)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed

People

(Reporter: jan, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

(Keywords: crash, nightly-community)

Crash Data

Attachments

(1 file, 2 obsolete files)

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.
It seems that when WebRenderBridgeParent::GetRootCompositorBridgeParent() was called, LayerTreeId was already removed at CompositorBridgeParent.
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.
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.
[2] seems simpler to address the problem.
I changed my mind [1] seems more consistent for WebRenderBridgeParent and better.
Assignee: nobody → sotaro.ikeda.g
Attachment #8898182 - Flags: review?(bugmail)
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?
Attachment #8898182 - Flags: review?(bugmail)
Thansk. It is reasonable :)
Attachment #8898182 - Attachment is obsolete: true
Attachment #8899655 - Flags: review?(bugmail)
Comment on attachment 8899655 [details] [diff] [review]
patch - Add pointer check to GetRootCompositorBridgeParent()

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

Great, thanks!
Attachment #8899655 - Flags: review?(bugmail) → review+
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/408eeeae3d6c
Add pointer check to GetRootCompositorBridgeParent() r=kats
https://hg.mozilla.org/mozilla-central/rev/408eeeae3d6c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: