Crash in mozilla::layers::WebRenderBridgeChild::IPCOpen

RESOLVED FIXED in Firefox 58

Status

()

P1
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: marcia, Assigned: sotaro)

Tracking

(Blocks: 2 bugs, {crash})

Trunk
mozilla58
Unspecified
Windows 10
crash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 unaffected, firefox57 unaffected, firefox58 fixed)

Details

(Whiteboard: [wr-mvp] [gfx-noted], crash signature)

Attachments

(2 attachments, 5 obsolete attachments)

This bug was filed from the Socorro interface and is 
report bp-26d215f5-3039-47f2-abe6-5fb6e0170817.
=============================================================

Seen while looking at nightly data: http://bit.ly/2v4LYAO. 16 crashes/11 installs. Crashes started using 20170816100153
The WrBridge() call is probably returning null. The SetLayerObserverEpoch gets called pretty early sometimes and it has the side-effect of creating a new LayerManager without fully initializing it.
(Assignee)

Updated

a year ago
Blocks: 1357819
(Assignee)

Updated

a year ago
See Also: → bug 1392316
Whiteboard: [gfx-noted]
Priority: P3 → P2
Whiteboard: [gfx-noted] → [wr-mvp] [gfx-noted]
status-firefox56: --- → unaffected
status-firefox57: affected → unaffected
(Assignee)

Updated

a year ago
See Also: → bug 1399850
(Assignee)

Updated

a year ago
Assignee: nobody → sotaro.ikeda.g
Status: NEW → ASSIGNED
Priority: P2 → P1
(Assignee)

Comment 2

a year ago
The crash still happens since Bug 1401849 fix. And all crash reports had "Failed to create WebRenderBridgeChild".
(Assignee)

Updated

a year ago
Depends on: 1404764
(Assignee)

Comment 3

a year ago
Created attachment 8916846 [details] [diff] [review]
patch - Do not use remote LayerManager when its initialization fails
(Assignee)

Comment 4

a year ago
Created attachment 8916847 [details] [diff] [review]
patch - Do not use remote LayerManager when its initialization fails
Attachment #8916846 - Attachment is obsolete: true
(Assignee)

Comment 5

a year ago
Created attachment 8916848 [details] [diff] [review]
patch part 1 - Do not use remote LayerManager when its initialization fails
Attachment #8916847 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
Created attachment 8916850 [details] [diff] [review]
patch part 2 - Create TabChild::CreateRemoteLayerManager()
(Assignee)

Updated

a year ago
Attachment #8916848 - Attachment description: patch - Do not use remote LayerManager when its initialization fails → patch part 1 - Do not use remote LayerManager when its initialization fails
(Assignee)

Updated

a year ago
Attachment #8916850 - Attachment description: patch - Create TabChild::CreateRemoteLayerManager() → patch part 2 - Create TabChild::CreateRemoteLayerManager()
(Assignee)

Updated

a year ago
Attachment #8916848 - Flags: review?(dvander)
(Assignee)

Updated

a year ago
Attachment #8916850 - Flags: review?(dvander)
Comment on attachment 8916848 [details] [diff] [review]
patch part 1 - Do not use remote LayerManager when its initialization fails

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

::: widget/PuppetWidget.h
@@ +181,5 @@
>    GetLayerManager(PLayerTransactionChild* aShadowManager = nullptr,
>                    LayersBackend aBackendHint = mozilla::layers::LayersBackend::LAYERS_NONE,
>                    LayerManagerPersistence aPersistence = LAYER_MANAGER_CURRENT) override;
>  
> +  // This is used for crearing "remote" layer manager and for re-creaging it

// This is used for creating remote layer managers and for re-creating
// them after a compositor reset.  [...]
Attachment #8916848 - Flags: review?(dvander) → review+
(Assignee)

Comment 9

a year ago
Created attachment 8917205 [details] [diff] [review]
patch part 1 - Do not use remote LayerManager when its initialization fails

Apply the comment.
Attachment #8916848 - Attachment is obsolete: true
Attachment #8917205 - Flags: review+

Comment 10

a year ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd22fb8666c
Do not use remote LayerManager when its initialization fails r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbd346f1d966
Create TabChild::CreateRemoteLayerManager() r=dvander

Comment 11

a year ago
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629e5c24737b
Backed out changeset fbd346f1d966 
https://hg.mozilla.org/integration/mozilla-inbound/rev/21156335f590
Backed out changeset 3dd22fb8666c for frequently asserting in mochitests on Windows 10 x64 debug, often in devtools/client/shared/webpack/shims/test/test_clipboard.html. r=backout
(Assignee)

Comment 13

a year ago
With the patches, When TabChild failed to allocate LayerTransactionChild, PuppetWidget::Paint() becomes to call "GetCurrentWidgetListener()->PaintWindow(this, region)". It seems to hit hidden bug.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 14

a year ago
From the investigation, when mAttachedWidgetListener->GetView()->IsPrimaryFramePaintSuppressed() is true, PuppetWidget::mPreviouslyAttachedWidgetListener is used, in this case, assert of nsView::PaintWindow() could fail.
 https://dxr.mozilla.org/mozilla-central/source/view/nsView.cpp#1066
(Assignee)

Comment 15

a year ago
(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> From the investigation, when
> mAttachedWidgetListener->GetView()->IsPrimaryFramePaintSuppressed() is true,
> PuppetWidget::mPreviouslyAttachedWidgetListener is used,

It was added by Bug 1157941.

But we do not need to call PaintWindow() in this case. We fallback to Basic window because of LayerTransactionChild creation failure. But we do not need to do painting here.
(Assignee)

Comment 16

a year ago
Created attachment 8918292 [details] [diff] [review]
patch part 1 - Do not use remote LayerManager when its initialization fails
Attachment #8917205 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8918292 - Flags: review+
(Assignee)

Comment 17

a year ago
Created attachment 8918293 [details] [diff] [review]
patch part 2 - Create TabChild::CreateRemoteLayerManager()
Attachment #8916850 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8918292 - Attachment description: patch part 1 - Do not use remote LayerManager when its initialization failspatch_1391262_5 → patch part 1 - Do not use remote LayerManager when its initialization fails
(Assignee)

Updated

a year ago
Attachment #8918293 - Flags: review+

Comment 19

a year ago
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38330dd0d9e5
Do not use remote LayerManager when its initialization fails r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c53e24ab2a
Create TabChild::CreateRemoteLayerManager() r=dvander
https://hg.mozilla.org/mozilla-central/rev/38330dd0d9e5
https://hg.mozilla.org/mozilla-central/rev/e0c53e24ab2a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
status-firefox-esr52: --- → unaffected
(Assignee)

Updated

a year ago
Duplicate of this bug: 1399850
Duplicate of this bug: 1406729
I think the root cause of this bug is the same as Bug 1406729, InternalSetDocShellIsActive might be called pretty early and CompositorBridge hasn't be fully initialized yet.

Since the crash volume in Bug 1406729 is not high and the patch is not a simple patch so I am not sure if we should uplift it to beta 57.

Sotaro, do you have any thought about this?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 24

a year ago
The fix has a risk of regression like Bug 1408490 and the crash volume of Bug 1406729 is not high, it might better not up lift the fix.
Flags: needinfo?(sotaro.ikeda.g)
You need to log in before you can comment on or make changes to this bug.