Changeset 222768 in webkit

Timestamp:

Oct 2, 2017, 10:10:29 PM (9 years ago)

Author:

Wenson Hsieh

Message:

REGRESSION(r222595): Intermittent crash while accessing DataTransferItemList
​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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/TestExpectations (modified) (1 diff)
• LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list-expected.txt (added)
• LayoutTests/editing/pasteboard/drag-end-crash-accessing-item-list.html (added)
• LayoutTests/platform/mac-wk1/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/DataTransfer.cpp (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/TestExpectations

@@ -71 +71 @@
 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

trunk/LayoutTests/platform/mac-wk1/TestExpectations

@@ -12 +12 @@
 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 ]
 
 #//////////////////////////////////////////////////////////////////////////////////////////

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/DataTransfer.cpp

@@ -52 +52 @@
     void startLoading(CachedResourceHandle<CachedImage>&);
     void stopLoading(CachedResourceHandle<CachedImage>&);
+    void moveToDataTransfer(DataTransfer&);
 
 private:
@@ -364 +365 @@
     : m_dataTransfer(dataTransfer)
 {
+}
+
+void DragImageLoader::moveToDataTransfer(DataTransfer& newDataTransfer)
+{
+    m_dataTransfer = &newDataTransfer;
 }
 
@@ -509 +515 @@
     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);
 }