Title: [269806] trunk/Source/WebCore
- Revision
- 269806
- Author
- [email protected]
- Date
- 2020-11-13 16:20:47 -0800 (Fri, 13 Nov 2020)
Log 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.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (269805 => 269806)
--- trunk/Source/WebCore/ChangeLog 2020-11-14 00:02:18 UTC (rev 269805)
+++ trunk/Source/WebCore/ChangeLog 2020-11-14 00:20:47 UTC (rev 269806)
@@ -1,3 +1,38 @@
+2020-11-13 Brian Burg <[email protected]>
+
+ 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 <[email protected]>
Move some more WebKit and WebKitLegacy preferences bound to Settings to WebPreferences.yaml
Modified: trunk/Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp (269805 => 269806)
--- trunk/Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp 2020-11-14 00:02:18 UTC (rev 269805)
+++ trunk/Source/WebCore/inspector/InspectorFrontendAPIDispatcher.cpp 2020-11-14 00:20:47 UTC (rev 269806)
@@ -65,13 +65,15 @@
ASSERT(m_frontendPage);
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;
@@ -95,7 +97,8 @@
m_suspended = false;
- evaluateQueuedExpressions();
+ if (m_frontendLoaded)
+ evaluateQueuedExpressions();
}
JSC::JSGlobalObject* InspectorFrontendAPIDispatcher::frontendGlobalObject()
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
