Oilpan: have Navigator and its supplements be on the heap by default.

Oilpan: have Navigator and its supplements be on the heap by default.

R=haraken
BUG=479467
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194442

[sof, 2015-04-24 15:09:16]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-04-24 15:09:17]

please take a look.

[haraken, 2015-04-24 15:32:35]

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

[haraken, 2015-04-24 15:32:36]

LGTM

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.h:39: Navigator& m_navigator;

This should be Member<Navigator>.

yutak@: I think we should improve the GC plugin so that it can catch references
to on-heap objects. Currnetly the plugin can catch only raw pointers to on-heap
objects.

BTW, it seems strange that NavigatorXXX needs to hold a reference to XXX. Is it
a mis-design?

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:45: // TODO(Oilpan): when
~DOMWindowProperty is removed, derive from GarbageCollected<> instead.

Can we move these comments to DOMWindowProperty and remove them from each
subclass? I guess we'll need to add more comments in the future.

[haraken, 2015-04-24 15:37:17]

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:87:
AsyncMethodRunner<NavigatorGamepad> m_dispatchOneEventRunner;

It seems that AsyncMethodRunner is not safe for lazy sweeping. I think we need
to implement the isHeapObjectAlive logic we implemented for Timer::fired()
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
for AsyncMethodRunner::fired() as well.

[sof, 2015-04-24 15:40:10]

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:87:
AsyncMethodRunner<NavigatorGamepad> m_dispatchOneEventRunner;
On 2015/04/24 15:37:17, haraken wrote:
> 
> It seems that AsyncMethodRunner is not safe for lazy sweeping. I think we need
> to implement the isHeapObjectAlive logic we implemented for Timer::fired()
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> for AsyncMethodRunner::fired() as well.

You have looked at the comment next to AsyncMethodRunner::m_object ?

[haraken, 2015-04-24 15:44:44]

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:87:
AsyncMethodRunner<NavigatorGamepad> m_dispatchOneEventRunner;
On 2015/04/24 15:40:10, sof wrote:
> On 2015/04/24 15:37:17, haraken wrote:
> > 
> > It seems that AsyncMethodRunner is not safe for lazy sweeping. I think we
need
> > to implement the isHeapObjectAlive logic we implemented for Timer::fired()
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > for AsyncMethodRunner::fired() as well.
> 
> You have looked at the comment next to AsyncMethodRunner::m_object ?

That is a different issue than the issue mentioned in the comment.

AsyncMethodRunner::fired() can call a function that can touch other on-heap
objects that have been already swept in the lazy sweeping. If the m_object is
going to be swept soon in the current lazy sweeping, we should avoid invoking
the fired() method. You've taken care of the problem for Timer by implementing
TimerIsObjectAliveTrait, and I think we need the same mechanism here.

[sof, 2015-04-24 15:55:13]

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:87:
AsyncMethodRunner<NavigatorGamepad> m_dispatchOneEventRunner;
On 2015/04/24 15:44:44, haraken wrote:
> On 2015/04/24 15:40:10, sof wrote:
> > On 2015/04/24 15:37:17, haraken wrote:
> > > 
> > > It seems that AsyncMethodRunner is not safe for lazy sweeping. I think we
> need
> > > to implement the isHeapObjectAlive logic we implemented for Timer::fired()
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > > for AsyncMethodRunner::fired() as well.
> > 
> > You have looked at the comment next to AsyncMethodRunner::m_object ?
> 
> That is a different issue than the issue mentioned in the comment.
> 
> AsyncMethodRunner::fired() can call a function that can touch other on-heap
> objects that have been already swept in the lazy sweeping. If the m_object is
> going to be swept soon in the current lazy sweeping, we should avoid invoking
> the fired() method. You've taken care of the problem for Timer by implementing
> TimerIsObjectAliveTrait, and I think we need the same mechanism here.
> 

Thanks, now I remember the non-local nature of this problem. I suppose we would
get it for "free" if AsyncMethodRunner was on the heap -- it's Timer would then
do the check before firing.

> 
>

[sof, 2015-04-24 21:25:08]

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.h:39: Navigator& m_navigator;
On 2015/04/24 15:32:35, haraken wrote:
> 
> This should be Member<Navigator>.
> 
> yutak@: I think we should improve the GC plugin so that it can catch
references
> to on-heap objects. Currnetly the plugin can catch only raw pointers to
on-heap
> objects.
> 
> BTW, it seems strange that NavigatorXXX needs to hold a reference to XXX. Is
it
> a mis-design?

A convenience, Navigator would live as long as the supplement. Turned
NavigatorBeacon into a LocalFrameLifecycleObserver instead.

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
File Source/modules/gamepad/NavigatorGamepad.h (right):

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:45: // TODO(Oilpan): when
~DOMWindowProperty is removed, derive from GarbageCollected<> instead.
On 2015/04/24 15:32:36, haraken wrote:
> 
> Can we move these comments to DOMWindowProperty and remove them from each
> subclass? I guess we'll need to add more comments in the future.

Done; I considered some time ago adding DOMWindowPropertyGarbageCollected which
would be a GarbageCollectedMixin, but I don't think the churn is worth it.

https://codereview.chromium.org/1101263003/diff/20001/Source/modules/gamepad/...
Source/modules/gamepad/NavigatorGamepad.h:87:
AsyncMethodRunner<NavigatorGamepad> m_dispatchOneEventRunner;
On 2015/04/24 15:55:13, sof wrote:
> On 2015/04/24 15:44:44, haraken wrote:
> > On 2015/04/24 15:40:10, sof wrote:
> > > On 2015/04/24 15:37:17, haraken wrote:
> > > > 
> > > > It seems that AsyncMethodRunner is not safe for lazy sweeping. I think
we
> > need
> > > > to implement the isHeapObjectAlive logic we implemented for
Timer::fired()
> > > >
> > >
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > > > for AsyncMethodRunner::fired() as well.
> > > 
> > > You have looked at the comment next to AsyncMethodRunner::m_object ?
> > 
> > That is a different issue than the issue mentioned in the comment.
> > 
> > AsyncMethodRunner::fired() can call a function that can touch other on-heap
> > objects that have been already swept in the lazy sweeping. If the m_object
is
> > going to be swept soon in the current lazy sweeping, we should avoid
invoking
> > the fired() method. You've taken care of the problem for Timer by
implementing
> > TimerIsObjectAliveTrait, and I think we need the same mechanism here.
> > 
> 
> Thanks, now I remember the non-local nature of this problem. I suppose we
would
> get it for "free" if AsyncMethodRunner was on the heap -- it's Timer would
then
> do the check before firing.
> 

Addressing this one via https://codereview.chromium.org/1108663002/

[haraken, 2015-04-25 08:15:00]

LGTM

https://codereview.chromium.org/1101263003/diff/40001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/1101263003/diff/40001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:116: allowed =
BeaconLoader::sendBeacon(impl.frame(), allowance, url,
data.getAsArrayBufferView(), bytes);

navigator.frame() and impl.frame() are guarnateed to be the same?

[sof, 2015-04-25 08:19:34]

https://codereview.chromium.org/1101263003/diff/40001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/1101263003/diff/40001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:116: allowed =
BeaconLoader::sendBeacon(impl.frame(), allowance, url,
data.getAsArrayBufferView(), bytes);
On 2015/04/25 08:15:00, haraken wrote:
> 
> navigator.frame() and impl.frame() are guarnateed to be the same?

Yes, doesn't it make sense to work over the 'this' (==impl) rather than some
other object here?

[haraken, 2015-04-25 08:31:14]

https://codereview.chromium.org/1101263003/diff/40001/Source/modules/beacon/N...
File Source/modules/beacon/NavigatorBeacon.cpp (right):

https://codereview.chromium.org/1101263003/diff/40001/Source/modules/beacon/N...
Source/modules/beacon/NavigatorBeacon.cpp:116: allowed =
BeaconLoader::sendBeacon(impl.frame(), allowance, url,
data.getAsArrayBufferView(), bytes);
On 2015/04/25 08:19:34, sof wrote:
> On 2015/04/25 08:15:00, haraken wrote:
> > 
> > navigator.frame() and impl.frame() are guarnateed to be the same?
> 
> Yes, doesn't it make sense to work over the 'this' (==impl) rather than some
> other object here?

Makes sense.

[sof, 2015-04-25 16:50:01]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-04-25 16:50:02]

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

[commit-bot: I haz the power, 2015-04-25 16:50:12]

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

[commit-bot: I haz the power, 2015-04-25 18:04:13]

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