Clean up the fix of DevTools crashing in AsyncCallTracker.

Clean up the fix of DevTools crashing in AsyncCallTracker.

BUG=441223
R=haraken, yurys

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

[haraken, 2015-01-13 15:17:48]

https://codereview.chromium.org/821643004/diff/1/Source/core/inspector/AsyncC...
File Source/core/inspector/AsyncCallTracker.cpp (right):

https://codereview.chromium.org/821643004/diff/1/Source/core/inspector/AsyncC...
Source/core/inspector/AsyncCallTracker.cpp:78: virtual void contextDestroyed()
override

Who removes the ExecutionContextData from the observee (i.e., ExecutionContext)?
Or are you intentionally leaving the ExecutionContextData on the observee,
assuming that the ExecutionContext will be destructed soon? (That will work but
sound a bit hacky.)

[haraken, 2015-01-13 15:18:13]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[aandrey, 2015-01-13 15:39:25]

https://codereview.chromium.org/821643004/diff/1/Source/core/inspector/AsyncC...
File Source/core/inspector/AsyncCallTracker.cpp (right):

https://codereview.chromium.org/821643004/diff/1/Source/core/inspector/AsyncC...
Source/core/inspector/AsyncCallTracker.cpp:78: virtual void contextDestroyed()
override
On 2015/01/13 15:17:48, haraken wrote:
> 
> Who removes the ExecutionContextData from the observee (i.e.,
ExecutionContext)?
> Or are you intentionally leaving the ExecutionContextData on the observee,
> assuming that the ExecutionContext will be destructed soon? (That will work
but
> sound a bit hacky.)

We could not unobserve from contextDestroyed() callback until your recent patch.
In this patch I assume this invariant is still valid.

The new unobserve() calls dispose() from the parent class which actually removes
the ExecutionContextData from the observee.

[haraken, 2015-01-13 16:02:21]

On 2015/01/13 15:39:25, aandrey wrote:
>
https://codereview.chromium.org/821643004/diff/1/Source/core/inspector/AsyncC...
> File Source/core/inspector/AsyncCallTracker.cpp (right):
> 
>
https://codereview.chromium.org/821643004/diff/1/Source/core/inspector/AsyncC...
> Source/core/inspector/AsyncCallTracker.cpp:78: virtual void contextDestroyed()
> override
> On 2015/01/13 15:17:48, haraken wrote:
> > 
> > Who removes the ExecutionContextData from the observee (i.e.,
> ExecutionContext)?
> > Or are you intentionally leaving the ExecutionContextData on the observee,
> > assuming that the ExecutionContext will be destructed soon? (That will work
> but
> > sound a bit hacky.)
> 
> We could not unobserve from contextDestroyed() callback until your recent
patch.
> In this patch I assume this invariant is still valid.
> 
> The new unobserve() calls dispose() from the parent class which actually
removes
> the ExecutionContextData from the observee.

Yeah, this CL is solving two issues:

a) Remove the need to unobserve from contextDestroyed.
b) Remove the need to check if(self) in contextDestroyed.

clearLifecycleContext() solves a), and other parts solve b). I have no doubt for
b) but am neutral about a) (i.e., I'm not sure which is better, leaving dead
observers in the observee or allow unobserve from contextDestroyed).

LGTM.

[aandrey, 2015-01-13 16:06:54]

The CQ bit was checked by aandrey@chromium.org

[commit-bot: I haz the power, 2015-01-13 16:07:33]

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

[commit-bot: I haz the power, 2015-01-13 18:34:21]

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