1717889 - MOZ_RELEASE_ASSERT(NextSurface(lock)) Crash in [@ mozilla::layers::NativeLayerCA::NextSurfaceAsFramebuffer]

Status: VERIFIED FIXED

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

Description

[Jamie Nicol [:jnicol]]

Attachment: about:support

Nightly won't stop crashing for me today. I managed to use it for about half an hour or so until the first crash. But now I can reproduce just by restoring my session and switching tabs a couple of times.

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/ec7ed16c-7407-4803-9909-f495c0210623

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(NextSurface(lock)) (NextSurfaceAsFramebuffer needs a surface.)

Top 10 frames of crashing thread:

0 XUL mozilla::layers::NativeLayerCA::NextSurfaceAsFramebuffer gfx/layers/NativeLayerCA.mm:717
1 XUL mozilla::wr::RenderCompositorNativeOGL::InitDefaultFramebuffer gfx/webrender_bindings/RenderCompositorNative.cpp:458
2 XUL mozilla::wr::RenderCompositorNative::BeginFrame gfx/webrender_bindings/RenderCompositorNative.cpp:68
3 XUL mozilla::wr::RenderThread::UpdateAndRender gfx/webrender_bindings/RenderThread.cpp:485
4 XUL mozilla::wr::RenderThread::HandleFrameOneDoc gfx/webrender_bindings/RenderThread.cpp:340
5 XUL mozilla::detail::RunnableMethodImpl<mozilla::wr::RenderThread*, void  xpcom/threads/nsThreadUtils.h:1203
6 XUL MessageLoop::DoWork ipc/chromium/src/base/message_loop.cc:538
7 XUL base::MessagePumpDefault::Run ipc/chromium/src/base/message_pump_default.cc:35
8 XUL MessageLoop::Run ipc/chromium/src/base/message_loop.cc:306
9 XUL base::Thread::ThreadMain ipc/chromium/src/base/thread.cc:187

Comment 1

[Jamie Nicol [:jnicol]]

I didn't mean to set the priority. (It doesn't seem to be affecting many other users so probably not super severe)

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Can you try getting a regression window using a clone of your profile?

Comment 3

[Chris Peterson [:cpeterson]]

This crash doesn't look like a Fission bug, even though comment 0's crash report says DOMFissionEnabled=1.

Over the last six months, there are crash reports from builds as old as Firefox 72.0.1.

Comment 4

[Chris Peterson [:cpeterson]]

A NextSurfaceAsFramebuffer crash bug 1578376 was fixed in 71.

Comment 5

[Jamie Nicol [:jnicol]]

Mozregression didn't help, but I realize I had set gfx.webrender.compositor to false to try and reproduce a different bug. Resetting that fixes the crash.

Not sure if this is still worth investigating further.

Comment 6

[Brad Werth [:bradwerth]]

Jamie, can you recover the gfx log from one of the crashes, such that we can see if NextSurface is returning null because of a bad mSize?

Comment 7

[Mike Kaply [:mkaply]]

I'm seeing this with gfx.webrender.compositor set to false as well (constantly) and that's a problem because we have customers that need to set gfx.webrender.compositor to false for color management issues.

My crashes are:

https://crash-stats.mozilla.org/report/index/3e5c0fbf-f06d-4866-9b7a-75e3c0210709
https://crash-stats.mozilla.org/report/index/5452c131-d845-4c2c-a4c0-8c7c90210709
https://crash-stats.mozilla.org/report/index/0d924aed-db38-4d0d-9060-0c01f0210709

Let me know if there's anything I can do to debug. I can recreate at will.

Oddly it happens when I pass my mouse cursor over the icon of the addon I built specifically to flip the color management preferences.

But no code is being executed when I move the mouse over. And if I modify the addon just have images, it doesn't happen.

Comment 8

[Mike Kaply [:mkaply]]

Mozregression is being problematic, but it seems to point to something around mid January 2021.

Comment 9

[Mike Kaply [:mkaply]]

mozregression narrowed it down to:

Bug 1685046: Hoist release asserts in RenderCompositorNativeOGL::Bind to crash earlier. r=jrmuizel
This change spreads the release asserts to functions that are invoked by other
callers, potentially increasing the cases where the assert will fail. This is
being done intentionally; we want additional information on the reasons for
the crashes.
Differential Revision: https://phabricator.services.mozilla.com/D101063

Comment 10

[Mike Kaply [:mkaply]]

Attachment: color_management-0.3-fx.xpi

I did a debug build and here is the exact error.

Crash Annotation GraphicsCriticalError: |[0][GFX1]: NextSurface returning false because of invalid mSize (0, 0). (t=12.4048) [GFX1]: NextSurface returning false because of invalid mSize (0, 0).

This is easy to recreate with attached extension.

Install it, click the button in the extension to disable color management, restart Firefox (icon should be green)

Then hover over the icon.

Comment 11

[Mike Kaply [:mkaply]]

I've verified this happens with SVG and PNG, and it only happens if the addon has a popup.html associated with the icon. And I've verified it with other WebExtension (not specific to mine).

Comment 12

[Glenn Watson [:gw]]

Any ideas Jeff or Markus?

Comment 13

[Robert Mader [:rmader]]

FTR., it looks like it should be easy to track down by adding an assert in https://searchfox.org/mozilla-central/source/gfx/layers/NativeLayerCA.mm#469-476 for a non-empty mSize. If that's not triggered, then we use the other constructor, which again is only used by webrender::composite::Compositor::create_external_surface, in which case we should never call NextSurface(). I just looked into it because I still wonder when we actually create_external_surface (it's not yet implemented on Wayland).

Comment 14

[Markus Stange [:mstange]]

Bug 1685046 turned a fallible state (zero-sized surface) into a crashing state. I'm not sure if that was the right idea. I think we have two options:

• Make NextSurface and NextSurfaceAsFramebuffer gracefully handle zero-sized surfaces (by returning Nothing()), or
• Document NextSurface and NextSurfaceAsFramebuffer as illegal to call on zero-sized surfaces, and make sure that all callers check the surface size before calling these methods.

Comment 15

[Markus Stange [:mstange]]

The crashing call is this one: https://searchfox.org/mozilla-central/rev/b79212b4fc017f27ac2435a658d4e9b9798efa86/gfx/webrender_bindings/RenderCompositorNative.cpp#458

Comment 16

[Markus Stange [:mstange]]

Attachment: Bug 1717889 - Make sure we don't call NextSurfaceAsFramebuffer on zero-sized native layers. r=jrmuizel

Comment 17

[Glenn Watson [:gw]]

From the WR perspective, I wouldn't expect us to ever call this with a zero size, that would be a bug. Though, it looks like in this case it's the default frame buffer, so maybe unrelated / different to what WR does for picture cache tiles (I haven't looked closely at all the comments, might be missing something too).

Comment 18

[Mike Kaply [:mkaply]]

I can verify that patch fixes the crash.

From the WR perspective, I wouldn't expect us to ever call this with a zero size, that would be a bug. Though, it looks like in this case it's the default frame buffer, so maybe unrelated / different to what WR does for picture cache tiles (I haven't looked closely at all the comments, might be missing something too).

Let me know if you'd like me to look at something further or give you more debug info.

Comment 19

[Markus Stange [:mstange]]

(In reply to Glenn Watson [:gw] from comment #17)

Though, it looks like in this case it's the default frame buffer, so maybe unrelated

Exactly; this crash happens if the webrender compositor is off, so the only "native layer" is the default framebuffer.

Comment 20

[Markus Stange [:mstange]]

I've filed bug 1720206 about the fact that we try to composite a 0x0 window in the first place.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 9230815 [details]
Bug 1717889 - Make sure we don't call NextSurfaceAsFramebuffer on zero-sized native layers. r=jrmuizel

Beta/Release Uplift Approval Request

• User impact if declined: Intermittent crashes with a non-default pref (gfx.webrender.compositor=false) when add-ons with panels are used.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 10
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small fix, only affects the off-by-default codepath.
• String changes made/needed:

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/283bfe656d3a
Make sure we don't call NextSurfaceAsFramebuffer on zero-sized native layers. r=mkaply

Comment 23

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/283bfe656d3a

Comment 24

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9230815 [details]
Bug 1717889 - Make sure we don't call NextSurfaceAsFramebuffer on zero-sized native layers. r=jrmuizel

Fix for a rare crash on beta but the patch is small, doesn't look risky and we are still in the early beta cycle, approved for 91.0b5, thanks.

Comment 25

[Pascal Chevrel (relman team) -> :pascalc]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3b146d60d28

Comment 26

[Catalin Sasca, Desktop Test Engineering [:csasca]]

Reproduced the crash on Firefox 91.0a1 (2021-06-23) under macOS 11.4 by using the addon from Comment 10 and webrender compositing set to False.

The crash is no longer reproducible on Firefox 91.0b5 (treeherder build) and Firefox 92.0a1 (2021-07-19). Tests were performed on macOS 11.4, Ubuntu 20.04 and Windows 10.