Don't attempt to collapse empty anonymous blocks if they might already have been destroyed

Don't attempt to collapse empty anonymous blocks if they might already have been destroyed

LayoutBlock::addChild() can destroy its own object and that of any out-of-flow
or floating siblings - for that reason it's not safe to access the object or any
of its siblings after calling it. So when checking for an empty anonymous block
here, do so after removing its child and before we add it to its new home.

The check was introduced in wkbug.com/78250 and a follow-up wkbug.com/79681
so we have test coverage from those two CLs as well.

BUG=598722
Committed: https://crrev.com/6b42be50e95deed9f709348eec4d8b05cc552d17
Cr-Commit-Position: refs/heads/master@{#385009}

[rhogan, 2016-03-30 19:40:43]

Description was changed from

==========
LayoutBlock collapses away empty anonymous blocks, so don't look for them in
LayoutListItem

BUG=598722
==========

to

==========
LayoutBlock collapses away empty anonymous blocks, so don't look for them in
LayoutListItem

This check is no longer necessary, and attempts to access a LayoutBlock that
is beingDestroyed(), now that we collapse away unnecessary anonymous blocks
in mergeContiguousAnonymousBlocks.

The check was introduced in wkbug.com/78250 and a follow-up wkbug.com/79681
so we have test coverage from those two CLs as well.

BUG=598722
==========

[rhogan, 2016-03-30 19:40:43]

robhogan@gmail.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2016-03-31 10:29:40]

I was about to say the magic word, but it looks like this change introduces a
regression.

[rhogan, 2016-03-31 20:07:28]

On 2016/03/31 at 10:29:40, mstensho wrote:
> I was about to say the magic word, but it looks like this change introduces a
regression.

There's a pre-existing bug here, created by
https://chromium.googlesource.com/chromium/src/+/4eb44f72c373bc568021e44ba1ab....

The list marker should be before the hr element so the first anonymous block
should never be empty like that. I'm going to look at this separately in:
https://crbug.com/599616

[rhogan, 2016-04-03 11:07:45]

On 2016/03/31 at 20:07:28, rhogan wrote:
> On 2016/03/31 at 10:29:40, mstensho wrote:
> > I was about to say the magic word, but it looks like this change introduces
a regression.
> 
> There's a pre-existing bug here, created by
https://chromium.googlesource.com/chromium/src/+/4eb44f72c373bc568021e44ba1ab....
> 
> The list marker should be before the hr element so the first anonymous block
should never be empty like that. I'm going to look at this separately in:
https://crbug.com/599616

The root of that bug is in the hack chosen to keep a list item open for editing
- the editing command adds a temporary BR element while editing is in progress
and then removes it. This creates the situation where an empty anonymous block
can happen. So I'm going to keep the anonymous-block removal since it seems
semi-legitimate and I bet it can be made to happen in non-hacky scenarios too.

I thought there might be a solution to the object getting nuked while we still
have a pointer to it but I notice WeakPtr isn't used at all in layout/ and I'm
guessing there's a performance reason for that? Will Oilpan solve this sort of
thing in future? addChild() is such a dangerous footgun that it seems like there
must be a way of alerting callers to the fact that it can make the parent and
even some of its siblings go away, so be wary of keeping pointers to them.

[mstensho (USE GERRIT), 2016-04-04 09:34:52]

Looks like you need to update the description now, since we actually DO look for
anonymous blocks again.

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

https://codereview.chromium.org/1843213002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutListItem.cpp:316: // If markerParent
is now an anonymous block with no children, this is the time to attemp to remove
it
"attempt"

[rhogan, 2016-04-04 17:52:26]

Description was changed from

==========
LayoutBlock collapses away empty anonymous blocks, so don't look for them in
LayoutListItem

This check is no longer necessary, and attempts to access a LayoutBlock that
is beingDestroyed(), now that we collapse away unnecessary anonymous blocks
in mergeContiguousAnonymousBlocks.

The check was introduced in wkbug.com/78250 and a follow-up wkbug.com/79681
so we have test coverage from those two CLs as well.

BUG=598722
==========

to

==========
Don't attempt to collapse empty anonymous blocks if they might already have been
destroyed

LayoutBlock::addChild() can destroy its own object and that of any out-of-flow
or floating siblings - for that reason it's not safe to access the object or any
of its siblings after calling it. So when checking for an empty anonymous block
here, do so after removing its child and before we add it to its new home.

The check was introduced in wkbug.com/78250 and a follow-up wkbug.com/79681
so we have test coverage from those two CLs as well.

BUG=598722
==========

[rhogan, 2016-04-04 18:19:01]

On 2016/04/04 at 09:34:52, mstensho wrote:
> Looks like you need to update the description now, since we actually DO look
for anonymous blocks again.
> 
>
https://codereview.chromium.org/1843213002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/layout/LayoutListItem.cpp (right):
> 
>
https://codereview.chromium.org/1843213002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutListItem.cpp:316: // If
markerParent is now an anonymous block with no children, this is the time to
attemp to remove it
> "attempt"

Done!

[mstensho (USE GERRIT), 2016-04-04 18:24:08]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-04-04 18:24:08]

lgtm

[commit-bot: I haz the power, 2016-04-04 18:24:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 18:24:58]

This CL has an open dependency (Issue 1602773005 Patch 220001). Please resolve
the dependency and try again.

[rhogan, 2016-04-04 18:25:03]

The CQ bit was checked by robhogan@gmail.com

[commit-bot: I haz the power, 2016-04-04 18:25:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-04 18:25:24]

This CL has an open dependency (Issue 1602773005 Patch 220001). Please resolve
the dependency and try again.

[rhogan, 2016-04-04 18:32:15]

The CQ bit was checked by robhogan@gmail.com

[rhogan, 2016-04-04 18:32:16]

The patchset sent to the CQ was uploaded after l-g-t-m from mstensho@opera.com
Link to the patchset: https://codereview.chromium.org/1843213002/#ps100001
(title: "Updated")

[commit-bot: I haz the power, 2016-04-04 18:32:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843213002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843213002/100001

[commit-bot: I haz the power, 2016-04-04 21:18:28]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-04 21:20:37]

Description was changed from

==========
Don't attempt to collapse empty anonymous blocks if they might already have been
destroyed

LayoutBlock::addChild() can destroy its own object and that of any out-of-flow
or floating siblings - for that reason it's not safe to access the object or any
of its siblings after calling it. So when checking for an empty anonymous block
here, do so after removing its child and before we add it to its new home.

The check was introduced in wkbug.com/78250 and a follow-up wkbug.com/79681
so we have test coverage from those two CLs as well.

BUG=598722
==========

to

==========
Don't attempt to collapse empty anonymous blocks if they might already have been
destroyed

LayoutBlock::addChild() can destroy its own object and that of any out-of-flow
or floating siblings - for that reason it's not safe to access the object or any
of its siblings after calling it. So when checking for an empty anonymous block
here, do so after removing its child and before we add it to its new home.

The check was introduced in wkbug.com/78250 and a follow-up wkbug.com/79681
so we have test coverage from those two CLs as well.

BUG=598722

Committed: https://crrev.com/6b42be50e95deed9f709348eec4d8b05cc552d17
Cr-Commit-Position: refs/heads/master@{#385009}
==========

[commit-bot: I haz the power, 2016-04-04 21:20:39]

Patchset 6 (id:??) landed as
https://crrev.com/6b42be50e95deed9f709348eec4d8b05cc552d17
Cr-Commit-Position: refs/heads/master@{#385009}