1469528 - Crash in free.cold.85 | mozilla::wr::Moz2DRenderCallback

Status: RESOLVED FIXED

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

Description

[Darkspirit]

Seen on Socorro. There was bug 1452409 in the past.

bp-5eb53e9a-50fd-4308-97ae-b27980180618 build 20180617220505 Linux
> MOZ_RELEASE_ASSERT((uintptr_t(aPtr) - (uintptr_t(run) + bin->mRunFirstRegionOffset)) % size == 0)

Comment 1

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

Looks like this is in jemalloc: https://searchfox.org/mozilla-central/rev/f822a0b61631cbb38901569e69b4967176314aa8/memory/build/mozjemalloc.cpp#3428

Comment 2

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

Oh, I guess based on https://bugzilla.mozilla.org/show_bug.cgi?id=1452409#c3 this might be us abusing jemalloc rather than a bug in jemalloc itself.

Comment 3

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

I can repro this on a macOS local build by scrolling down on https://bug1294843.bmoattachments.org/attachment.cgi?id=8780694

Comment 4

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

Investigation so far shows that the recorded command stream is bad, in that there is a PopClip early on without a preceding PushClip/PushClipREct. And so while translating the command stream we pop the clip from [1], and then after we're done the translation we try popping it again and that crashes.

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/webrender_bindings/Moz2DImageRenderer.cpp#272

Comment 5

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

I think this is a bug in the blob merging algorithm. The blobs generated on the client side seem to be fine in that they don't try to Pop something that wasn't pushed, but after merging the blob update message into the blob image already on the parent side, it ends up with a command sequence that has a pop before a push.

Comment 6

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

I *think* the problem is that after doing the Save() call at [1], we're not flushing the item. So the PushClip that can happen on the DT as part of Save() gets smushed into some other item. Same goes for the Restore(). So then during blob merging we can end up with the PushClip/PopClip getting moved around and that causes the problem. Throwing in some FlushItem() calls after those Save() and Restore() calls seems to fix it for me, but I'll test this more thoroughly tomorrow.

[1] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/gfx/layers/wr/WebRenderCommandBuilder.cpp#754

Comment 7

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

Attachment: Bug 1469528 - Flush the clip push/pop draw commands so they don't end up in the wrong entry.

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

Comment 8

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

Pretty sure this is the right thing, although I really don't have much understanding of this code.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

I'll look at this tomorrow.

Comment 10

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

Sure, no rush. This isn't a very high volume crash.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8986936 [details]
Bug 1469528 - Flush the clip push/pop draw commands so they don't end up in the wrong entry.

https://reviewboard.mozilla.org/r/252168/#review263218

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:755
(Diff revision 1)
>        DisplayItemClip currentClip = aItem->GetClip();
>  
>        gfx::Matrix matrix;
>        if (currentClip.HasClip()) {
>          aContext->Save();
> +        aContext->GetDrawTarget()->FlushItem(aItemBounds);

I think the FlushItem should actually come after the ApplyTo because we want to include that clipping operation in that Transform item and not in the following item.

Comment 12

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

Comment on attachment 8986936 [details]
Bug 1469528 - Flush the clip push/pop draw commands so they don't end up in the wrong entry.

https://reviewboard.mozilla.org/r/252168/#review263480

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:755
(Diff revision 1)
>        DisplayItemClip currentClip = aItem->GetClip();
>  
>        gfx::Matrix matrix;
>        if (currentClip.HasClip()) {
>          aContext->Save();
> +        aContext->GetDrawTarget()->FlushItem(aItemBounds);

Ok, that makes sense. I was also wondering about the aContext->SetMatrix(matrix) call on line 770, in the case where the item doesn't have a clip. Does that need to be flushed as well? Otherwise that SetTransform operation might end up in the wrong item as well.

Comment 13

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

Comment on attachment 8986936 [details]
Bug 1469528 - Flush the clip push/pop draw commands so they don't end up in the wrong entry.

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

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8986936 [details]
Bug 1469528 - Flush the clip push/pop draw commands so they don't end up in the wrong entry.

https://reviewboard.mozilla.org/r/252168/#review263524

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> 
> Ok, that makes sense. I was also wondering about the
> aContext->SetMatrix(matrix) call on line 770, in the case where the item
> doesn't have a clip. Does that need to be flushed as well? Otherwise that
> SetTransform operation might end up in the wrong item as well.

The transform is recorded at each item boundary so we don't have to have separate items for matrix manipulations.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5953de4f6aa8
Flush the clip push/pop draw commands so they don't end up in the wrong entry. r=jrmuizel

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/5953de4f6aa8