1450162 - Crash in mozalloc_abort | abort | _$LT$webrender_bindings..moz2d_renderer..Moz2dImageRenderer$u20$as$u20$webrender_api..image..BlobImageRenderer$GT$::update::h24630926a4127e2a

Status: RESOLVED FIXED

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

Description

[Darkspirit]

Seen on Socorro. One crash (so far).

bp-8b51fdde-1600-4b61-8b39-1a1c00180329 build 2018-03-28_141001 MacOS
> Unexpectedly empty result. This blob should just have been deleted

This crash reason was introduced by bug 1388842. The wording triggered me to file this bug.

Comment 2

[Darkspirit]

found on about:crashes:
bp-3b952fe3-1629-42d5-81ff-67f7a0180331 build 2018-03-31_101347 (main profile: gfx.webrender.blob.invalidation;true etc.)
[@ mozalloc_abort | abort | libxul.so@0x3d6c728 | libxul.so@0x3d6c718 | libxul.so@0x3d5c270 | webrender_bindings::moz2d_renderer::{{impl}}::update ]
> Unexpectedly empty result. This blob should just have been deleted

Comment 3

[Darkspirit]

Attachment: 2018-04-03_22-37-34.mp4

Nightly 61 x64 20180403100105 de_DE 4a3275936ddf871103b53e00608e2b8d5aee7e69 @ Debian Testing, KDE, Radeon RX480, 2560x1440
fresh profile: gfx.webrender.all + gfx.webrender.blob.invalidation

So far I couldn't reproduce with gfx.webrender.blob.invalidation;false (default).

http://www.spiegel.de/wirtschaft/soziales/hartz-iv-grosse-mehrheit-will-grundsaetzliche-aenderung-beim-arbeitslosengeld-ii-a-1201002.html

bp-d9873ab9-9274-4277-a138-3dde20180403 [@ mozalloc_abort | abort | libxul.so@0x3d69b08 | libxul.so@0x3d69af8 | libxul.so@0x3d59650 | libxul.so@0x3d59495 | libxul.so@0x3d59429 | webrender_bindings::moz2d_renderer::{{impl}}::update ]
> MOZ_CRASH Reason 	assertion failed: `(left == right)`

Comment 4

[Jeff Muizelaar [:jrmuizel]]

I was able to reproduce this.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Thanks for finding steps to do so.

Comment 6

[Darkspirit]

https://crash-stats.mozilla.com/search/?moz_crash_reason=%3Dassertion%20failed%3A%20%60%28left%20%3D%3D%20right%29%60&product=Firefox&date=%3E%3D2018-03-20T22%3A17%3A38.000Z&date=%3C2018-04-03T23%3A17%3A38.000Z&_sort=-date&_facets=signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

> assertion failed: `(left == right)`
also appeared in bug 1446286 and bug 1442921. (But I still couldn't reproduce comment 3 without blob invalidation so far.)

Comment 7

[Jeff Muizelaar [:jrmuizel]]

It looks like display items are being reordered without us including them invalid area.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

I have an rr recording of this happening. While stepping through it realized that it might be possible for this reordering to happen when using retained display lists. The current blob merging code has an assumption that this won't happen.

Comment 9

[Darkspirit]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #3)
> Created attachment 8964693 [details]

gfx.webrender.all;true = good
gfx.webrender.all;true + gfx.webrender.blob.invalidation;true = visual bug + crash
gfx.webrender.all;true + gfx.webrender.blob.invalidation;true + layout.display-list.retain;false = visual bug + no crash

In the video you can see that sometimes when hovering over the diagram the appearing box has a thicker border/shadow for a moment. (blob invalidation bug)

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1450162. Add a crash test that invalidates our previous assumptions.

This will make things a bit cleaner when interacting with the upcoming cache.

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

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Attachment: Bug 1450162. Correctly merge in the presence of reorderings.

Previously we were assuming that we'd always get the display list in
a canonical ordering. With retained display lists this assumption is false.

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

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8965098 [details]
Bug 1450162. Correctly merge in the presence of reorderings.

https://reviewboard.mozilla.org/r/233798/#review239478

r+ once the logic is rewritten to look more like https://pastebin.mozilla.org/9082275 .

::: gfx/webrender_bindings/src/moz2d_renderer.rs:267
(Diff revision 1)
> +    // We use a BTree as a kind of multi-map by appending cache_order to the key
> +    // this lets us multiple items with matching bounds in the map and allows
> +    // us to fetch and remove them while retaining the ordering of the original list

Make this:

    // We use a BTree as a kind of multi-map, by appending an integer "cache_order" to the key.
    // This lets us use multiple items with matching bounds in the map and allows
    // us to fetch and remove them while retaining the ordering of the original list.

There was a missing "use" in there.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8965097 [details]
Bug 1450162. Add a crash test that invalidates our previous assumptions.

https://reviewboard.mozilla.org/r/233796/#review239480

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f967d0a02df
Use an Entry type instead of tuples. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6ec4f3984e
Correctly merge in the presence of reorderings. r=mstange

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8965097 [details]
Bug 1450162. Add a crash test that invalidates our previous assumptions.

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

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Discovering that overlay scrollbars disable partial updates made building the crash test take way longer than I would've liked.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8965097 [details]
Bug 1450162. Add a crash test that invalidates our previous assumptions.

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

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8965097 [details]
Bug 1450162. Add a crash test that invalidates our previous assumptions.

https://reviewboard.mozilla.org/r/233796/#review239518

Looks great!

I would be happier if you declared all the local variables with a var or a let or a const, though.

::: layout/svg/crashtests/blob-merging-and-retained-display-list.html:6
(Diff revision 3)
> +<!doctype html>
> +<html class="reftest-wait">
> +<script>
> +        var r = 40;
> +        var xmlns = "http://www.w3.org/2000/svg";
> +        raf = requestAnimationFrame;

This happens to work but it makes me a little queasy. Could you change it to
const raf = f => window.requestAnimationFrame(f);
?

::: layout/svg/crashtests/blob-merging-and-retained-display-list.html:9
(Diff revision 3)
> +        var r = 40;
> +        var xmlns = "http://www.w3.org/2000/svg";
> +        raf = requestAnimationFrame;
> +        function rect(x, y) {
> +            var r = document.createElementNS (xmlns, "rect");
> +            r.setAttributeNS (null, "x", x);

What does passing null mean, here? Does it end up doing something different then just plain setAttribute?

::: layout/svg/crashtests/blob-merging-and-retained-display-list.html:22
(Diff revision 3)
> +            x = rect(110, 110);
> +            y = rect(120, 120);
> +            z = rect(130, 130);
> +            w = rect(140, 140);
> +            x2 = rect(310, 140);
> +            y2 = rect(320, 130);
> +            z2 = rect(330, 120);
> +            w2 = rect(340, 110);

maybe call these a/b/c/d? and rename the existing c to svg?

::: layout/svg/crashtests/blob-merging-and-retained-display-list.html:36
(Diff revision 3)
> +                c.appendChild(x);
> +                c.appendChild(y);
> +                c.appendChild(z);
> +                c.appendChild(w);
> +                raf(() => {
> +                    // the display list partial update will end up with these items before x,y,w,z

Crucial comment. Thanks.

::: layout/svg/crashtests/blob-merging-and-retained-display-list.html:60
(Diff revision 3)
> +
> +        onload = f;
> +</script>
> +
> +<body>
> +<svg height="600" width="700" id=c>

Could you switch the order of width and height? I was a bit confused for a second.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1307a26d167
Add a crash test that invalidates our previous assumptions. r=mstange

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/6f967d0a02df
https://hg.mozilla.org/mozilla-central/rev/3b6ec4f3984e
https://hg.mozilla.org/mozilla-central/rev/f1307a26d167

Comment 21

[Darkspirit]

(In reply to Jan Andre Ikenmeyer [:darkspirit] from comment #3)
> http://www.spiegel.de/wirtschaft/soziales/hartz-iv-grosse-mehrheit-will-grundsaetzliche-aenderung-beim-arbeitslosengeld-ii-a-1201002.html

I can't reproduce the crash from comment 3 anymore. :) But I found another website that still crashes: bug 1451458