Oilpan: Notify supplements of Page and LocalFrame destruction.

Oilpan: Notify LocalFrame supplements of destruction.

This fixes the flaky crash in screen_orientation/page-visibility.html

The willBeDestroyed callback of GeolocationController has not been called in either build since r175404.

In addition LocalFrame supplements receive an explicit callback when the LocalHost is torn down in order to perform coordinated cleanup. This callback can be removed again once LocalFrame is moved to the heap and it again dies together with its supplements.

R=ager@chromium.org, tkent@chromium.org
BUG=

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

[Mads Ager (chromium), 2014-06-12 11:55:33]

LGTM

We should continue the investigation of what it will take to get LocalFrame on
the heap.

[Michael van Ouwerkerk, 2014-06-12 12:32:35]

Just a drive-by nit.

https://codereview.chromium.org/335573004/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/335573004/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationController.h:75: virtual void
persistentHostHasBeenDestroyed() OVERRIDE;
Nit: please note where these are inherited from.

[tkent, 2014-06-12 12:53:13]

lgtm

[haraken, 2014-06-12 13:16:16]

The willBeDestroyed() change looks reasonable, but the
persistentHostHasBeenDestroyed() change looks hacky. Would it be possible to
call observeContext(0) in willBeDestroyed(), and avoid introducing
persistentHostHasBeenDestroyed()?

However, I'm getting confused when willBeDestroyed() and
persistentHostHasBeenDestroyed() should be called. For example:

  class A : public HeapSupplement<LocalFrame>, HeapSupplement<A>,
HeapSupplement<B> {
    void willBeDestroyed();  // When is this expected to be called?
    void persistentHostHasBeenDestroyed();  // When is this expected to be
called?
  };

If the class A inherits from multiple supplementable objects, it becomes
impossible to achieve "the supplement object dies with the supplementable
object". I'm curious how we are going to handle the situation.

[zerny-chromium, 2014-06-12 13:39:16]

On 2014/06/12 13:16:16, haraken wrote:
> The willBeDestroyed() change looks reasonable, but the
> persistentHostHasBeenDestroyed() change looks hacky. Would it be possible to
> call observeContext(0) in willBeDestroyed(), and avoid introducing
> persistentHostHasBeenDestroyed()?
> 
> However, I'm getting confused when willBeDestroyed() and
> persistentHostHasBeenDestroyed() should be called. For example:
> 
>   class A : public HeapSupplement<LocalFrame>, HeapSupplement<A>,
> HeapSupplement<B> {
>     void willBeDestroyed();  // When is this expected to be called?
>     void persistentHostHasBeenDestroyed();  // When is this expected to be
> called?
>   };
> 
> If the class A inherits from multiple supplementable objects, it becomes
> impossible to achieve "the supplement object dies with the supplementable
> object". I'm curious how we are going to handle the situation.

I understand your concerns, both methods are hacks. I've just moved the call to
willBeDestroyed to WebLocalFrameImpl::close since that objects is the owner of
the GeolocationClientProxy object referred to by GeolocationController.

Both of these methods should be removed asap. As you say, we can't really say
when the "host" is destructed in all cases because a supplement can be attached
multiple places. So really the callback does not make sense. However, for safety
we need it right now.

Once LocalFrame is on-heap we can remove
Supplement::persistentHostHasBeenDestroyed.
Once GeolocationClient is on-heap we can remove Supplement::willBeDestroyed.

For now we need both, because the first is always called just as the frame is
about to die and the latter is called once the frame resources are going down
:-/

[zerny-chromium, 2014-06-12 13:39:25]

https://codereview.chromium.org/335573004/diff/1/Source/modules/geolocation/G...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/335573004/diff/1/Source/modules/geolocation/G...
Source/modules/geolocation/GeolocationController.h:75: virtual void
persistentHostHasBeenDestroyed() OVERRIDE;
On 2014/06/12 12:32:34, Michael van Ouwerkerk wrote:
> Nit: please note where these are inherited from.

Done.

[haraken, 2014-06-12 13:47:49]

On 2014/06/12 13:39:16, zerny-chromium wrote:
> On 2014/06/12 13:16:16, haraken wrote:
> > The willBeDestroyed() change looks reasonable, but the
> > persistentHostHasBeenDestroyed() change looks hacky. Would it be possible to
> > call observeContext(0) in willBeDestroyed(), and avoid introducing
> > persistentHostHasBeenDestroyed()?
> > 
> > However, I'm getting confused when willBeDestroyed() and
> > persistentHostHasBeenDestroyed() should be called. For example:
> > 
> >   class A : public HeapSupplement<LocalFrame>, HeapSupplement<A>,
> > HeapSupplement<B> {
> >     void willBeDestroyed();  // When is this expected to be called?
> >     void persistentHostHasBeenDestroyed();  // When is this expected to be
> > called?
> >   };
> > 
> > If the class A inherits from multiple supplementable objects, it becomes
> > impossible to achieve "the supplement object dies with the supplementable
> > object". I'm curious how we are going to handle the situation.
> 
> I understand your concerns, both methods are hacks. I've just moved the call
to
> willBeDestroyed to WebLocalFrameImpl::close since that objects is the owner of
> the GeolocationClientProxy object referred to by GeolocationController.
> 
> Both of these methods should be removed asap. As you say, we can't really say
> when the "host" is destructed in all cases because a supplement can be
attached
> multiple places. So really the callback does not make sense. However, for
safety
> we need it right now.
> 
> Once LocalFrame is on-heap we can remove
> Supplement::persistentHostHasBeenDestroyed.
> Once GeolocationClient is on-heap we can remove Supplement::willBeDestroyed.
> 
> For now we need both, because the first is always called just as the frame is
> about to die and the latter is called once the frame resources are going down
> :-/

Thanks, makes sense. LGTM.

[zerny-chromium, 2014-06-12 14:02:25]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-06-12 14:02:59]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/335573004/20001

[commit-bot: I haz the power, 2014-06-12 15:01:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/11224)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11760)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-12 15:01:31]

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

[commit-bot: I haz the power, 2014-06-12 15:01:32]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[tkent, 2014-06-12 23:40:10]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-06-12 23:40:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/335573004/20001

[commit-bot: I haz the power, 2014-06-13 00:03:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  mac_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/11289)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/11840)

[commit-bot: I haz the power, 2014-06-13 00:14:46]

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

[commit-bot: I haz the power, 2014-06-13 00:14:47]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)

[zerny-chromium, 2014-06-13 08:13:49]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-06-13 08:14:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/335573004/40001

[commit-bot: I haz the power, 2014-06-13 09:23:58]

Change committed as 176102