Changeset 269806 in webkit

Timestamp:

Nov 13, 2020 4:20:47 PM (3 years ago)

Author:

BJ Burg

Message:

REGRESSION(r269701): inspector/console/webcore-logging.html is crashing
​https://bugs.webkit.org/show_bug.cgi?id=218840
<rdar://problem/71310952>

Reviewed by Devin Rousso.

This test is triggering a bizarre backtrace that fails the assertion ASSERT(m_frontendLoaded)
in the frontend dispatcher. The sequence of events present in the stack trace is as follows:

1. Test.html finishes loading.
2. DOMContentLoaded event listener eventually calls InspectorFrontendHost::loaded().
3. InspectorFrontendClient::frontendLoaded() calls -[NSWindow showWindow:] on the inspector window, which makes the inspected webpage window unfocus and resign first responder.
4. Unfocusing causes an activity state change and style recalc. Script is unsafe to execute under this.
5. Synchronously under the style recalc, inspector instrumentation is triggered when layers change.
6. The backend sends a protocol event, which is synchronously dispatched to the frontend.
7. Due to script evaluation being unsafe in (4), the frontend dispatcher tries to suspend. This hits the assertion because the frontend dispatcher hasn't been notified that the frontend finished loading as part of (3).

This only affects WebKitLegacy clients, where events 1-7 happen synchronously from top to bottom
due to the single process architecture. In the multiprocess case, frontendLoaded is an async
IPC message and frontend evaluations are not affected by the state of the inspected page.

• inspector/InspectorFrontendAPIDispatcher.cpp:

(WebCore::InspectorFrontendAPIDispatcher::frontendLoaded): Dispatch messages iff not suspended.

(WebCore::InspectorFrontendAPIDispatcher::suspend): Remove the assertion that is incorrect.
Allow clients of this class to call suspend() before frontendLoaded().

(WebCore::InspectorFrontendAPIDispatcher::unsuspend): Using a similar argument as above, it
is possible that WebKitLegacy may unsuspend the dispatcher before the frontend has fully loaded.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• inspector/InspectorFrontendAPIDispatcher.cpp (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-11-13  Brian Burg  <bburg@apple.com>
+
+        REGRESSION(r269701): inspector/console/webcore-logging.html is crashing
+        https://bugs.webkit.org/show_bug.cgi?id=218840
+        <rdar://problem/71310952>
+
+        Reviewed by Devin Rousso.
+
+        This test is triggering a bizarre backtrace that fails the assertion ASSERT(m_frontendLoaded)
+        in the frontend dispatcher. The sequence of events present in the stack trace is as follows:
+
+        1. Test.html finishes loading.
+        2. DOMContentLoaded event listener eventually calls InspectorFrontendHost::loaded().
+        3. InspectorFrontendClient::frontendLoaded() calls -[NSWindow showWindow:] on the inspector window,
+           which makes the inspected webpage window unfocus and resign first responder.
+        4. Unfocusing causes an activity state change and style recalc. Script is unsafe to execute under this.
+        5. Synchronously under the style recalc, inspector instrumentation is triggered when layers change.
+        6. The backend sends a protocol event, which is synchronously dispatched to the frontend.
+        7. Due to script evaluation being unsafe in (4), the frontend dispatcher tries to suspend.
+           This hits the assertion because the frontend dispatcher hasn't been notified that the frontend
+           finished loading as part of (3).
+
+        This only affects WebKitLegacy clients, where events 1-7 happen synchronously from top to bottom
+        due to the single process architecture. In the multiprocess case, frontendLoaded is an async
+        IPC message and frontend evaluations are not affected by the state of the inspected page.
+
+        * inspector/InspectorFrontendAPIDispatcher.cpp:
+        (WebCore::InspectorFrontendAPIDispatcher::frontendLoaded): Dispatch messages iff not suspended.
+
+        (WebCore::InspectorFrontendAPIDispatcher::suspend): Remove the assertion that is incorrect.
+        Allow clients of this class to call suspend() before frontendLoaded().
+
+        (WebCore::InspectorFrontendAPIDispatcher::unsuspend): Using a similar argument as above, it
+        is possible that WebKitLegacy may unsuspend the dispatcher before the frontend has fully loaded.
+
 2020-11-13  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp

@@ -66 +66 @@
     m_frontendLoaded = true;
 
-    evaluateQueuedExpressions();
+    // In some convoluted WebKitLegacy-only scenarios, the backend may try to dispatch events to the frontend
+    // underneath InspectorFrontendHost::loaded() when it is unsafe to execute script, causing suspend() to
+    // be called before the frontend has fully loaded. See <https://bugs.webkit.org/show_bug.cgi?id=218840>.
+    if (!m_suspended)
+        evaluateQueuedExpressions();
 }
 
 void InspectorFrontendAPIDispatcher::suspend(UnsuspendSoon unsuspendSoon)
 {
-    ASSERT(m_frontendLoaded);
-
     if (m_suspended)
         return;
@@ -96 +98 @@
     m_suspended = false;
 
-    evaluateQueuedExpressions();
+    if (m_frontendLoaded)
+        evaluateQueuedExpressions();
 }