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: http://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 4c0962a..a9f6be0 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 11adc35..f6dff43 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 3c4b7b5..ec49fc4 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 0712d03..dfe0b1d 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 68801a7..addef0f 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);
 }