Changeset 286609 in webkit

Timestamp:

Dec 7, 2021, 12:42:25 PM (5 years ago)

Author:

Chris Dumez

Message:

ASSERTION FAILED: m_messagesBeingDispatched.isEmpty() on http/tests/resourceLoadStatistics/website-data-removal-for-site-with-user-interaction.html
​https://bugs.webkit.org/show_bug.cgi?id=228164
<rdar://problem/80914914>

Reviewed by Darin Adler.

Source/WebKit:

dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection() has an assertion to make sure that m_messagesBeingDispatched
was empty when getting called. However, in the case where there is a nested run loop, the call to
m_messagesBeingDispatched.takeFirst().dispatch() at the end of the function may cause the function to re-enter. When entering,
m_messagesBeingDispatched may not be empty so the assertion is wrong. Looking at the code, I think dropping the assertion is the
right thing to do as the implementation seems to be doing something sane upon re-entering. It will just append new messages to
m_messagesBeingDispatched and then try and dispatch them. Based on how it is implemented, message ordering would be preserved.

No new tests, unskipped existing test.

• Platform/IPC/Connection.cpp:

(IPC::Connection::SyncMessageState::dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection):

LayoutTests:

Unskip test that should no longer be crashing.

• platform/ios-wk2/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/ios-wk2/TestExpectations (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/Platform/IPC/Connection.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-12-07  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILED: m_messagesBeingDispatched.isEmpty() on http/tests/resourceLoadStatistics/website-data-removal-for-site-with-user-interaction.html
+        https://bugs.webkit.org/show_bug.cgi?id=228164
+        <rdar://problem/80914914>
+
+        Reviewed by Darin Adler.
+
+        Unskip test that should no longer be crashing.
+
+        * platform/ios-wk2/TestExpectations:
+
 2021-12-07  Kyle Piddington  <kpiddington@apple.com>
 

trunk/LayoutTests/platform/ios-wk2/TestExpectations

@@ -2023 +2023 @@
 http/tests/resourceLoadStatistics/exemptDomains/ [ Pass ]
 
-webkit.org/b/228164 [ Debug ] http/tests/resourceLoadStatistics/website-data-removal-for-site-with-user-interaction.html [ Pass Crash ]
-
 webkit.org/b/216492 [ Debug ] imported/w3c/web-platform-tests/selection/extend-20.html [ Slow ]
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-12-07  Chris Dumez  <cdumez@apple.com>
+
+        ASSERTION FAILED: m_messagesBeingDispatched.isEmpty() on http/tests/resourceLoadStatistics/website-data-removal-for-site-with-user-interaction.html
+        https://bugs.webkit.org/show_bug.cgi?id=228164
+        <rdar://problem/80914914>
+
+        Reviewed by Darin Adler.
+
+        dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection() has an assertion to make sure that m_messagesBeingDispatched
+        was empty when getting called. However, in the case where there is a nested run loop, the call to
+        `m_messagesBeingDispatched.takeFirst().dispatch()` at the end of the function may cause the function to re-enter. When entering,
+        m_messagesBeingDispatched may not be empty so the assertion is wrong. Looking at the code, I think dropping the assertion is the
+        right thing to do as the implementation seems to be doing something sane upon re-entering. It will just append new messages to
+        m_messagesBeingDispatched and then try and dispatch them. Based on how it is implemented, message ordering would be preserved.
+
+        No new tests, unskipped existing test.
+
+        * Platform/IPC/Connection.cpp:
+        (IPC::Connection::SyncMessageState::dispatchMessagesAndResetDidScheduleDispatchMessagesForConnection):
+
 2021-12-07  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/Platform/IPC/Connection.cpp

@@ -227 +227 @@
         ASSERT(m_didScheduleDispatchMessagesWorkSet.contains(&connection));
         m_didScheduleDispatchMessagesWorkSet.remove(&connection);
-        ASSERT(m_messagesBeingDispatched.isEmpty());
         Deque<ConnectionAndIncomingMessage> messagesToPutBack;
         for (auto& connectionAndIncomingMessage : m_messagesToDispatchWhileWaitingForSyncReply) {
@@ -239 +238 @@
 
     while (!m_messagesBeingDispatched.isEmpty())
-        m_messagesBeingDispatched.takeFirst().dispatch();
+        m_messagesBeingDispatched.takeFirst().dispatch(); // This may cause the function to re-enter when there is a nested run loop.
 }