Avoid dereferencing layoutObject during FloatingObject::unsafeClone

third_party/WebKit/Source/core/layout/FloatingObjects.cpp

Index: third_party/WebKit/Source/core/layout/FloatingObjects.cpp
diff --git a/third_party/WebKit/Source/core/layout/FloatingObjects.cpp b/third_party/WebKit/Source/core/layout/FloatingObjects.cpp
index 3972c9d69f9998cf7847d98fbe07c521be9995a0..2dcf0d9e167aa0b28a6fd1f5f92c5f644361d5d8 100644
--- a/third_party/WebKit/Source/core/layout/FloatingObjects.cpp
+++ b/third_party/WebKit/Source/core/layout/FloatingObjects.cpp
@@ -64,7 +64,7 @@ FloatingObject::FloatingObject(LayoutBox* layoutObject)
         m_type = FloatRight;
 }
 
-FloatingObject::FloatingObject(LayoutBox* layoutObject, Type type, const LayoutRect& frameRect, bool shouldPaint, bool isDescendant, bool isLowestNonOverhangingFloatInChild)
+FloatingObject::FloatingObject(LayoutBox* layoutObject, Type type, const LayoutRect& frameRect, bool shouldPaint, bool isDescendant, bool isLowestNonOverhangingFloatInChild, bool performingUnsafeClone)
     : m_layoutObject(layoutObject)
     , m_originatingLine(nullptr)
     , m_frameRect(frameRect)
@@ -76,7 +76,15 @@ FloatingObject::FloatingObject(LayoutBox* layoutObject, Type type, const LayoutR
     , m_isInPlacedTree(false)
 #endif
 {
-    m_shouldPaint = shouldPaint || shouldPaintForCompositedLayoutPart();
+    m_shouldPaint = shouldPaint;
+    // TODO(chrishtr): Avoid the following hack when performing an unsafe clone.
+    // This avoids a use-after-free bug due to the fact that we sometimes fail to remove
+    // floats from their container when detaching (crbug.com/619380). This is actually a bug in the
+    // floats detach machinery, which needs to be fixed, in which case this workaround can be removed.
+    // In any case, it should be safe because moving floats from one owner to another should cause layout,
+    // which will in turn update the m_shouldPaint property.
+    if (!performingUnsafeClone)
+        m_shouldPaint = m_shouldPaint || shouldPaintForCompositedLayoutPart();
 }
 
 bool FloatingObject::shouldPaintForCompositedLayoutPart()
@@ -113,7 +121,7 @@ std::unique_ptr<FloatingObject> FloatingObject::copyToNewContainer(LayoutSize of
 
 std::unique_ptr<FloatingObject> FloatingObject::unsafeClone() const
 {
-    std::unique_ptr<FloatingObject> cloneObject = wrapUnique(new FloatingObject(layoutObject(), getType(), m_frameRect, m_shouldPaint, m_isDescendant, false));
+    std::unique_ptr<FloatingObject> cloneObject = wrapUnique(new FloatingObject(layoutObject(), getType(), m_frameRect, m_shouldPaint, m_isDescendant, false, true));

[wkorman, 2016/06/24 18:10:35]

It looked like the unsafe ref was on what would be 'this' in this method, such
that this could already be too late?

[chrishtr, 2016/07/01 15:43:39]

The unsafe ref was on the layoutObject() parameter to the FloatingObject
constructor, since the layout
object was already deleted during subtree detach (caused by the editing command
in the test).

     cloneObject->m_isPlaced = m_isPlaced;
     return cloneObject;
 }