Oilpan: reliably track listening PlatformEventControllers.

Oilpan: reliably track listening PlatformEventControllers.

Turn PlatformEventController into a GC mixin with Oilpan enabled, and
have the PlatformEventDispatcher instance track those via a Persistent
reference.

It seems precarious to rely on the controller lifetimes be maintained by
way of other references 'long enough' for the embedder's platform event
handling layer.

R=mlamouri,mkwst,haraken,timvolodine
BUG=419515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183158

[sof, 2014-09-30 11:44:37]

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

[sof, 2014-09-30 11:44:37]

Please take a look.

Goes together with the Chromium-side https://codereview.chromium.org/613203002/
to some extent.

[Mike West, 2014-09-30 11:51:21]

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

[Mike West, 2014-09-30 11:51:21]

Someone else should review the oilpan changes, but the non-oilpan changes (in
particular the `!document` check) LGTM.

[haraken, 2014-09-30 12:02:13]

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

[haraken, 2014-09-30 12:02:14]

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

https://codereview.chromium.org/618673002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.h:31:
WillBePersistentHeapVector<RawPtrWillBeMember<PlatformEventController> >
m_controllers;

Are you sure that this persistent heap vector of members doesn't cause a memory
leak? I'm wondering this since you're changing a raw pointer to a persistent
handle.

(Previously I tried to move PlatformEventDispatcher to the heap at the same time
and make m_controllers a HeapHashMap of weak members, although I didn't complete
the CL at that time :-)

[haraken, 2014-09-30 12:03:33]

On 2014/09/30 12:02:14, haraken wrote:
>
https://codereview.chromium.org/618673002/diff/1/Source/core/frame/PlatformEv...
> File Source/core/frame/PlatformEventDispatcher.h (right):
> 
>
https://codereview.chromium.org/618673002/diff/1/Source/core/frame/PlatformEv...
> Source/core/frame/PlatformEventDispatcher.h:31:
> WillBePersistentHeapVector<RawPtrWillBeMember<PlatformEventController> >
> m_controllers;
> 
> Are you sure that this persistent heap vector of members doesn't cause a
memory
> leak? I'm wondering this since you're changing a raw pointer to a persistent
> handle.
> 
> (Previously I tried to move PlatformEventDispatcher to the heap at the same
time
> and make m_controllers a HeapHashMap of weak members, although I didn't
complete
> the CL at that time :-)

s/HeapHashMap/HeapHashSet/.

FYI, if we move PlatformEventDispatcher to the heap, we have to make
m_controllers a HeapHashSet of weak members because ~BatteryManager() tries to
remove the controller from m_controllers.

[sof, 2014-09-30 12:12:21]

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

https://codereview.chromium.org/618673002/diff/1/Source/core/frame/PlatformEv...
Source/core/frame/PlatformEventDispatcher.h:31:
WillBePersistentHeapVector<RawPtrWillBeMember<PlatformEventController> >
m_controllers;
On 2014/09/30 12:02:14, haraken wrote:
> 
> Are you sure that this persistent heap vector of members doesn't cause a
memory
> leak? I'm wondering this since you're changing a raw pointer to a persistent
> handle.
> 
> (Previously I tried to move PlatformEventDispatcher to the heap at the same
time
> and make m_controllers a HeapHashMap of weak members, although I didn't
complete
> the CL at that time :-)

I haven't done a complete testrun with --enable-leak-detection, but the
module-specific tests that are relevant here don't show up leaks afaict.

That's for leaks on shutdown. If this unnecessarily prolong lifetimes, is a
separate matter. But the onus on unregistering ought to be with the controllers.

[mlamouri (slow - plz ping), 2014-09-30 17:08:49]

https://codereview.chromium.org/618673002/diff/1/Source/modules/battery/Batte...
File Source/modules/battery/BatteryManager.cpp (right):

https://codereview.chromium.org/618673002/diff/1/Source/modules/battery/Batte...
Source/modules/battery/BatteryManager.cpp:92: if (!document ||
document->activeDOMObjectsAreSuspended() ||
document->activeDOMObjectsAreStopped())
Is there any situation where we expect !document? Should we assert instead?

[haraken, 2014-10-01 15:58:27]

LGTM

It would be great if we could move PlatformEventDispatcher to the heap and make
PlatformEventDispatcher::m_controllers a hash set of weak members in a follow-up
CL. It will give us a more proper lifetime semantics, since the m_controllers
are weak conceptually.

[sof, 2014-10-02 07:47:15]

On 2014/09/30 17:08:49, Mounir Lamouri wrote:
>
https://codereview.chromium.org/618673002/diff/1/Source/modules/battery/Batte...
> File Source/modules/battery/BatteryManager.cpp (right):
> 
>
https://codereview.chromium.org/618673002/diff/1/Source/modules/battery/Batte...
> Source/modules/battery/BatteryManager.cpp:92: if (!document ||
> document->activeDOMObjectsAreSuspended() ||
> document->activeDOMObjectsAreStopped())
> Is there any situation where we expect !document? Should we assert instead?

Yes, there is, but I think we can avoid the condition. It can currently happen
as a result of calling navigator.getBattery() on a detached document & with the
leak in https://codereview.chromium.org/619583004 plugged (*). The document can
then be GCed, but without the updating being stopped.

But why not just override contextDestroyed() and stop the updating then? Due to
BatteryManager being a double lifecycle observer (Page and
ActiveDOMObject/ExecutionContext), this isn't readily possible.

I suggest we instead don't start the updating if the ExecutionContext is in a
stopped state in startRequest(). That way we can simply assert a non-null
Document here. Sounds reasonable?

* - https://code.google.com/p/chromium/issues/detail?id=419515 is what triggers
without checking for a non-null document. I erroneously thought that check was
only needed due to using Persistents here w/Oilpan, and didn't include it in
https://codereview.chromium.org/619583004.

[sof, 2014-10-02 08:11:04]

On 2014/10/02 07:47:15, sof wrote:
> On 2014/09/30 17:08:49, Mounir Lamouri wrote:
> >
>
https://codereview.chromium.org/618673002/diff/1/Source/modules/battery/Batte...
> > File Source/modules/battery/BatteryManager.cpp (right):
> > 
> >
>
https://codereview.chromium.org/618673002/diff/1/Source/modules/battery/Batte...
> > Source/modules/battery/BatteryManager.cpp:92: if (!document ||
> > document->activeDOMObjectsAreSuspended() ||
> > document->activeDOMObjectsAreStopped())
> > Is there any situation where we expect !document? Should we assert instead?
> 
> Yes, there is, but I think we can avoid the condition. It can currently happen
> as a result of calling navigator.getBattery() on a detached document & with
the
> leak in https://codereview.chromium.org/619583004 plugged (*). The document
can
> then be GCed, but without the updating being stopped.
> 
> But why not just override contextDestroyed() and stop the updating then? Due
to
> BatteryManager being a double lifecycle observer (Page and
> ActiveDOMObject/ExecutionContext), this isn't readily possible.
> 
> I suggest we instead don't start the updating if the ExecutionContext is in a
> stopped state in startRequest(). That way we can simply assert a non-null
> Document here. Sounds reasonable?
> 
> * - https://code.google.com/p/chromium/issues/detail?id=419515 is what
triggers
> without checking for a non-null document. I erroneously thought that check was
> only needed due to using Persistents here w/Oilpan, and didn't include it in
> https://codereview.chromium.org/619583004.

Now done; please take another look?

[sof, 2014-10-02 08:37:52]

On 2014/10/01 15:58:27, haraken wrote:
> LGTM
> 
> It would be great if we could move PlatformEventDispatcher to the heap and
make
> PlatformEventDispatcher::m_controllers a hash set of weak members in a
follow-up
> CL. It will give us a more proper lifetime semantics, since the m_controllers
> are weak conceptually.

We can try in a followup; handling the removal of controllers while dispatching
will require some care.

[mlamouri (slow - plz ping), 2014-10-02 10:29:18]

mlamouri@chromium.org changed reviewers:
+ timvolodine@chromium.org

[mlamouri (slow - plz ping), 2014-10-02 10:29:19]

lgtm. Let's get Tim's opinion on the change given that he is owning the battery
module.

[sof, 2014-10-02 11:52:26]

On 2014/10/02 10:29:19, Mounir Lamouri wrote:
> lgtm. Let's get Tim's opinion on the change given that he is owning the
battery
> module.

thanks, certainly.

will put up https://codereview.chromium.org/620683006/ for review once this has
landed to move PlatformEventDispatchers to the heap.

[timvolodine, 2014-10-02 16:22:35]

On 2014/10/02 11:52:26, sof wrote:
> On 2014/10/02 10:29:19, Mounir Lamouri wrote:
> > lgtm. Let's get Tim's opinion on the change given that he is owning the
> battery
> > module.
> 
> thanks, certainly.
> 
> will put up https://codereview.chromium.org/620683006/ for review once this
has
> landed to move PlatformEventDispatchers to the heap.

I don't really see how this fixes battery page-visibility test crbug.com/419515,
but the idea not to start battery api when the the active dom objects are
stopped seems reasonable.
also there seems to be crrev.com/622733002 which has recently landed I wonder if
that fixes the test..

[sof, 2014-10-02 17:40:55]

On 2014/10/02 16:22:35, timvolodine wrote:
> On 2014/10/02 11:52:26, sof wrote:
> > On 2014/10/02 10:29:19, Mounir Lamouri wrote:
> > > lgtm. Let's get Tim's opinion on the change given that he is owning the
> > battery
> > > module.
> > 
> > thanks, certainly.
> > 
> > will put up https://codereview.chromium.org/620683006/ for review once this
> has
> > landed to move PlatformEventDispatchers to the heap.
> 
> I don't really see how this fixes battery page-visibility test
crbug.com/419515,
> but the idea not to start battery api when the the active dom objects are
> stopped seems reasonable.
> also there seems to be crrev.com/622733002 which has recently landed I wonder
if
> that fixes the test..

Marking page-visibility.html as crashing in crbug.com/419515 isn't right, it's
the test one prior to that no-leak-on-detached-use.html that's the problem (and
is all about detached use.)

crrev.com/622733002 generalizes the leak fix, no change in behavior for
battery/.

[sof, 2014-10-02 17:56:43]

Based on timvolodine's general nod of approval :) , I'll go ahead and land this.
Will separately followup on any other feedback, of course.

[sof, 2014-10-02 17:56:47]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-02 17:57:39]

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

[commit-bot: I haz the power, 2014-10-02 20:12:41]

Committed patchset #2 (id:2) as 183158