Changeset 272990 in webkit

Timestamp:

Feb 17, 2021 1:38:47 AM (3 years ago)

Author:

Chris Dumez

Message:

Regression(r268097): Calling processDidTerminate delegate asynchronously is risky compatibility-wise
​https://bugs.webkit.org/show_bug.cgi?id=222011

Reviewed by Carlos Garcia Campos.

Calling processDidTerminate delegate asynchronously like we did in r268097 is risky compatibility-wise.
This caused breakage in at least 2 client applications. While this can be dealt with on the client side,
it would be better to fix what r268097 was trying to fix without making the delegate call asynchronous.
The reason calling the delegate asynchronously is risky is because some view state may have time to get
reset by the time the client gets notified on the crash, potentially confusing the crash handling logic
in the client.

No new tests, covered by WKNavigation.ReloadRelatedViewsInProcessDidTerminate API test that is
still passing even though the delegate call is no longer asynchronous.

• UIProcess/WebPageProxy.cpp:

(WebKit::WebPageProxy::commitProvisionalPage):
(WebKit::WebPageProxy::resetStateAfterProcessTermination):
(WebKit::WebPageProxy::dispatchProcessDidTerminate):

• UIProcess/WebPageProxy.h:
• UIProcess/WebProcessProxy.cpp:

(WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
(WebKit::WebProcessProxy::requestTermination):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/WebPageProxy.cpp (modified) (4 diffs)
• UIProcess/WebPageProxy.h (modified) (1 diff)
• UIProcess/WebProcessProxy.cpp (modified) (4 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-02-17  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r268097): Calling processDidTerminate delegate asynchronously is risky compatibility-wise
+        https://bugs.webkit.org/show_bug.cgi?id=222011
+
+        Reviewed by Carlos Garcia Campos.
+
+        Calling processDidTerminate delegate asynchronously like we did in r268097 is risky compatibility-wise.
+        This caused breakage in at least 2 client applications. While this can be dealt with on the client side,
+        it would be better to fix what r268097 was trying to fix without making the delegate call asynchronous.
+        The reason calling the delegate asynchronously is risky is because some view state may have time to get
+        reset by the time the client gets notified on the crash, potentially confusing the crash handling logic
+        in the client.
+
+        No new tests, covered by WKNavigation.ReloadRelatedViewsInProcessDidTerminate API test that is
+        still passing even though the delegate call is no longer asynchronous.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::commitProvisionalPage):
+        (WebKit::WebPageProxy::resetStateAfterProcessTermination):
+        (WebKit::WebPageProxy::dispatchProcessDidTerminate):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebProcessProxy.cpp:
+        (WebKit::WebProcessProxy::processDidTerminateOrFailedToLaunch):
+        (WebKit::WebProcessProxy::requestTermination):
+
 2021-02-17  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebKit/UIProcess/WebPageProxy.cpp

@@ -3344 +3344 @@
         send(Messages::WebPage::UnfreezeLayerTreeDueToSwipeAnimation());
 
-    processDidTerminate(ProcessTerminationReason::NavigationSwap);
+    resetStateAfterProcessTermination(ProcessTerminationReason::NavigationSwap);
 
     m_process->removeMessageReceiver(Messages::WebPageProxy::messageReceiverName(), m_webPageID);
@@ -7238 +7238 @@
 }
 
-void WebPageProxy::processDidTerminate(ProcessTerminationReason reason)
+void WebPageProxy::resetStateAfterProcessTermination(ProcessTerminationReason reason)
 {
     if (reason != ProcessTerminationReason::NavigationSwap)
@@ -7265 +7265 @@
     // If it does *during* process swapping, and the client triggers a reload, that causes bizarre WebKit re-entry.
     // FIXME: This might have to change
-    if (reason != ProcessTerminationReason::NavigationSwap) {
+    if (reason != ProcessTerminationReason::NavigationSwap)
         navigationState().clearAllNavigations();
-        dispatchProcessDidTerminate(reason);
-    }
 
     if (m_controlledByAutomation) {
@@ -7302 +7300 @@
     RELEASE_LOG_ERROR_IF_ALLOWED(Loading, "dispatchProcessDidTerminate: reason = %d", reason);
 
-    // We notify the client asynchronously because several pages may share the same process
-    // and we want to make sure all pages are aware their process has crashed before the
-    // the client reacts to the process termination.
-    RunLoop::main().dispatch([this, weakThis = makeWeakPtr(*this), reason] {
-        if (!weakThis)
-            return;
-
-        bool handledByClient = false;
-        if (m_loaderClient)
-            handledByClient = reason != ProcessTerminationReason::RequestedByClient && m_loaderClient->processDidCrash(*this);
-        else
-            handledByClient = m_navigationClient->processDidTerminate(*this, reason);
-
-        if (!handledByClient && shouldReloadAfterProcessTermination(reason))
-            tryReloadAfterProcessTermination();
-    });
+    bool handledByClient = false;
+    if (m_loaderClient)
+        handledByClient = reason != ProcessTerminationReason::RequestedByClient && m_loaderClient->processDidCrash(*this);
+    else
+        handledByClient = m_navigationClient->processDidTerminate(*this, reason);
+
+    if (!handledByClient && shouldReloadAfterProcessTermination(reason))
+        tryReloadAfterProcessTermination();
 }
 

trunk/Source/WebKit/UIProcess/WebPageProxy.h

@@ -1177 +1177 @@
     void processDidBecomeUnresponsive();
     void processDidBecomeResponsive();
-    void processDidTerminate(ProcessTerminationReason);
+    void resetStateAfterProcessTermination(ProcessTerminationReason);
     void provisionalProcessDidTerminate();
     void dispatchProcessDidTerminate(ProcessTerminationReason);

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -886 +886 @@
 
     for (auto& page : pages)
-        page->processDidTerminate(ProcessTerminationReason::Crash);
+        page->resetStateAfterProcessTermination(ProcessTerminationReason::Crash);
 
     for (auto& provisionalPage : provisionalPages) {
@@ -892 +892 @@
             provisionalPage->processDidTerminate();
     }
+
+    for (auto& page : pages)
+        page->dispatchProcessDidTerminate(ProcessTerminationReason::Crash);
 
     m_sleepDisablers.clear();
@@ -1255 +1258 @@
 
     for (auto& page : pages)
-        page->processDidTerminate(reason);
+        page->resetStateAfterProcessTermination(reason);
         
     for (auto& provisionalPage : provisionalPages) {
@@ -1261 +1264 @@
             provisionalPage->processDidTerminate();
     }
+
+    for (auto& page : pages)
+        page->dispatchProcessDidTerminate(reason);
 }