1414853 - Crash in nsDisplayImageContainer::ConfigureLayer

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P1, critical)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-32c5947b-f773-4bb3-9d47-c483c0171105.
=============================================================

Seen while looking at crash stats - not exactly sure where to bucket it: http://bit.ly/2ya3K8b

On 58 crashes started using 20171104100412. Install time indicates a few unique users crashed. Crash reason is EXCEPTION_ACCESS_VIOLATION_READ.

Comment 1

[Marcia Knous [:marcia]]

One comment "Tried to change the background colors (new page etc..) to something other than blinding white. I think I broke it. Sorry"

Comment 2

[Jim Mathies [:jimm]]

New crash in 58 that only happens in the extension process. Appears to have something to do with painting.

Comment 3

[Andy McKay]

I wonder what "change the background colors (new page etc...)". Is that someone fiddling with userChrome.css?

Comment 4

[alreiten]

I am getting this crash signature when trying to use the Page Shadow or Update Bookmark webextensions.  When trying to use these extensions, this crash occurs, and I then cannot use any (other) webextension until after I close and restart the browser.

Note: this occurs only with webrender enabled.

https://crash-stats.mozilla.com/report/index/a9c886c2-eb84-4f3e-976a-3b00d0171116#tab-details

Comment 5

[Andy McKay]

Based on comment 4, blocking on webrender.

Comment 6

[Peter Chang[:pchang]]

Webrender is disabled on FF 58.

Comment 7

[Peter Chang[:pchang]]

I think this should be a P2.

Kevin, please help to check this.

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

This problem can be reproduced by installing google translator lite add-on and click the icon.
A blank widget will be shown in Windows Firefox nightly with Webrender enabled and hit the crash (which should be to translator kit in the widget in the normal case).

According to the crash stack, the process is trying to build layers from displaylist. This should not happen since we don't build layers when Webrender is enabled.

The backend type of the layer manager might be wrong here[1].

I will keep investigating.

[1] https://hg.mozilla.org/mozilla-central/annotate/e7fee7042d97/layout/painting/nsDisplayList.cpp#l2353

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

According to Bug 1390741, because of transparent window problem and extensions.webextensions.remote pref enabled, we use BasicCompositor to render this kind of widget, that's why we use ClientLayerManager instead of WebrenderLayrManager.

Now the problem is that we are try to build layer from a nsDisplayBackgroundImage without image data.
Make a early return in [1] can skip the crash, but some component won't be shown on the add-on widget.

I will keep investigating why these image data are missing.


[1] https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/layout/painting/nsDisplayList.cpp#4408

Comment 10

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1414853 - Ensure LayerManager's backend type is LAYERS_WR in CanUseAdvancedLayer since BasicCompositor might be used for remote extension process;

Review commit: https://reviewboard.mozilla.org/r/203824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203824/

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

We added a condition in nsDisplayBackgroundImage::GetLayerState[1] to decide if we should create Webrender commands; however, by using BasicCompositor for remote extension process with Webrender enabled, it causes some unexpected behavior.

This fix decouple the condition from GetLayerState, I will request for review after it passes try tests.

[1] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/layout/painting/nsDisplayList.cpp#3856

Comment 12

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8932771 [details]
Bug 1414853 - Ensure LayerManager's backend type is LAYERS_WR in CanUseAdvancedLayer since BasicCompositor might be used for remote extension process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203824/diff/1-2/

Comment 13

[Sotaro Ikeda [:sotaro]]

:kechen, we use ShouldUseAdvancedLayer() a lot of places and BasicCompositor usage was enabled by Bug 1390741.  ShouldUseAdvancedLayer()s might cause another problems. It might better to change that ShouldUseAdvancedLayer() returns false for non WebRenderLayerManager case. It seems OK since gecko seems to use AdvancedLayer only with WebRender for now. How do you think about it? If it is OK, we could address the problem with the following change.

---------------------------------------

 bool
 nsDisplayItem::CanUseAdvancedLayer(LayerManager* aManager) const
 {
   if (!gfxPrefs::LayersAdvancedBasicLayerEnabled() &&
-      aManager && aManager->GetBackendType() == layers::LayersBackend::LAYERS_BASIC) {
+      aManager && aManager->GetBackendType() != layers::LayersBackend::LAYERS_WR) {
     return false;
   }

Comment 14

[Kevin Chen[:kechen] (UTC + 8)]

Hello Sotaro, thank you for the feedback.
Yes, it seems more reasonable and can solve this kind of problems at one time.
I will do the test and try before requesting the review.

Comment 16

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8932771 [details]
Bug 1414853 - Ensure LayerManager's backend type is LAYERS_WR in CanUseAdvancedLayer since BasicCompositor might be used for remote extension process;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203824/diff/2-3/

Comment 17

[Kevin Chen[:kechen] (UTC + 8)]

The try looks good, the failures are related to the fix.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c77d83449d17debbf8d71b31cd6eb650ab1d9f2f

Comment 18

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8932771 [details]
Bug 1414853 - Ensure LayerManager's backend type is LAYERS_WR in CanUseAdvancedLayer since BasicCompositor might be used for remote extension process;

https://reviewboard.mozilla.org/r/203824/#review211438

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/699d482c86c9
Ensure LayerManager's backend type is LAYERS_WR in CanUseAdvancedLayer since BasicCompositor might be used for remote extension process; r=sotaro

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/699d482c86c9

Comment 21

[Calixte Denizet (:calixte)]

No more crashes in nightly 59 after the patch landed.