1635769 - Crash in [@ mozilla::layers::WebRenderCommandBuilder::Destroy]

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

100% reproducible crash trying to open the extension panel in an empty private browsing session.

This bug is for crash report bp-d6a0b21b-acd8-481c-b336-b375d0200506.

Top 10 frames of crashing thread:

0 libxul.so mozilla::layers::WebRenderCommandBuilder::Destroy gfx/layers/wr/WebRenderCommandBuilder.cpp:1508
1 libxul.so mozilla::layers::WebRenderLayerManager::DoDestroy gfx/layers/wr/WebRenderLayerManager.cpp:115
2 libxul.so nsWindow::Destroy widget/gtk/nsWindow.cpp:674
3 libxul.so DestroyWidgetRunnable::Run view/nsView.cpp:123
4 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1200
5 libxul.so mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
6 libxul.so MessageLoop::Run ipc/chromium/src/base/message_loop.cc:290
7 libxul.so nsBaseAppShell::Run widget/nsBaseAppShell.cpp:137
8 libxul.so nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:271
9 libxul.so XREMain::XRE_mainRun toolkit/xre/nsAppRunner.cpp:4608

Comment 1

[Gian-Carlo Pascutto [:gcp]]

This is hitting the following assertion:

MOZ_RELEASE_ASSERT(mWebRenderUserDatas.Count() == 0)

Comment 2

[Darkspirit]

KDE, X11, Debian Testing
Confirmed with my main profile (regular browsing session) by moving uBlock Origin into the overflow menu and clicking on uBlock Origin.
bp-d15cc04a-c852-4009-a3a1-370830200506

Comment 3

[Darkspirit]

Install uBlock Origin.
Right click on its icon > Pin to overflow menu.
Open overflow menu and click on uBlock Origin.
Main process crash.

mozregression --good 2020-03-01 --bad 20200506093716 --pref gfx.webrender.all:true -a https://addons.mozilla.org/en-US/firefox/addon/ublock-origin/

17:04.20 INFO: Last good revision: 74d50028caec9d5856a173c98a6172700f1ccc29
17:04.20 INFO: First bad revision: 6a43f985307516ec1ae2d413a39cd6d813560b8b
17:04.20 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=74d50028caec9d5856a173c98a6172700f1ccc29&tochange=6a43f985307516ec1ae2d413a39cd6d813560b8b

6a43f985307516ec1ae2d413a39cd6d813560b8b sotaro — Bug 1574746 - Remove AllowWebRenderForThisWindow() r=nical

This crash is not reproducible with OpenGL compositor.
mozregression --launch 20200506093716 --pref gfx.webrender.force-disabled:true layers.acceleration.force-enabled:true -a https://addons.mozilla.org/en-US/firefox/addon/ublock-origin/

Comment 4

[Jessie [:jbonisteel] pls NI]

Hey Sotaro, can you take a look at this? Thanks!

Comment 5

[Darkspirit]

And MacOS had this crash reason long before bug 1574746.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #3)

Install uBlock Origin.
Right click on its icon > Pin to overflow menu.
Open overflow menu and click on uBlock Origin.
Main process crash.

Thank you. I confirmed the crash with the STR.

Comment 7

[Sotaro Ikeda [:sotaro]]

Crash happened with nsImageBoxFrame. It was shared between different WebRenderLayerManager/WebRenderCommandBuilder. When the problem happened, nsDisplayItem::GetPerFrameKey() was same between different nsDisplayItems of same nsImageBoxFrame.

Comment 8

[Sotaro Ikeda [:sotaro]]

Current WebRenderUserDataTable expects one WebRenderUserData for each PerFrameKey + WebRenderUserData Type. But its assumption does not work for this bug.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1635769 - Do not recycle WebRenderUserData when RenderRootStateManager is different

Comment 10

[Sotaro Ikeda [:sotaro]]

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

Comment 11

[Miko Mynttinen]

(In reply to Sotaro Ikeda [:sotaro] from comment #7)

nsDisplayItem::GetPerFrameKey() was same between different nsDisplayItems of same nsImageBoxFrame.

What do you mean with this? Display list building has a specific assertion to prevent this situation1. If the assumption is not holding, we might have multiple bugs here.

Comment 12

[Sotaro Ikeda [:sotaro]]

I tested the STR of Comment 3 with log.

When the crash happened, one nsImageBoxFrame instance and one WebRenderUserData instance were shared between different WebRenderLayerManager/WebRenderCommandBuilder. In this case, each WebRenderLayerManager uses each nsDisplayItem for the nsImageBoxFrame instance.

There were 2 nsDisplayItems for one nsImageBoxFrame instance. But aItem->GetPerFrameKey() of both nsDisplayItems were same. It might happen because they belong to different WebRenderLayerManager.

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: temporal patch - Add some logs

Temporal patch that was used for testing the STR of Comment 3

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #12)

I tested the STR of Comment 3 with log.

When the crash happened, one nsImageBoxFrame instance and one WebRenderUserData instance were shared between different WebRenderLayerManager/WebRenderCommandBuilder. In this case, each WebRenderLayerManager uses each nsDisplayItem for the nsImageBoxFrame instance.

There were 2 nsDisplayItems for one nsImageBoxFrame instance. But aItem->GetPerFrameKey() of both nsDisplayItems were same. It might happen because they belong to different WebRenderLayerManager.

I wonder if it could happen when nsImageBoxFrame was moved from one widget to another widget and old widget was destroyed.

Comment 15

[Sotaro Ikeda [:sotaro]]

:hiro, do you know if it is possible to detect when frame is moved from one widget to another widget in Parent process.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I am not familiar with nsIWidget setup to answer the question. I am surprised that an nsIFrame can be moved into another document based on a different nsIWidget. I don't think an nsIFrame can be moved into a different document.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)

I am not familiar with nsIWidget setup to answer the question. I am surprised that an nsIFrame can be moved into another document based on a different nsIWidget. I don't think an nsIFrame can be moved into a different document.

I mean, a pres shell associated to a different document.

Comment 18

[Sotaro Ikeda [:sotaro]]

nsMenuPopupFrame::EnsureWidget() seems to request to re-create a widget.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Indeed, it changes the associated nsIWidget. So this is the EnsureWidget call in question?

Comment 20

[Timothy Nikkel (:tnikkel)]

There is nsFrameLoader::SwapWithOtherLoader which can move the document inside a frame loader keeping frames intact, I know it used to be used to swap document into a popup (and thus change the widget), I'm not sure if that still is the case.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)

Indeed, it changes the associated nsIWidget. So this is the EnsureWidget call in question?

Yes, this bug happened there.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: Bug 1635769 - Call WebRenderLayerManager::ClearCachedResources() during re-creating widget

Comment 23

[Sotaro Ikeda [:sotaro]]

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3dd35da1c8be
Call WebRenderLayerManager::ClearCachedResources() during re-creating widget r=tnikkel

Comment 25

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/3dd35da1c8be