REGRESSION(r222595): Intermittent crash while accessing DataTransferItemList
authorwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Oct 2017 05:10:29 +0000 (05:10 +0000)
committerwenson_hsieh@apple.com <wenson_hsieh@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 3 Oct 2017 05:10:29 +0000 (05:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=177791
<rdar://problem/34781456>

Reviewed by Ryosuke Niwa.

Source/WebCore:

DataTransfer::moveDragState() currently attempts to move the other DataTransfer's DataTransferItemList and
DragImageLoader as members of its own. This is incorrect, since both of these entities hold raw references of
some form to the other DataTransfer, yet they are held as unique_ptrs in the new DataTransfer. To fix this, we
(1) remove the line of code that moves the item list, since item lists will be lazily generated on the new
DataTransfer anyways, and (2) update the DataTransfer pointer on the old DataTransfer's DragImageLoader after
moving it to the new DataTransfer.

Test: editing/pasteboard/drag-end-crash-accessing-item-list.html

* dom/DataTransfer.cpp:
(WebCore::DragImageLoader::moveToDataTransfer):
(WebCore::DataTransfer::moveDragState):

LayoutTests:

Add a new layout test that simulates the crash encountered in this bug by forcing a garbage collection sweep
right before accessing the pasteboard in a "dragend" event handler.

* TestExpectations:
* editing/pasteboard/drag-end-crash-accessing-item-list-expected.txt: Added.
* editing/pasteboard/drag-end-crash-accessing-item-list.html: Added.
* platform/mac-wk1/TestExpectations:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@222768 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/TestExpectations
LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list-expected.txt [new file with mode: 0644]
LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list.html [new file with mode: 0644]
LayoutTests/platform/mac-wk1/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/DataTransfer.cpp

index 4c0962ad237826cedb8969859ed6b9cfb57bec3d..a9f6be0a3b9e605d918c92dd211dc7d993812343 100644 (file)
@@ -1,3 +1,19 @@
+2017-10-02  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION(r222595): Intermittent crash while accessing DataTransferItemList
+        https://bugs.webkit.org/show_bug.cgi?id=177791
+        <rdar://problem/34781456>
+
+        Reviewed by Ryosuke Niwa.
+
+        Add a new layout test that simulates the crash encountered in this bug by forcing a garbage collection sweep
+        right before accessing the pasteboard in a "dragend" event handler.
+
+        * TestExpectations:
+        * editing/pasteboard/drag-end-crash-accessing-item-list-expected.txt: Added.
+        * editing/pasteboard/drag-end-crash-accessing-item-list.html: Added.
+        * platform/mac-wk1/TestExpectations:
+
 2017-10-02  Brent Fulgham  <bfulgham@apple.com>
 
         Merge three Blink test cases
index 11adc35fd17c886dbbcb5bf990729252f1cc0d02..f6dff431683ca0e82e0b82bf679dc8a7a676b1e8 100644 (file)
@@ -70,6 +70,7 @@ editing/pasteboard/data-transfer-get-data-on-drop-custom.html [ Skip ]
 editing/pasteboard/data-transfer-get-data-on-drop-plain-text.html [ Skip ]
 editing/pasteboard/data-transfer-get-data-on-drop-rich-text.html [ Skip ]
 editing/pasteboard/data-transfer-get-data-on-drop-url.html [ Skip ]
+editing/pasteboard/drag-end-crash-accessing-item-list.html [ Skip ]
 
 # Only iOS supports QuickLook
 quicklook [ Skip ]
diff --git a/LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list-expected.txt b/LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list-expected.txt
new file mode 100644 (file)
index 0000000..0ebe4d8
--- /dev/null
@@ -0,0 +1 @@
+DRAG ME AND LET GO
diff --git a/LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list.html b/LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list.html
new file mode 100644 (file)
index 0000000..03ded30
--- /dev/null
@@ -0,0 +1,38 @@
+<!DOCTYPE html>
+<body>
+    <div draggable="true" style="width: 200px; height: 200px" id="source">DRAG ME AND LET GO</div>
+</body>
+<script>
+function forceGarbageCollection() {
+    if (window.GCController)
+        GCController.collect();
+    else {
+        for (let i = 0; i < 1000; i++)
+            new ArrayBuffer(0x100000);
+    }
+}
+
+source.addEventListener("dragend", event => {
+    forceGarbageCollection();
+    event.dataTransfer.items.clear();
+    if (window.testRunner)
+        testRunner.notifyDone();
+});
+
+source.addEventListener("dragstart", event => {
+    event.dataTransfer.items.add(event.target.id, "text/plain");
+    event.dataTransfer.items.add("Test1", "text/html");
+    event.dataTransfer.items.add("Test2", "text/uri-list");
+});
+
+if (window.testRunner && window.eventSender && window.internals) {
+    internals.settings.setCustomPasteboardDataEnabled(true);
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+    eventSender.mouseMoveTo(100, 100);
+    eventSender.mouseDown();
+    eventSender.leapForward(1000);
+    eventSender.mouseMoveTo(400, 400);
+    eventSender.mouseUp();
+}
+</script>
index 3c4b7b5f2cde7da262935d28227f9e58d116c59f..ec49fc48a4e9b0b2a0f4c3cb70e6fd19ae9decdd 100644 (file)
@@ -11,6 +11,7 @@ editing/pasteboard/data-transfer-get-data-on-drop-custom.html [ Pass ]
 editing/pasteboard/data-transfer-get-data-on-drop-plain-text.html [ Pass ]
 editing/pasteboard/data-transfer-get-data-on-drop-rich-text.html [ Pass ]
 editing/pasteboard/data-transfer-get-data-on-drop-url.html [ Pass ]
+editing/pasteboard/drag-end-crash-accessing-item-list.html [ Pass ]
 
 #//////////////////////////////////////////////////////////////////////////////////////////
 # End platform-specific directories.
index 0712d031b159bffe16a9158f1c0bd7a085356552..dfe0b1d632d61193b78e093acd8b45130af944a6 100644 (file)
@@ -1,3 +1,24 @@
+2017-10-02  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION(r222595): Intermittent crash while accessing DataTransferItemList
+        https://bugs.webkit.org/show_bug.cgi?id=177791
+        <rdar://problem/34781456>
+
+        Reviewed by Ryosuke Niwa.
+
+        DataTransfer::moveDragState() currently attempts to move the other DataTransfer's DataTransferItemList and
+        DragImageLoader as members of its own. This is incorrect, since both of these entities hold raw references of
+        some form to the other DataTransfer, yet they are held as unique_ptrs in the new DataTransfer. To fix this, we
+        (1) remove the line of code that moves the item list, since item lists will be lazily generated on the new
+        DataTransfer anyways, and (2) update the DataTransfer pointer on the old DataTransfer's DragImageLoader after
+        moving it to the new DataTransfer.
+
+        Test: editing/pasteboard/drag-end-crash-accessing-item-list.html
+
+        * dom/DataTransfer.cpp:
+        (WebCore::DragImageLoader::moveToDataTransfer):
+        (WebCore::DataTransfer::moveDragState):
+
 2017-10-02  Chris Dumez  <cdumez@apple.com>
 
         Rename computeSharedStringHash() overload taking a URL to computedVisitedLinkHash()
index 68801a74a8066f33d0479e3f3043925bba03ce1c..addef0f9173c456a81eae74905124f0c2f493fe2 100644 (file)
@@ -51,6 +51,7 @@ public:
     explicit DragImageLoader(DataTransfer*);
     void startLoading(CachedResourceHandle<CachedImage>&);
     void stopLoading(CachedResourceHandle<CachedImage>&);
+    void moveToDataTransfer(DataTransfer&);
 
 private:
     void imageChanged(CachedImage*, const IntRect*) override;
@@ -365,6 +366,11 @@ DragImageLoader::DragImageLoader(DataTransfer* dataTransfer)
 {
 }
 
+void DragImageLoader::moveToDataTransfer(DataTransfer& newDataTransfer)
+{
+    m_dataTransfer = &newDataTransfer;
+}
+
 void DragImageLoader::startLoading(CachedResourceHandle<WebCore::CachedImage>& image)
 {
     // FIXME: Does this really trigger a load? Does it need to?
@@ -508,7 +514,8 @@ void DataTransfer::moveDragState(Ref<DataTransfer>&& other)
     m_dragImage = other->m_dragImage;
     m_dragImageElement = WTFMove(other->m_dragImageElement);
     m_dragImageLoader = WTFMove(other->m_dragImageLoader);
-    m_itemList = WTFMove(other->m_itemList);
+    if (m_dragImageLoader)
+        m_dragImageLoader->moveToDataTransfer(*this);
     m_fileList = WTFMove(other->m_fileList);
 }