Move PlatformEventDispatcher implementations to the Oilpan heap.

Move PlatformEventDispatcher implementations to the Oilpan heap.

Make PlatformEventDispatcher a GC mixin, maintaining weak references
to its registered controllers.

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

[sof, 2014-10-02 21:41:36]

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

[sof, 2014-10-02 21:41:37]

Please take a look.

PlatformEventDispatcher::m_controllers is kept as a vector (a HeapVector of bare
pointers to avoid a finalizer for the object.)

[haraken, 2014-10-03 01:55:32]

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

[haraken, 2014-10-03 01:55:33]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:28: void
PlatformEventDispatcher::removeController(PlatformEventController* controller)

We need to make sure that removeController() is never called in a destructor of
on-heap objects.

Currently ~BatteryManager calls stopUpdating() and it calls in
removeController().

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers);

Why do you trace m_controllers? If we trace m_controllers, it will strongly mark
all the controllers. Then clearWeakMembers will never run.

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.h (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.h:34:
HeapVector<PlatformEventController*> m_controllers;

I'd prefer using HeapVector<WeakMember<PlatformEventController>>.

https://codereview.chromium.org/619383002/diff/1/Source/modules/device_light/...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/modules/device_light/...
Source/modules/device_light/DeviceLightDispatcher.cpp:15:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>,
deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher())));

Shouldn't this be Persistent<DeviceLightDispatcher>, since DeviceLightDispatcher
is on-heap in both oilpan and non-oilpan?

I wonder why using OwnPtrWillBePersistent doesn't cause compilation issues.

https://codereview.chromium.org/619383002/diff/1/Source/modules/device_orient...
File Source/modules/device_orientation/DeviceOrientationDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/modules/device_orient...
Source/modules/device_orientation/DeviceOrientationDispatcher.cpp:42:
DEFINE_STATIC_LOCAL(DeviceOrientationDispatcher, deviceOrientationDispatcher,
());

Let's use the Persistent<DeviceOrientationDispatcher> pattern here as well.

[sof, 2014-10-03 05:05:29]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers);
On 2014/10/03 01:55:33, haraken wrote:
> 
> Why do you trace m_controllers? If we trace m_controllers, it will strongly
mark
> all the controllers. Then clearWeakMembers will never run.

It's a HeapVector<> of PlatformEventController* - that will mark the vector
backing store allocation, but not the elements.

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.h (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.h:34:
HeapVector<PlatformEventController*> m_controllers;
On 2014/10/03 01:55:33, haraken wrote:
> 
> I'd prefer using HeapVector<WeakMember<PlatformEventController>>.

Do we support WeakMember<> in HeapVector<> ?

[haraken, 2014-10-03 05:36:02]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers);
On 2014/10/03 05:05:29, sof wrote:
> On 2014/10/03 01:55:33, haraken wrote:
> > 
> > Why do you trace m_controllers? If we trace m_controllers, it will strongly
> mark
> > all the controllers. Then clearWeakMembers will never run.
> 
> It's a HeapVector<> of PlatformEventController* - that will mark the vector
> backing store allocation, but not the elements. 

Makes sense. Shall we add a comment?

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.h (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.h:34:
HeapVector<PlatformEventController*> m_controllers;
On 2014/10/03 05:05:29, sof wrote:
> On 2014/10/03 01:55:33, haraken wrote:
> > 
> > I'd prefer using HeapVector<WeakMember<PlatformEventController>>.
> 
> Do we support WeakMember<> in HeapVector<> ?

oh, you're right.

[sof, 2014-10-03 06:34:04]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:28: void
PlatformEventDispatcher::removeController(PlatformEventController* controller)
On 2014/10/03 01:55:33, haraken wrote:
> 
> We need to make sure that removeController() is never called in a destructor
of
> on-heap objects.
> 
> Currently ~BatteryManager calls stopUpdating() and it calls in
> removeController().

The singleton dispatcher instances are held by persistents, so why is this a
problem?

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers);
On 2014/10/03 05:36:01, haraken wrote:
> On 2014/10/03 05:05:29, sof wrote:
> > On 2014/10/03 01:55:33, haraken wrote:
> > > 
> > > Why do you trace m_controllers? If we trace m_controllers, it will
strongly
> > mark
> > > all the controllers. Then clearWeakMembers will never run.
> > 
> > It's a HeapVector<> of PlatformEventController* - that will mark the vector
> > backing store allocation, but not the elements. 
> 
> Makes sense. Shall we add a comment?

Done. 

I incorrectly assumed that PlatformEventControllers were all on the heap by
default; they're not, m_controllers needs to be a WillBeHeapVector<> as a
result.

[haraken, 2014-10-03 10:10:56]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:28: void
PlatformEventDispatcher::removeController(PlatformEventController* controller)
On 2014/10/03 06:34:04, sof wrote:
> On 2014/10/03 01:55:33, haraken wrote:
> > 
> > We need to make sure that removeController() is never called in a destructor
> of
> > on-heap objects.
> > 
> > Currently ~BatteryManager calls stopUpdating() and it calls in
> > removeController().
> 
> The singleton dispatcher instances are held by persistents, so why is this a
> problem?

ah, you're right.

However, it looks a bit inconsistent that only ~BatteryManager calls
stopUpdating() while other PlatormEventControllers don't call stopUpdating() in
~BatteryManager.

Probably can we just remove the stopUpdating() in ~BatteryManager? Even if we
don't call it, the BatteryManager will be removed from the m_controllers by the
weak processing.

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers);
On 2014/10/03 06:34:04, sof wrote:
> On 2014/10/03 05:36:01, haraken wrote:
> > On 2014/10/03 05:05:29, sof wrote:
> > > On 2014/10/03 01:55:33, haraken wrote:
> > > > 
> > > > Why do you trace m_controllers? If we trace m_controllers, it will
> strongly
> > > mark
> > > > all the controllers. Then clearWeakMembers will never run.
> > > 
> > > It's a HeapVector<> of PlatformEventController* - that will mark the
vector
> > > backing store allocation, but not the elements. 
> > 
> > Makes sense. Shall we add a comment?
> 
> Done. 
> 
> I incorrectly assumed that PlatformEventControllers were all on the heap by
> default; they're not, m_controllers needs to be a WillBeHeapVector<> as a
> result.

Can we move DeviceSingleWindowEventController, NavigatorGamepad and 
ScreenOrientationController to the heap at the same time?

It's confusing to use WillBeHeapVector because not all of
PlatformEventControllers are on the heap.

[sof, 2014-10-03 10:27:12]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:28: void
PlatformEventDispatcher::removeController(PlatformEventController* controller)
On 2014/10/03 10:10:56, haraken wrote:
> On 2014/10/03 06:34:04, sof wrote:
> > On 2014/10/03 01:55:33, haraken wrote:
> > > 
> > > We need to make sure that removeController() is never called in a
destructor
> > of
> > > on-heap objects.
> > > 
> > > Currently ~BatteryManager calls stopUpdating() and it calls in
> > > removeController().
> > 
> > The singleton dispatcher instances are held by persistents, so why is this a
> > problem?
> 
> ah, you're right.
> 
> However, it looks a bit inconsistent that only ~BatteryManager calls
> stopUpdating() while other PlatormEventControllers don't call stopUpdating()
in
> ~BatteryManager.
> 
> Probably can we just remove the stopUpdating() in ~BatteryManager? Even if we
> don't call it, the BatteryManager will be removed from the m_controllers by
the
> weak processing.
> 

With Oilpan enabled, it is redundant, but not without Oilpan - no weak
processing is done.

We could make that clear with ENABLE(OILPAN) usage, if that's preferable.

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:85: visitor->trace(m_controllers);
On 2014/10/03 10:10:56, haraken wrote:
> On 2014/10/03 06:34:04, sof wrote:
> > On 2014/10/03 05:36:01, haraken wrote:
> > > On 2014/10/03 05:05:29, sof wrote:
> > > > On 2014/10/03 01:55:33, haraken wrote:
> > > > > 
> > > > > Why do you trace m_controllers? If we trace m_controllers, it will
> > strongly
> > > > mark
> > > > > all the controllers. Then clearWeakMembers will never run.
> > > > 
> > > > It's a HeapVector<> of PlatformEventController* - that will mark the
> vector
> > > > backing store allocation, but not the elements. 
> > > 
> > > Makes sense. Shall we add a comment?
> > 
> > Done. 
> > 
> > I incorrectly assumed that PlatformEventControllers were all on the heap by
> > default; they're not, m_controllers needs to be a WillBeHeapVector<> as a
> > result.
> 
> Can we move DeviceSingleWindowEventController, NavigatorGamepad and 
> ScreenOrientationController to the heap at the same time?
> 
> It's confusing to use WillBeHeapVector because not all of
> PlatformEventControllers are on the heap.

NavigatorGamepad is a navigator supplement, so we cannot move it without also
having Navigator be on the heap always(*). Ditto for ScreenOrientationController
and LocalFrame.

* - assuming we don't want to reintroduce PersistentHeapSupplementables<>, which
I don't think we want to..?

[sof, 2014-10-03 12:21:17]

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
File Source/core/frame/PlatformEventDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.cpp:28: void
PlatformEventDispatcher::removeController(PlatformEventController* controller)
On 2014/10/03 10:27:11, sof wrote:
> On 2014/10/03 10:10:56, haraken wrote:
> > On 2014/10/03 06:34:04, sof wrote:
> > > On 2014/10/03 01:55:33, haraken wrote:
> > > > 
> > > > We need to make sure that removeController() is never called in a
> destructor
> > > of
> > > > on-heap objects.
> > > > 
> > > > Currently ~BatteryManager calls stopUpdating() and it calls in
> > > > removeController().
> > > 
> > > The singleton dispatcher instances are held by persistents, so why is this
a
> > > problem?
> > 
> > ah, you're right.
> > 
> > However, it looks a bit inconsistent that only ~BatteryManager calls
> > stopUpdating() while other PlatormEventControllers don't call stopUpdating()
> in
> > ~BatteryManager.
> > 
> > Probably can we just remove the stopUpdating() in ~BatteryManager? Even if
we
> > don't call it, the BatteryManager will be removed from the m_controllers by
> the
> > weak processing.
> > 
> 
> With Oilpan enabled, it is redundant, but not without Oilpan - no weak
> processing is done.
> 
> We could make that clear with ENABLE(OILPAN) usage, if that's preferable.

Went ahead and added such.

The Oilpan-specific call to stopUpdating() needed in ~NavigatorGamepad was
removed entirely (it was originally added for reliable DOMWindowProperty
finalization.)

[haraken, 2014-10-03 13:18:17]

LGTM

https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/Platfo...
File Source/core/frame/PlatformEventDispatcher.h (right):

https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/Platfo...
Source/core/frame/PlatformEventDispatcher.h:32: void clearWeakMembers(Visitor*);

You can add #if ENABLE(OILPAN).

https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
Source/modules/device_light/DeviceLightDispatcher.cpp:15:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>,
deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher())));

I still don't understand why this isn't Persistent<DeviceLightDispatcher>.

OwnPtr<DeviceLightDispatcher> should cause a compile error, I guess.

[sof, 2014-10-03 13:33:58]

Thanks for the review.

https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/Platfo...
File Source/core/frame/PlatformEventDispatcher.h (right):

https://codereview.chromium.org/619383002/diff/60001/Source/core/frame/Platfo...
Source/core/frame/PlatformEventDispatcher.h:32: void clearWeakMembers(Visitor*);
On 2014/10/03 13:18:17, haraken wrote:
> 
> You can add #if ENABLE(OILPAN).

Added.

https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
Source/modules/device_light/DeviceLightDispatcher.cpp:15:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>,
deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher())));
On 2014/10/03 13:18:17, haraken wrote:
> 
> I still don't understand why this isn't Persistent<DeviceLightDispatcher>.
> 
> OwnPtr<DeviceLightDispatcher> should cause a compile error, I guess.

Ouch, there should be no use of OwnPtr<> here. Fixed, thanks for catching that.

adoptPtr() takes a bare pointer, so that won't elicit a complaint.

[haraken, 2014-10-03 14:15:07]

https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
File Source/modules/device_light/DeviceLightDispatcher.cpp (right):

https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
Source/modules/device_light/DeviceLightDispatcher.cpp:15:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>,
deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher())));
On 2014/10/03 13:33:58, sof wrote:
> On 2014/10/03 13:18:17, haraken wrote:
> > 
> > I still don't understand why this isn't Persistent<DeviceLightDispatcher>.
> > 
> > OwnPtr<DeviceLightDispatcher> should cause a compile error, I guess.
> 
> Ouch, there should be no use of OwnPtr<> here. Fixed, thanks for catching
that.
> 
> adoptPtr() takes a bare pointer, so that won't elicit a complaint.

Ideally we want to insert an assert to adoptPtr() and verify that adoptPtr()
does not adopt GarbageCollected objects. Previously we tried to insert the
assert but we failed, because the assert requires to violate our directory
dependency rules (wtf/ cannot depend on platform/heap or something like that). I
forgot the exact reason why we couldn't. It might be worth revisiting the issue.

[sof, 2014-10-03 14:31:55]

On 2014/10/03 14:15:07, haraken wrote:
>
https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
> File Source/modules/device_light/DeviceLightDispatcher.cpp (right):
> 
>
https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
> Source/modules/device_light/DeviceLightDispatcher.cpp:15:
> DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>,
> deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher())));
> On 2014/10/03 13:33:58, sof wrote:
> > On 2014/10/03 13:18:17, haraken wrote:
> > > 
> > > I still don't understand why this isn't Persistent<DeviceLightDispatcher>.
> > > 
> > > OwnPtr<DeviceLightDispatcher> should cause a compile error, I guess.
> > 
> > Ouch, there should be no use of OwnPtr<> here. Fixed, thanks for catching
> that.
> > 
> > adoptPtr() takes a bare pointer, so that won't elicit a complaint.
> 
> Ideally we want to insert an assert to adoptPtr() and verify that adoptPtr()
> does not adopt GarbageCollected objects. Previously we tried to insert the
> assert but we failed, because the assert requires to violate our directory
> dependency rules (wtf/ cannot depend on platform/heap or something like that).
I
> forgot the exact reason why we couldn't. It might be worth revisiting the
issue.

The GC plugin _could_ also traverse method bodies and look for
OwnPtr<SomeGCedClass>, but a lot more work for questionable payback.

[haraken, 2014-10-03 14:36:33]

On 2014/10/03 14:31:55, sof wrote:
> On 2014/10/03 14:15:07, haraken wrote:
> >
>
https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
> > File Source/modules/device_light/DeviceLightDispatcher.cpp (right):
> > 
> >
>
https://codereview.chromium.org/619383002/diff/60001/Source/modules/device_li...
> > Source/modules/device_light/DeviceLightDispatcher.cpp:15:
> > DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<DeviceLightDispatcher>,
> > deviceLightDispatcher, (adoptPtrWillBeNoop(new DeviceLightDispatcher())));
> > On 2014/10/03 13:33:58, sof wrote:
> > > On 2014/10/03 13:18:17, haraken wrote:
> > > > 
> > > > I still don't understand why this isn't
Persistent<DeviceLightDispatcher>.
> > > > 
> > > > OwnPtr<DeviceLightDispatcher> should cause a compile error, I guess.
> > > 
> > > Ouch, there should be no use of OwnPtr<> here. Fixed, thanks for catching
> > that.
> > > 
> > > adoptPtr() takes a bare pointer, so that won't elicit a complaint.
> > 
> > Ideally we want to insert an assert to adoptPtr() and verify that adoptPtr()
> > does not adopt GarbageCollected objects. Previously we tried to insert the
> > assert but we failed, because the assert requires to violate our directory
> > dependency rules (wtf/ cannot depend on platform/heap or something like
that).
> I
> > forgot the exact reason why we couldn't. It might be worth revisiting the
> issue.
> 
> The GC plugin _could_ also traverse method bodies and look for
> OwnPtr<SomeGCedClass>, but a lot more work for questionable payback.

Yeah, the GC plugin can catch OwnPtr<SomeGCedClass> in on-heap objects, but
cannot catch OwnPtr<SomeGCedClass>s in method bodies. I guess it will be a lot
of work :-/

[sof, 2014-10-03 18:37:51]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-03 18:38:14]

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

[commit-bot: I haz the power, 2014-10-03 20:09:56]

Committed patchset #5 (id:80001) as 183211