Avoid dereferencing layoutObject during FloatingObject::unsafeClone

Avoid dereferencing layoutObject during FloatingObject::unsafeClone

There are bugs in the code that detaches subtrees in which a floating object sometimes is not detached from its owning m_floatingObjects set yet is still
deleted.

BUG=619380
Committed: https://crrev.com/23aafd7fac7245071163eea43cc4dfeb6f302b9f
Cr-Commit-Position: refs/heads/master@{#401899}

[chrishtr, 2016-06-23 23:29:18]

Description was changed from

==========
none

BUG=
==========

to

==========
Avoid dereferencing layoutObject during FloatingObject::unsafeClone

There are bugs in the code that detaches subtrees in which a floating object
sometimes is not detached from its owning m_floatingObjects set yet is still
deleted. I was unable to understand why; this fixes the beta blocker in the
meantime.

BUG=619380
==========

[chrishtr, 2016-06-23 23:29:27]

Description was changed from

==========
Avoid dereferencing layoutObject during FloatingObject::unsafeClone

There are bugs in the code that detaches subtrees in which a floating object
sometimes is not detached from its owning m_floatingObjects set yet is still
deleted. I was unable to understand why; this fixes the beta blocker in the
meantime.

BUG=619380
==========

to

==========
Avoid dereferencing layoutObject during FloatingObject::unsafeClone

There are bugs in the code that detaches subtrees in which a floating object
sometimes is not detached from its owning m_floatingObjects set yet is still
deleted.

BUG=619380
==========

[chrishtr, 2016-06-23 23:35:05]

chrishtr@chromium.org changed reviewers:
+ wangxianzhu@chromium.org

[chrishtr, 2016-06-23 23:35:06]

I was unable to fix out the root cause in floats. The layout team will need to
investigate
further. This CL fixes the immediate issue.

[Xianzhu, 2016-06-24 00:20:12]

lgtm as a workaround.

https://codereview.chromium.org/2099523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/2099523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FloatingObjects.cpp:80: //
FIXME(chrishtr): Avoid the following hack when performing an unsafe clone.
Nit: s/FIXME/TODO/

[chrishtr, 2016-06-24 16:11:25]

https://codereview.chromium.org/2099523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/2099523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FloatingObjects.cpp:80: //
FIXME(chrishtr): Avoid the following hack when performing an unsafe clone.
On 2016/06/24 at 00:20:12, Xianzhu wrote:
> Nit: s/FIXME/TODO/

Done.

[chrishtr, 2016-06-24 16:11:32]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-06-24 16:11:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2099523002/#ps80001
(title: "none")

[commit-bot: I haz the power, 2016-06-24 16:12:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-24 17:47:05]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-06-24 17:51:07]

Description was changed from

==========
Avoid dereferencing layoutObject during FloatingObject::unsafeClone

There are bugs in the code that detaches subtrees in which a floating object
sometimes is not detached from its owning m_floatingObjects set yet is still
deleted.

BUG=619380
==========

to

==========
Avoid dereferencing layoutObject during FloatingObject::unsafeClone

There are bugs in the code that detaches subtrees in which a floating object
sometimes is not detached from its owning m_floatingObjects set yet is still
deleted.

BUG=619380

Committed: https://crrev.com/23aafd7fac7245071163eea43cc4dfeb6f302b9f
Cr-Commit-Position: refs/heads/master@{#401899}
==========

[commit-bot: I haz the power, 2016-06-24 17:51:10]

Patchset 5 (id:??) landed as
https://crrev.com/23aafd7fac7245071163eea43cc4dfeb6f302b9f
Cr-Commit-Position: refs/heads/master@{#401899}

[wkorman, 2016-06-24 18:10:34]

wkorman@chromium.org changed reviewers:
+ wkorman@chromium.org

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

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/float-reparent-during-detach-crash.html
(right):

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/float-reparent-during-detach-crash.html:2:
Passes if it does not crash.
Does this crash without the change, in a non-ASAN build? Or is this test just
for validating fix with ASAN? It doesn't seem to crash with current ToT or dev
build.

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FloatingObjects.cpp:124:
std::unique_ptr<FloatingObject> cloneObject = wrapUnique(new
FloatingObject(layoutObject(), getType(), m_frameRect, m_shouldPaint,
m_isDescendant, false, true));
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]

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/fast/block/float/float-reparent-during-detach-crash.html
(right):

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/block/float/float-reparent-during-detach-crash.html:2:
Passes if it does not crash.
On 2016/06/24 at 18:10:35, wkorman wrote:
> Does this crash without the change, in a non-ASAN build? Or is this test just
for validating fix with ASAN? It doesn't seem to crash with current ToT or dev
build.

Just crashes in ASAN. But there are ASAN bots that run on all layout tests.

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/FloatingObjects.cpp (right):

https://codereview.chromium.org/2099523002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/FloatingObjects.cpp:124:
std::unique_ptr<FloatingObject> cloneObject = wrapUnique(new
FloatingObject(layoutObject(), getType(), m_frameRect, m_shouldPaint,
m_isDescendant, false, true));
On 2016/06/24 at 18:10:35, wkorman wrote:
> It looked like the unsafe ref was on what would be 'this' in this method, such
that this could already be too late?

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).