Closed Bug 1469528 Opened 2 years ago Closed 2 years ago

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

Categories

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

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox61 --- disabled
firefox62 --- disabled
firefox63 --- fixed

People

(Reporter: darkspirit, Assigned: kats)

References

(Blocks 2 open bugs, )

Details

(Keywords: crash, nightly-community, Whiteboard: [gfx-noted])

Crash Data

Attachments

(1 file)

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)
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.
Component: Memory Allocator → Graphics: WebRender
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
Crash Signature: [@ free.cold.85 | mozilla::wr::Moz2DRenderCallback ]
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.
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
Pretty sure this is the right thing, although I really don't have much understanding of this code.
Assignee: nobody → bugmail
I'll look at this tomorrow.
Sure, no rush. This isn't a very high volume crash.
Priority: -- → P1
Whiteboard: [gfx-noted]
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.
Attachment #8986936 - Flags: review?(jmuizelaar) → review-
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 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
Attachment #8986936 - Flags: review?(jmuizelaar) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Pushed by kgupta@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/5953de4f6aa8
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.