Changeset 259613 in webkit

Timestamp:

Apr 6, 2020, 5:00:04 PM (6 years ago)

Author:

Wenson Hsieh

Message:

REGRESSION: 4 TestWebKitAPI.DragAndDropTests.DataTransferSetData tests failing on iOS
​https://bugs.webkit.org/show_bug.cgi?id=209685
<rdar://problem/60987461>

Reviewed by Megan Gardner.

After updating a WebKit open source test runner to iOS 13.4, 4 pasteboard-related API tests began to fail in
release builds on that particular bot. Logging statements added in r259465, r259518, r259534, and r259541
strongly suggest that this is due to an IPC dispatch race when clearing the platform pasteboard before writing
custom pasteboard data. On iOS, the former is dispatched asynchronously, while the latter is dispatched as sync
IPC. This means that if the UI process happens to be waiting for a sync IPC response from the web process, it
will end up handling the incoming IPC messages out of order by immediately dispatching sync IPC (in this case,
writing custom pasteboard data) before dispatching the async IPC (clearing data). This causes the custom
pasteboard data to be cleared on the platform pasteboard immediately after it is written.

To fix this, we limit clearing pasteboard data to when we would've otherwise avoided writing any custom
pasteboard data, and additionally make it so that writing custom pasteboard data always clears out any pre-
existing content on the pasteboard (obviating the need for a separate message to clear the pasteboard). Note
that writing custom pasteboard data always clears the existing pasteboard on macOS and iOS -- on macOS, we use
-declareTypes:owner:; on iOS, we use -setItemProviders:; in the case of macCatalyst, we -setItems:.

• dom/DataTransfer.cpp:

(WebCore::DataTransfer::commitToPasteboard):

Push the call to clear the pasteboard down from the call sites of commitToPasteboard into commitToPasteboard
itself; then, only explicitly clear the pasteboard in the case where we aren't writing custom pasteboard data
(i.e. either custom pasteboard data is disabled, or there is no data to write),

(WebCore::DataTransfer::moveDragState): See above.

• editing/Editor.cpp:

(WebCore::dispatchClipboardEvent): See above.

• platform/ios/WebItemProviderPasteboard.mm:

(-[WebItemProviderPasteboard stageRegistrationLists:]):

Remove always-on logging added in r259541 to help diagnose the test failures.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/DataTransfer.cpp (modified) (3 diffs)
• editing/Editor.cpp (modified) (1 diff)
• platform/ios/WebItemProviderPasteboard.mm (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-04-06  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION: 4 TestWebKitAPI.DragAndDropTests.DataTransferSetData tests failing on iOS
+        https://bugs.webkit.org/show_bug.cgi?id=209685
+        <rdar://problem/60987461>
+
+        Reviewed by Megan Gardner.
+
+        After updating a WebKit open source test runner to iOS 13.4, 4 pasteboard-related API tests began to fail in
+        release builds on that particular bot. Logging statements added in r259465, r259518, r259534, and r259541
+        strongly suggest that this is due to an IPC dispatch race when clearing the platform pasteboard before writing
+        custom pasteboard data. On iOS, the former is dispatched asynchronously, while the latter is dispatched as sync
+        IPC. This means that if the UI process happens to be waiting for a sync IPC response from the web process, it
+        will end up handling the incoming IPC messages out of order by immediately dispatching sync IPC (in this case,
+        writing custom pasteboard data) before dispatching the async IPC (clearing data). This causes the custom
+        pasteboard data to be cleared on the platform pasteboard immediately after it is written.
+
+        To fix this, we limit clearing pasteboard data to when we would've otherwise avoided writing any custom
+        pasteboard data, and additionally make it so that writing custom pasteboard data always clears out any pre-
+        existing content on the pasteboard (obviating the need for a separate message to clear the pasteboard). Note
+        that writing custom pasteboard data always clears the existing pasteboard on macOS and iOS -- on macOS, we use
+        `-declareTypes:owner:`; on iOS, we use `-setItemProviders:`; in the case of macCatalyst, we `-setItems:`.
+
+        * dom/DataTransfer.cpp:
+        (WebCore::DataTransfer::commitToPasteboard):
+
+        Push the call to clear the pasteboard down from the call sites of `commitToPasteboard` into `commitToPasteboard`
+        itself; then, only explicitly clear the pasteboard in the case where we aren't writing custom pasteboard data
+        (i.e. either custom pasteboard data is disabled, or there is no data to write),
+
+        (WebCore::DataTransfer::moveDragState): See above.
+        * editing/Editor.cpp:
+        (WebCore::dispatchClipboardEvent): See above.
+        * platform/ios/WebItemProviderPasteboard.mm:
+        (-[WebItemProviderPasteboard stageRegistrationLists:]):
+
+        Remove always-on logging added in r259541 to help diagnose the test failures.
+
 2020-04-06  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/dom/DataTransfer.cpp

@@ -426 +426 @@
     ASSERT(is<StaticPasteboard>(*m_pasteboard) && !is<StaticPasteboard>(nativePasteboard));
     PasteboardCustomData customData = downcast<StaticPasteboard>(*m_pasteboard).takeCustomData();
-    if (!customData.hasData())
-        return;
+    if (!customData.hasData()) {
+        // We clear the platform pasteboard here to ensure that the pasteboard doesn't contain any data
+        // that may have been written before starting the drag or copying, and after ending the last
+        // drag session or paste. After pushing the static pasteboard's contents to the platform, the
+        // pasteboard should only contain data that was in the static pasteboard.
+        nativePasteboard.clear();
+        return;
+    }
 
     if (RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled()) {
@@ -435 +441 @@
     }
 
+    nativePasteboard.clear();
     customData.forEachPlatformString([&] (auto& type, auto& string) {
         nativePasteboard.writeString(type, string);
@@ -703 +710 @@
 {
     RELEASE_ASSERT(is<StaticPasteboard>(other->pasteboard()));
-    // We clear the platform pasteboard here to ensure that the pasteboard doesn't contain any data
-    // that may have been written before starting the drag, and after ending the last drag session.
-    // After pushing the static pasteboard's contents to the platform, the pasteboard should only
-    // contain data that was in the static pasteboard.
-    m_pasteboard->clear();
     other->commitToPasteboard(*m_pasteboard);
 

trunk/Source/WebCore/editing/Editor.cpp

@@ -428 +428 @@
     target->dispatchEvent(event);
     bool noDefaultProcessing = event->defaultPrevented();
-    if (noDefaultProcessing && (kind == ClipboardEventKind::Copy || kind == ClipboardEventKind::Cut)) {
-        auto pasteboard = Pasteboard::createForCopyAndPaste();
-        pasteboard->clear();
-        dataTransfer->commitToPasteboard(*pasteboard);
-    }
+    if (noDefaultProcessing && (kind == ClipboardEventKind::Copy || kind == ClipboardEventKind::Cut))
+        dataTransfer->commitToPasteboard(*Pasteboard::createForCopyAndPaste());
 
     dataTransfer->makeInvalidForSecurity();

trunk/Source/WebCore/platform/ios/WebItemProviderPasteboard.mm

@@ -857 +857 @@
 {
     ASSERT(lists.count);
-    NSLog(@"%s - %@", __PRETTY_FUNCTION__, lists);
-    WTFReportBacktrace();
     _stagedRegistrationInfoLists = lists;
 }