1452513 - blob-invalidation: Crash in mozalloc_abort | abort | libxul.so@0x3d1f718 | libxul.so@0x3d1f708 | libxul.so@0x3d0f260 | webrender::resource_cache::ResourceCache::update_resources

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Darkspirit]

Nightly 61 x64 20180408100251 de_DE @ Debian Testing, KDE, Radeon RX480
main profile: gpu process, webrender, blob-invalidation, etc.

I wanted to know how I could make a Gtk window with Rust, so I opened some examples. Then I dragged a tab of https://github.com/ab0v3g4me/epicwar-downloader a bit down to open it in a new window. I grabbed the tab there and placed it at back to the original position in the first window. Then I did a middleclick to activate autoscroll and scrolled a bit down. I could reproduce the crash once. (Only tested once.)

bp-d4184df8-9c01-4fcf-b637-cca210180408	09.04.18 00:08
bp-0e17119d-53c7-4726-bc4c-6e0e30180408	09.04.18 00:06
> MOZ_CRASH Reason 	Attempt to update non-existent image

https://github.com/servo/webrender/blob/df73569c5a9fba35a7c268d176a3eb0d6c52b249/webrender/src/resource_cache.rs#L501

Comment 1

[Darkspirit]

fresh profile: gfx.webrender.all, gfx.webrender.blob.invalidation
Open 2 tabs of https://github.com/ab0v3g4me/epicwar-downloader, move one out of the window, move the tab back to the first window. Hover something or try to scroll down. Browser crash.

Not reproducible without blob invalidation.

bp-06ef322f-eeb4-4973-a088-1d5a20180409 09.04.18 02:06
bp-2eb4d2ad-51d1-4d44-99a4-f96ec0180409 09.04.18 02:06
bp-53bcb452-bc72-4660-83a9-fbe470180409 09.04.18 02:06
bp-59c5c9d0-1f40-4764-8da4-8973f0180409 09.04.18 02:04

Comment 2

[Darkspirit]

Win10, Radeon RX480: STR from comment 1. Not reproducible without blob invalidation.
bp-422dfe39-2572-44dd-a7cf-5bf020180409 09.04.2018 16:46 [@ static void std::panicking::rust_panic_with_hook ]
> Attempt to update non-existent image

Comment 3

[Darkspirit]

Nightly 61 x64 20180412100111 de_DE 246c614e160586c1eb3167cff866dd550be35e03 @ Debian Testing, KDE, Radeon RX480
fresh profile: gfx.webrender.all, gfx.webrender.blob.invalidation

bp-72266f65-30a6-4571-b81d-bad230180412 12.04.18 17:40
> Attempt to update non-existent image

https://hg.mozilla.org/mozilla-central/graph/246c614e160586c1eb3167cff866dd550be35e03
This build includes the patch from bug 1451458.

Comment 4

[Darkspirit]

https://hg.mozilla.org/mozilla-central/rev/789e30ff2e3d6e1fcfce1a373c1e5635488d24da includes the latest WebRender update.
bp-49deb394-f7c0-4a58-8c30-3c0fa0180418 18.04.18 03:34

try build from bug 1454659 comment 6:
mozregression --repo mozilla-inbound --launch 7b816219f708 --pref gfx.webrender.all:true gfx.webrender.blob.invalidation:true startup.homepage_welcome_url:'https://github.com/ab0v3g4me/epicwar-downloader|https://github.com/ab0v3g4me/epicwar-downloader'
-> Crash. (Process exited with code 11)

Comment 5

[Darkspirit]

Not "try", I meant "inbound".

Comment 6

[Jeff Muizelaar [:jrmuizel]]

I'll take a look at this tomorrow.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I have a recording in rr. Will debug tomorrow.

Comment 8

[Martin Giger [:freaktechnik]]

This happens for me when I move a tab with video playback out of the main browser window (with WebRender enabled).

Comment 9

[Jeff Muizelaar [:jrmuizel]]

This might be fixed by bug 1453801 which landed on inbound this morning.

Comment 10

[Darkspirit]

https://hg.mozilla.org/integration/mozilla-inbound/graph/d1479b21a284

> Bug 1453801 - Part 3. Fix race condition shutting down the render thread and shared surfaces. r=sotaro
mozregression --repo mozilla-inbound --launch d1479b21a284 --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://github.com/ab0v3g4me/epicwar-downloader|https://github.com/ab0v3g4me/epicwar-downloader'
no crash

> Bug 1454398 - Comment out another invalid assert I missed in 42e037e0b8d1. r=me
mozregression --repo mozilla-inbound --launch e5b94fa417c8 --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://github.com/ab0v3g4me/epicwar-downloader|https://github.com/ab0v3g4me/epicwar-downloader'
crash

Promising, thanks! :)

Comment 11

[Darkspirit]

Seen on Socorro.
bp-11139dd5-260b-46b8-a900-6b9290180425 build 20180424220100 MacOS
> Attempt to update non-existent image

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

FWIW, I believe this is somehow related to concurrency: I seem to encounter it when I open-in-new-tab many many tabs from the same website (ie that share the same resources)

Comment 13

[Stuart Philp :sphilp]

I'm reproducing this a lot on today's nightly by having some youtube videos open and dragging a video tab to another window (2nd screen is an external monitor if that matters). As soon as I drop the tab Firefox crashes.

Comment 14

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

Saw this in a tsvgr_opacity run: https://treeherder.mozilla.org/logviewer.html#?job_id=181085651&repo=try&lineNumber=1220

Comment 15

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

I think the patch from bug 1455597 might help here. At least, I saw this crash once while trying to reproduce that crash and it seemed to be the same sort of problem.

Comment 16

[Intermittent Failures Robot]

1 failures in 655 pushes (0.002 failures/push) were associated with this bug in the last 7 days.    

Repository breakdown:
* try: 1

Platform breakdown:
* linux64-qr: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1452513&startday=2018-05-28&endday=2018-06-03&tree=trunk

Comment 17

[Darkspirit]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> I think the patch from bug 1455597 might help here. At least, I saw this crash once while trying to reproduce that crash and it seemed to be the same sort of problem.

Last crash with build 20180604100129.

Comment 18

[Darkspirit]

Someone uploaded a new report:
bp-f1e96392-4d98-46b1-b04f-679170180606 build 20180605220158 Win10

Comment 19

[Darkspirit]

Attachment: 2018-06-25_14-03-28.mp4

Ohoo, I have STR.

Debian Testing, GTX 1060

See attached video:
Fresh profile with gfx.webrender.all;true.
https://addons.mozilla.org/en-US/firefox/addon/starry-space-fantasy/
https://bug1470855.bmoattachments.org/attachment.cgi?id=8987464
https://www.gatsbyjs.org/

bp-47dcb1b8-4e88-4e97-bdd0-521e80180625 25.06.18 14:04
bp-197f5b1f-bf9b-44b3-af00-6b7fe0180625 25.06.18 14:03
bp-08862c59-166e-47c9-98f8-9dffa0180625 25.06.18 14:03
bp-aeb7c2c2-c4e2-49b6-aae5-7b23a0180625 25.06.18 14:02
bp-38d66ea6-8699-460e-a873-b691d0180625 25.06.18 14:02

Comment 20

[Darkspirit]

Win10 with GTX 1060. Same STR as comment 19, but I didn't open and install the AMO link.
bp-b33ffc9e-4140-408c-9aaf-20b520180625 25 Jun 2018 14:46
bp-0b86c0d4-e5f5-4d35-92b8-499210180625 25 Jun 2018 14:46
bp-b725d547-b7ce-4a62-99b9-82e3c0180625 25 Jun 2018 14:46
bp-b46ce205-2cfd-4a3d-83b5-0a4190180625 25 Jun 2018 14:46

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

When the WebRenderApi object is destroyed, it will trigger RenderApi::drop which clears the namespace. This can happen while the scene builder is still busy, and has yet to send its SceneBuilderResult::Transaction message. As a result we process the ApiMsg::ClearNamespace message beforehand, and then we hit the panic once the scene is ready. We need synchronization on the teardown. I'll see if I can figure it out.

Comment 22

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

I thought I already did that, at https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/layers/wr/WebRenderBridgeParent.cpp#1651-1656

Comment 23

[Andrew Osmond [:aosmond] (he/him)]

Yeah, I have that patch in my tree, but for some reason, the flush doesn't stop a transaction from starting afterwards. Event ordering is something like this:

api flush scene builder send
api flush scene builder sent
render backend send scene builder flush
scene builder transaction start
render backend sent scene builder flush
scene builder transaction end
scene builder flush start
scene builder flush end
api flush scene builder recv
clear_namespace: IdNamespace(14)
scene builder transaction start
scene builder transaction end
update_image_template: ImageKey(IdNamespace(12), 82)

Investigation continues.

Comment 24

[Andrew Osmond [:aosmond] (he/him)]

Oh I was missing the most important part of the log, because there were two flushes, whoops...

api flush scene builder send
api flush scene builder sent
render backend send scene builder flush
render backend sent scene builder flush
scene builder transaction start
scene builder transaction end
scene builder flush start
scene builder flush end
api flush scene builder recv
clear_namespace: IdNamespace(12)

api flush scene builder send
api flush scene builder sent
render backend send scene builder flush
scene builder transaction start
render backend sent scene builder flush
scene builder transaction end
scene builder flush start
scene builder flush end
api flush scene builder recv
clear_namespace: IdNamespace(14)

scene builder transaction start
scene builder transaction end
update_image_template: ImageKey(IdNamespace(12), 82)

Comment 25

[Andrew Osmond [:aosmond] (he/him)]

Adding the thread IDs to the logging proved important. It appears that the images were originally being handled on one WRRenderBackend thread, we flushed / cleared the namespace on that thread, and then we try to use the same namespace on a different WRRenderBackend thread. Thus even if we hadn't done the clear, this would have panicked since the resource caches are different. In the successful transitions, it seems to generate new keys for a new namespace.

Comment 26

[Andrew Osmond [:aosmond] (he/him)]

I think this line causes us to inject stale transaction data into the new WebRenderAPI object:

https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/layers/wr/WebRenderBridgeParent.cpp#657

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1452513-Avoid-issuing-transactions-with-WebRende.patch, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fe9e493d6e0d32ec7b33845ed1eb1f1bfea4591

Comment 28

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

Comment on attachment 8990305 [details] [diff] [review]
0001-Bug-1452513-Avoid-issuing-transactions-with-WebRende.patch, v1

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

Hm, so before I added this AutoSender thingy in https://searchfox.org/mozilla-central/diff/4cb71672840e62bff04aa2e4184f88ed048043ee/gfx/layers/wr/WebRenderBridgeParent.cpp#622 we were explicitly sending the transaction after ProcessWebRenderParentCommands, regardless of the namespace check. Do we want to restore that behaviour? The ProcessWebRenderParentCommands looks like it does some useful cleanup stuff which we probably want to still run even if the namespaces don't match.

Can you also update the commit message to specify exactly which commands (UpdateImageBuffer, per your response on IRC) were getting sent in the transaction that were undesirable in the case of a namespace change?

Comment 29

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

Comment on attachment 8990305 [details] [diff] [review]
0001-Bug-1452513-Avoid-issuing-transactions-with-WebRende.patch, v1

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

Dropping flag for now, see comment above.

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> Comment on attachment 8990305 [details] [diff] [review]
> 0001-Bug-1452513-Avoid-issuing-transactions-with-WebRende.patch, v1
> 
> Review of attachment 8990305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hm, so before I added this AutoSender thingy in
> https://searchfox.org/mozilla-central/diff/
> 4cb71672840e62bff04aa2e4184f88ed048043ee/gfx/layers/wr/WebRenderBridgeParent.
> cpp#622 we were explicitly sending the transaction after
> ProcessWebRenderParentCommands, regardless of the namespace check. Do we
> want to restore that behaviour? The ProcessWebRenderParentCommands looks
> like it does some useful cleanup stuff which we probably want to still run
> even if the namespaces don't match.
> 
> Can you also update the commit message to specify exactly which commands
> (UpdateImageBuffer, per your response on IRC) were getting sent in the
> transaction that were undesirable in the case of a namespace change?

Hmmm, so the transactions created by ProcessWebRenderParentCommands are all for pipeline removal, but any stale pipelines should have been removed by ClearResources when the namespace was changed in UpdateWebRender. Similarly any stale texture hosts and shared surfaces should have been released. However, this begs the question of should we even call ProcessWebRenderParentCommands at all. Surely we don't want to *add* stale pipelines, or compositor animations...

Comment 31

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

So looking at this more closely, you seem to be mostly right. The one discrepancy I see between ClearResources and ProcessWebRenderParentCommands is that the latter also removes the compositable pipeline [1] whereas the former does not [2]. However we can fix that pretty easily by also removing the pipeline in ClearResources. With that in place I guess your patch is fine.

The only other consideration is Sotaro's change in bug 1475187, but if I'm understanding you correctly we should be ok to skip that pipeline-addition in the case where the namespace id has changed, because it would be stale anyway. Is that right?

[1] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/gfx/layers/wr/WebRenderBridgeParent.cpp#1089
[2] https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/gfx/layers/wr/WebRenderBridgeParent.cpp#1662-1665

Comment 32

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #31)
> So looking at this more closely, you seem to be mostly right. The one
> discrepancy I see between ClearResources and ProcessWebRenderParentCommands
> is that the latter also removes the compositable pipeline [1] whereas the
> former does not [2]. However we can fix that pretty easily by also removing
> the pipeline in ClearResources. With that in place I guess your patch is
> fine.
> 

I'll wait for bug 1477571 to land.

> The only other consideration is Sotaro's change in bug 1475187, but if I'm
> understanding you correctly we should be ok to skip that pipeline-addition
> in the case where the namespace id has changed, because it would be stale
> anyway. Is that right?
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> a80651653faa78fa4dfbd238d099c2aad1cec304/gfx/layers/wr/WebRenderBridgeParent.
> cpp#1089
> [2]
> https://searchfox.org/mozilla-central/rev/
> a80651653faa78fa4dfbd238d099c2aad1cec304/gfx/layers/wr/WebRenderBridgeParent.
> cpp#1662-1665

Yes I believe so.

Comment 33

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

Comment on attachment 8990305 [details] [diff] [review]
0001-Bug-1452513-Avoid-issuing-transactions-with-WebRende.patch, v1

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

With bug 1479939 landed this patch should be fine, after appropriate rebasing.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd62249f8b51
Avoid issuing transactions with WebRender when the namespace has changed. r=kats

Comment 35

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/bd62249f8b51