Bug 303103 – Crash in WebKit::WebPageProxy::resetStateAfterProcessExited when destroying ProvisionalPageProxy

• Component: WebKit2
• Status: RESOLVED FIXED

[Michael Catanzaro]

Created attachment 477515 [details]
Full backtrace

Using Epiphany Tech Preview with WebKitGTK 2.51.1, after a web process crash due to an unrelated bug (bug #303057), my UI process unexpectedly crashed in WebKit::WebPageProxy::resetStateAfterProcessExited when destroying m_provisionalPage:

(gdb) bt
#0  0x00007fd3fcbc2a82 in WebKit::WebPageProxy::resetStateAfterProcessExited
    (this=0x7fd3ea1e4200, terminationReason=WebKit::ProcessTerminationReason::Crash)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebKit/UIProcess/ProvisionalPageProxy.h:90
#1  0x00007fd3fcba24d2 in WebKit::WebPageProxy::resetStateAfterProcessTermination
    (this=0x7fd3ea1e4200, reason=WebKit::ProcessTerminationReason::Crash)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebKit/UIProcess/WebPageProxy.cpp:11579
#2  0x00007fd3fcc1f590 in WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch(WebKit::ProcessTerminationReason)::$_1::operator()<WTF::Ref<WebKit::WebPageProxy, WTF::RawPtrTraits<WebKit::WebPageProxy>, WTF::DefaultRefDerefTraits<WebKit::WebPageProxy> > >(WTF::Ref<WebKit::WebPageProxy, WTF::RawPtrTraits<WebKit::WebPageProxy>, WTF::DefaultRefDerefTraits<WebKit::WebPageProxy> >&) const
    (page=..., this=<optimized out>) at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebKit/UIProcess/WebProcessProxy.cpp:1342
#3  WTF::Mapper<WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch(WebKit::ProcessTerminationReason)::$_1, WTF::Vector<WebKit::PageLoadState::Transaction, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WTF::Vector<WTF::Ref<WebKit::WebPageProxy, WTF::RawPtrTraits<WebKit::WebPageProxy>, WTF::DefaultRefDerefTraits<WebKit::WebPageProxy> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&>::map(WTF::Vector<WebKit::PageLoadState::Transaction, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WTF::Vector<WTF::Ref<WebKit::WebPageProxy, WTF::RawPtrTraits<WebKit::WebPageProxy>, WTF::DefaultRefDerefTraits<WebKit::WebPageProxy> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch(WebKit::ProcessTerminationReason)::$_1 const&)
    (source=<optimized out>, result=<optimized out>, mapFunction=<optimized out>) at WTF/Headers/wtf/Vector.h:1810
#4  WTF::map<0ul, WTF::CrashOnOverflow, 16ul, WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch(WebKit::ProcessTerminationReason)::$_1, WTF::Vector<WTF::Ref<WebKit::WebPageProxy, WTF::RawPtrTraits<WebKit::WebPageProxy>, WTF::DefaultRefDerefTraits<WebKit::WebPageProxy> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> >(WTF::Vector<WTF::Ref<WebKit::WebPageProxy, WTF::RawPtrTraits<WebKit::WebPageProxy>, WTF::DefaultRefDerefTraits<WebKit::WebPageProxy> >, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch(WebKit::ProcessTerminationReason)::$_1&&) (source=<optimized out>, mapFunction=<optimized out>)
    at WTF/Headers/wtf/Vector.h:1834
#5  WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch (this=0x7fd3ea3c1200, reason=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebKit/UIProcess/WebProcessProxy.cpp:1340
#6  0x00007fd3fcada120 in IPC::Connection::dispatchDidCloseAndInvalidate()::$_0::operator()() const (this=0x7fd372540638)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WebKit/Platform/IPC/Connection.cpp:1311
#7  WTF::Detail::CallableWrapper<IPC::Connection::dispatchDidCloseAndInvalidate()::$_0, void>::call (this=0x7fd372540630)
    at WTF/Headers/wtf/Function.h:59
#8  0x00007fd3fb61ee55 in WTF::Function<void()>::operator() (this=0x7ffdc67416b0)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WTF/wtf/Function.h:103
#9  WTF::RunLoop::performWork (this=0x7fd3eb014180) at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WTF/wtf/RunLoop.cpp:148
#10 0x00007fd3fb6e566d in WTF::RunLoop::RunLoop()::$_0::operator()(void*) const (userData=0x7fd3ea984000, 
    userData@entry=0x7fd3eb014180, this=<optimized out>)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WTF/wtf/glib/RunLoopGLib.cpp:80
#11 WTF::RunLoop::RunLoop()::$_0::__invoke(void*) (userData=0x7fd3ea984000)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WTF/wtf/glib/RunLoopGLib.cpp:79
#12 0x00007fd3fb6e3d11 in WTF::RunLoop::$_1::operator()
    (source=0x56386215f4a0, callback=0x7fd3fb6e5660 <WTF::RunLoop::RunLoop()::$_0::__invoke(void*)>, userData=0x7fd3eb014180, this=<optimized out>) at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WTF/wtf/glib/RunLoopGLib.cpp:57
#13 WTF::RunLoop::$_1::__invoke
    (source=0x56386215f4a0, callback=0x7fd3fb6e5660 <WTF::RunLoop::RunLoop()::$_0::__invoke(void*)>, userData=0x7fd3eb014180)
    at /usr/lib/debug/source/sdk/webkitgtk-6.0.bst/Source/WTF/wtf/glib/RunLoopGLib.cpp:49
#14 0x00007fd4029027cb in g_main_dispatch (context=context@entry=0x563862138cb0) at ../glib/gmain.c:3565
#15 0x00007fd402905bd7 in g_main_context_dispatch_unlocked (context=0x563862138cb0) at ../glib/gmain.c:4425
#16 g_main_context_iterate_unlocked
    (context=context@entry=0x563862138cb0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:4490
#17 0x00007fd402906433 in g_main_context_iteration (context=context@entry=0x563862138cb0, may_block=may_block@entry=1)
    at ../glib/gmain.c:4556
#18 0x00007fd402afeb0d in g_application_run (application=0x56386215d600 [EphyShell], argc=<optimized out>, argv=<optimized out>)
    at ../gio/gapplication.c:2741
#19 0x0000563823d131cf in main (argc=<optimized out>, argv=<optimized out>) at ../src/ephy-main.c:455

Full backtrace attached. Unfortunately it does not indicate what has actually gone wrong. Presumably some member of the ProvisionalPageProxy is invalid, but there is no indication which it might be. Without a better indication of what went wrong, I don't know how we could debug this.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Using Epiphany Tech Preview with WebKitGTK 2.51.1, 

Whoops, I am actually using WebKitGTK 2.51.2.

[Radar WebKit Bug Importer]

<rdar://problem/165701790>

[Michael Catanzaro]

I hit this again today. I got confused because the file name and line number are wrong in the 0th frame of the stack trace due to inlining. It must be crashing here:

    if (terminationReason != ProcessTerminationReason::NavigationSwap)
        m_provisionalPage = nullptr;

I see I even wrote that in my first comment, above. Unfortunately, without any reproducer, it's probably hopeless to try guessing what is wrong.

[Michael Catanzaro]

I've been hitting this myself quite frequently recently. We may need to add additional diagnostics. I think some unknown member of ProvisionalPageProxy is invalid, but we are unlikely to be able to guess which one. What might work is: adding code to the destructor to manually destroy each data member, in the hopes that a stack trace will point us to the exact line where something goes wrong, instead of the class declaration ProvisionalPageProxy.h:90.

[Michael Catanzaro]

Got a reliable reproducer:

 * Build 2.51.93
 * Open a bunch of tabs; I've found multiple that almost always trigger bug #310304, causing a web process crash
 * Close all the tabs

Plot twist: it's not crashing when destroying m_provisionalPage. Rather, the line number is misleading. It actually crashes when calling protectedPageClient->processDidExit(), because the PageClient gets destroyed *before* WebPageProxy::resetStateAfterProcessExited is called. WebPageProxy::pageClient returns nullptr, so protectedPageClient is protecting nullptr. Alas.

That's not surprising, because the lifetime of a PageClient corresponds to the lifetime of a web view, and when closing tabs it's expected that the web view gets destroyed.

P.S. For my reproducer, I've been loading: https://www.reddit.com/, https://כולנו.ישראל/, and https://www.cnn.com/middleeast/live-news/israel-hamas-war-gaza-news-05-06-24-intl-hnk/index.html, and https://www.cnn.com/, among other pages, because these pages are good at triggering bug #310304.

[Michael Catanzaro]

Workaround is:

diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
index 1c80308fb3f8..73ade752bc2a 100644
--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
@@ -12222,12 +12222,14 @@ void WebPageProxy::resetStateAfterProcessExited(ProcessTerminationReason termina
     if (terminationReason != ProcessTerminationReason::NavigationSwap)
         m_provisionalPage = nullptr;
 
-    if (terminationReason == ProcessTerminationReason::NavigationSwap)
-        protectedPageClient->processWillSwap();
-    else
-        protectedPageClient->processDidExit();
+    if (protectedPageClient) {
+        if (terminationReason == ProcessTerminationReason::NavigationSwap)
+            protectedPageClient->processWillSwap();
+        else
+            protectedPageClient->processDidExit();
 
-    protectedPageClient->clearAllEditCommands();
+        protectedPageClient->clearAllEditCommands();
+    }
 
 #if PLATFORM(COCOA)
     WebPasteboardProxy::singleton().revokeAccess(m_legacyMainFrameProcess.get());

[Michael Catanzaro]

Hm, I think that's actually a correct fix, because protectedPageClient uses RefPtr, not Ref, indicating it's intentionally allowed to be nullptr. And it's a common pattern throughout this file to check validity before using the PageClient. And there's surely no need to call PageClient vfuncs after the PageClient is destroyed; that functionality is only important if the web view still exists, but it doesn't. So I will create a pull request.

I tried to figure out what has changed to introduce the crash, but I don't see any suspicious commits, and I don't think it's worth bisecting since we have a solution.

[Michael Catanzaro]

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

[Michael Catanzaro]

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

[EWS]

Committed 309620@main (fc2c9a74dbf1): <https://commits.webkit.org/309620@main>

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