Deregister image client in LayoutObject::willBeDestroyed()

Deregister image client in LayoutObject::willBeDestroyed()

Before this CL, LayoutObjects deregister themself from the style images
until the last moment when the object is deleted. This causes problems
with LayoutParts because undetached LayoutPart is not deleted until all
references are deleted (namely, from FrameView), but they are technically
destroyed (with no associated Node object). This creates a small time frame
where a fetched image can invoke LayoutObject::imageChanged() on a detached
object.

This CL moves the deregistration code to LayoutObject::willBeDestroyed(),
and LayoutObject::postDestroy() becomes a one liner, thus removed.

BUG=500054
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197193

[trchen, 2015-06-16 03:54:21]

The CQ bit was checked by trchen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-06-16 03:54:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1174373003/1

[trchen, 2015-06-16 03:56:36]

trchen@chromium.org changed reviewers:
+ cevans@chromium.org, wangxianzhu@chromium.org

[trchen, 2015-06-16 04:05:55]

https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
File Source/core/layout/LayoutObject.cpp (left):

https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
Source/core/layout/LayoutObject.cpp:2554: // It seems ugly that this is not in
willBeDestroyed().
Hello cevans@, I noticed you added this comment in
https://codereview.chromium.org/20231002 in 2013. Do you still remember the
reason why this can't be done in LayoutObject::willBeDestroyed()?

I looked at the revision history of your CL. My best guess is that you were
trying to minimize behavioral difference?

Thanks in advance for your insights. :)

FYI, I made a recent change that removed the if(parent())return; check from
LayoutBox::imageChanged, and got this crash I described in CL comments. IMO
instead of adding back the superfluous check we should clean this up (if we
can).

[Chris Evans, 2015-06-16 04:35:19]

On 2015/06/16 04:05:55, trchen wrote:
>
https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
> File Source/core/layout/LayoutObject.cpp (left):
> 
>
https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
> Source/core/layout/LayoutObject.cpp:2554: // It seems ugly that this is not in
> willBeDestroyed().
> Hello cevans@, I noticed you added this comment in
> https://codereview.chromium.org/20231002 in 2013. Do you still remember the
> reason why this can't be done in LayoutObject::willBeDestroyed()?
> 
> I looked at the revision history of your CL. My best guess is that you were
> trying to minimize behavioral difference?
> 
> Thanks in advance for your insights. :)
> 
> FYI, I made a recent change that removed the if(parent())return; check from
> LayoutBox::imageChanged, and got this crash I described in CL comments. IMO
> instead of adding back the superfluous check we should clean this up (if we
> can).

Do not remember, sorry :-/
I expect the thoroughness of layouttests + asan + clusterfuzz will expose any
trouble, FWIW.

[Xianzhu, 2015-06-16 04:51:56]

On 2015/06/16 04:35:19, Chris Evans wrote:
> On 2015/06/16 04:05:55, trchen wrote:
> >
>
https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
> > File Source/core/layout/LayoutObject.cpp (left):
> > 
> >
>
https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
> > Source/core/layout/LayoutObject.cpp:2554: // It seems ugly that this is not
in
> > willBeDestroyed().
> > Hello cevans@, I noticed you added this comment in
> > https://codereview.chromium.org/20231002 in 2013. Do you still remember the
> > reason why this can't be done in LayoutObject::willBeDestroyed()?
> > 
> > I looked at the revision history of your CL. My best guess is that you were
> > trying to minimize behavioral difference?
> > 
> > Thanks in advance for your insights. :)
> > 
> > FYI, I made a recent change that removed the if(parent())return; check from
> > LayoutBox::imageChanged, and got this crash I described in CL comments. IMO
> > instead of adding back the superfluous check we should clean this up (if we
> > can).
> 
> Do not remember, sorry :-/
> I expect the thoroughness of layouttests + asan + clusterfuzz will expose any
> trouble, FWIW.

Perhaps we wanted to ensure that the code in postDestroy is the last step before
delete, when some child class overrides willBeDestroyed() but doesn't call
super-class's willBeDestroyed as the last statement?

[Xianzhu, 2015-06-16 05:00:28]

lgtm

[commit-bot: I haz the power, 2015-06-16 05:03:32]

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

[commit-bot: I haz the power, 2015-06-16 05:03:33]

Dry run: This issue passed the CQ dry run.

[trchen, 2015-06-16 20:48:36]

I think we can land and see if it sticks.

Let me know if you think a test is necessary. I find this bug very timing
sensitive to produce a reliable test.

[trchen, 2015-06-16 20:48:42]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2015-06-16 20:49:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1174373003/1

[commit-bot: I haz the power, 2015-06-16 20:54:21]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197193

[Noel Gordon, 2015-06-28 14:04:05]

On 2015/06/16 04:05:55, trchen wrote:
>
https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
> File Source/core/layout/LayoutObject.cpp (left):
> 
>
https://codereview.chromium.org/1174373003/diff/1/Source/core/layout/LayoutOb...
> Source/core/layout/LayoutObject.cpp:2554: // It seems ugly that this is not in
> willBeDestroyed().
> Hello cevans@, I noticed you added this comment in
> https://codereview.chromium.org/20231002 in 2013. Do you still remember the
> reason why this can't be done in LayoutObject::willBeDestroyed()?
> 
> I looked at the revision history of your CL. My best guess is that you were
> trying to minimize behavioral difference?
> 
> Thanks in advance for your insights. :)
> 
> FYI, I made a recent change that removed the if(parent())return; check from
> LayoutBox::imageChanged, and got this crash I described in CL comments. 

The change that removed the parent() check is not mentioned in the change
description.  Why was it removed?

Also how many other elements depend on / call LayoutBox::imageChanged(). 
Replaced elements can do this for sure.  Can they now be left in this
semi-deleted state too, and hence crash on imageChanged() in the same way?

[Noel Gordon, 2015-06-28 14:04:26]

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