Bug 278402 – [GTK] Crash in FenceMonitor::addFileDescriptor when using Google Maps

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[darkblaze69]

Created attachment 472249 [details]
gdb-93827.log

* Epiphany 47.beta
* WebKitGTK 2.45.90
* GStreamer 1.24.6
* GTK 4.15.5-106-a196bf1132
* Libadwaita 1.6.beta
* Distributor: "Arch Linux"

Not sure, maybe its after updating to WebKitGTK 2.45.90.
Crash in Epiphany when using Google Maps.

                Stack trace of thread 93827:
                #0  0x00007f0997b593f4 __pthread_kill_implementation (libc.so.6 + 0x963f4)
                #1  0x00007f0997b00120 __GI_raise (libc.so.6 + 0x3d120)
                #2  0x00007f0997ae74c3 __GI_abort (libc.so.6 + 0x244c3)
                #3  0x00007f09927bbbb8 _Z16WTFCrashWithInfoiPKcS0_i (libwebkitgtk-6.0.so.4 + 0x1bbbb8)
                #4  0x00007f0992a6b32d _ZZN3IPC18callMemberFunctionIN6WebKit29AcceleratedBackingStoreDMABufES2_FvmON7WebCore6RegionEON3WTF18UnixFileDescriptorEESt5tupleIJmS4_S7_EEEEvPT_MT0_T1_OT2_ENKUlDpOT_E_clIJmS4_S7_EEEDaSL_ (libwebkitgtk-6.0.so.4 + 0x46b32d)
                #5  0x00007f0992d43ced _ZN3IPC18MessageReceiverMap15dispatchMessageERNS_10ConnectionERNS_7DecoderE (libwebkitgtk-6.0.so.4 + 0x743ced)
                #6  0x00007f0992e864a7 _ZThn32_N6WebKit15WebProcessProxy17didReceiveMessageERN3IPC10ConnectionERNS1_7DecoderE (libwebkitgtk-6.0.so.4 + 0x8864a7)
                #7  0x00007f0992d3ef75 _ZN3IPC10Connection15dispatchMessageEN3WTF9UniqueRefINS_7DecoderEEE (libwebkitgtk-6.0.so.4 + 0x73ef75)
                #8  0x00007f0992d3fab4 _ZN3IPC10Connection24dispatchIncomingMessagesEv (libwebkitgtk-6.0.so.4 + 0x73fab4)
                #9  0x00007f09920472fe _ZNK3WTF8FunctionIFvvEEclEv (libjavascriptcoregtk-6.0.so.1 + 0x1a472fe)
                #10 0x00007f099211314a operator() (libjavascriptcoregtk-6.0.so.1 + 0x1b1314a)
                #11 0x00007f0992113c5c operator() (libjavascriptcoregtk-6.0.so.1 + 0x1b13c5c)
                #12 0x00007f0998a09459 g_main_dispatch (libglib-2.0.so.0 + 0x5d459)
                #13 0x00007f0998a6c0f7 g_main_context_dispatch_unlocked (libglib-2.0.so.0 + 0xc00f7)
                #14 0x00007f0998a08955 g_main_context_iteration (libglib-2.0.so.0 + 0x5c955)
                #15 0x00007f0998c39b36 g_application_run (libgio-2.0.so.0 + 0xdeb36)
                #16 0x0000562e1d6be6b6 main (epiphany + 0x36b6)
                #17 0x00007f0997ae8e08 __libc_start_call_main (libc.so.6 + 0x25e08)
                #18 0x00007f0997ae8ecc __libc_start_main_impl (libc.so.6 + 0x25ecc)
                #19 0x0000562e1d6beb05 _start (epiphany + 0x3b05)


Full gdb log in attachment.

[Michael Catanzaro]

So AcceleratedBackingStoreDMABuf::frame calls FenceMonitor::addFileDescriptor with renderingFenceFD = 0, which is illegal. And the bogus fd is coming directly from the web process.

This shouldn't result in a UI process crash, though. It should result in a web process kill. We are missing MESSAGE_CHECK() here.

[Carlos Garcia Campos]

This happened to me once, but it's not easy to reproduce. The problem is actually earlier, frame is called when we already have a pending buffer, which is not expected to happen, but in this case it's protected by a debug assert, so release builds don't catch it. I think I know how this can happen, but I haven't been able to reproduce it again to confirm it.

[Carlos Garcia Campos]

Pull request: https://github.com/WebKit/WebKit/pull/32522

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #1)
> So AcceleratedBackingStoreDMABuf::frame calls
> FenceMonitor::addFileDescriptor with renderingFenceFD = 0, which is illegal.
> And the bogus fd is coming directly from the web process.

No, renderingFenceID is a valid fd, the asserts checks that the monitor is not processing a previous fence while a new one is added, because we only allow one frame at a time. As I explained in the commit message, we have an assert to check that in AcceleratedBackingStoreDMABuf::frame(), but it's not a release assert.

> This shouldn't result in a UI process crash, though. It should result in a
> web process kill. We are missing MESSAGE_CHECK() here.

[EWS]

Committed 282608@main (802d54adc70e): <https://commits.webkit.org/282608@main>

Reviewed commits have been landed. Closing PR #32522 and removing active labels.

[Carlos Garcia Campos]

*** Bug 278515 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #4)
> No, renderingFenceID is a valid fd, the asserts checks that the monitor is
> not processing a previous fence while a new one is added, because we only
> allow one frame at a time. As I explained in the commit message, we have an
> assert to check that in AcceleratedBackingStoreDMABuf::frame(), but it's not
> a release assert.

I misread the code. :(