Allow ActiveDOMObjects to be destructed while iterating the list of ActiveDOMObjects

Allow ActiveDOMObjects to be destructed while iterating ContextLifeCycleNotifier::m_activeDOMObjects

Currently we are not allowed to destruct ActiveDOMObjects while iterating ContextLifeCycleNotifier::m_activeDOMObjects. This restriction is needed in order not to confuse the iteration. However, the restriction is too strong and became a real issue in https://codereview.chromium.org/218953002/. Specifically, we want to make the following scenario workable:

(1) ContextLifeCycleNotifier::notifyStoppingActiveDOMObjects() starts iterating m_activeDOMObjects.
(2) stop() is notified on an IDBRequest (which is an ActiveDOMObject).
(3) The stop() clears a RefPtr<IDBDatabase> (IDBDatabase is also an ActiveDOMObject).
(4) IDBDatabase is destructed.

This CL changes ContextLifecycleNotifier so that we can destruct ActiveDOMObjects while iterating ContextLifeCycleNotifier::m_activeDOMObjects.

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

[haraken, 2014-04-22 11:06:08]

morrita-san: Would you take a look?

[abarth-chromium, 2014-04-22 16:20:06]

not lgtm

The problem here is that the IDB code is doing too much work inside the stop()
callback.  Rather than making this iteration more complex, we should do less
work inside stop().

[haraken, 2014-04-23 00:56:57]

On 2014/04/22 16:20:06, abarth wrote:
> not lgtm
> 
> The problem here is that the IDB code is doing too much work inside the stop()
> callback.  Rather than making this iteration more complex, we should do less
> work inside stop().

hmm, I have no good idea at this point. (ccing jsbell@)

One way to prevent ~ActiveDOMObject from getting called inside stop() is to use
a one shot timer for delaying derefing RefPtr<IDBDatabase>. This hack is already
used in XMLHttpRequest::stop(). However, IMHO, it would be better to improve the
ActiveDOMObject infrastructure and remove the one shot timer hacks from IDB code
and XMLHttpRequest code. What do you think?

FWIW, I cleaned up the patch set so that it doesn't use an additional hash set
of ActiveDOMObjects (i.e., removed m_removedActiveDOMObjects in the patch set
1).

[Hajime Morrita, 2014-04-23 01:10:36]

Just a random idea:

Probably we could ask the context to remove/delete specific object after its
cleanup phase is done, instead of letting it remove itself directly.
If we hook removedLastRef()-like phase of IDBDatabase and ask the context to
delete it later, we can do this. 

RefCounted currently doesn't have removedLastRef() so we need some alternation
if we take this approach.
Once this lazy deletion pattern becomes possible, we can possibly turn all the
ActiveDOMObjects lazily-deleted objects.
Kinda like poor-mans mark-and-sweep.

[haraken, 2014-04-23 01:31:17]

On 2014/04/23 01:10:36, morrita1 wrote:
> Just a random idea:
> 
> Probably we could ask the context to remove/delete specific object after its
> cleanup phase is done, instead of letting it remove itself directly.
> If we hook removedLastRef()-like phase of IDBDatabase and ask the context to
> delete it later, we can do this. 
> 
> RefCounted currently doesn't have removedLastRef() so we need some alternation
> if we take this approach.
> Once this lazy deletion pattern becomes possible, we can possibly turn all the
> ActiveDOMObjects lazily-deleted objects.
> Kinda like poor-mans mark-and-sweep.

That's a possible approach.

If we want to make all ActiveDOMObjects lazily-deleted, a conceptually better
approach would be to let ContextLifecycleNotifier add ref() to all the
ActiveDOMObjects before starting the iteration and deref() them after finishing
the iteration. By doing this, we can make all the ActiveDOMObjects
lazily-deleted.

However, the problem of the approach is that ActiveDOMObject cannot become
RefCounted, because it will create a diamond inheritance.

  class ActiveDOMObject : public RefCounted<ActiveDOMObject> { ... };
  class DOMObject : public RefCounted<DOMObject>, public ActiveDOMObject { ...
};

Given the above, I guess that the approach in the patch set 3 would be the
simplest to achieve what we want to achieve at the moment.

What do you think?

[abarth-chromium, 2014-04-23 03:23:37]

On 2014/04/23 01:31:17, haraken wrote:
> What do you think?

Maybe we should create a ReleaseSoon templated subclass of Task that would make
it easy to deref an object via the event loop?  Chromium has such a thing.

[haraken, 2014-04-23 07:31:51]

On 2014/04/23 03:23:37, abarth wrote:
> On 2014/04/23 01:31:17, haraken wrote:
> > What do you think?
> 
> Maybe we should create a ReleaseSoon templated subclass of Task that would
make
> it easy to deref an object via the event loop?  Chromium has such a thing.

I tried that but noticed that it won't solve the problem I need to solve now :-/

When shutting down a worker thread, things need to happen in the following order
(See https://codereview.chromium.org/218953002/ for more details):

(1) IDBRequest::stop() is called.
(2) IDBRequest::m_result is cleared and ~IDBDatabase() is called.
(3) Call didStopWorkerRunLoop() in the Chromium side.
(4) Other shutdown logic of the worker thread.

If we delay (2) to the next event loop, we also need to delay (3) to the next of
the next event loop. This means that we need multiple event loops just for
shutting down a worker thread, which sounds bad.

abarth: Do you think that the patch set 3 is complicated? I guess it's a common
thing to take a snapshot of a hashset before starting iterating the hash set.

Another option might be to refactor IDB code so that didStopWorkerRunLoop() can
be called before ~IDBDatabase(), but I've not yet much investigated it.
(Conceptually didStopWorkerRunLoop() should be called after disconnecting IDB
backends, so I'm not sure if this is a right thing to do.)

[abarth-chromium, 2014-04-23 17:54:22]

On 2014/04/23 07:31:51, haraken wrote:
> On 2014/04/23 03:23:37, abarth wrote:
> > On 2014/04/23 01:31:17, haraken wrote:
> > > What do you think?
> > 
> > Maybe we should create a ReleaseSoon templated subclass of Task that would
> make
> > it easy to deref an object via the event loop?  Chromium has such a thing.
> 
> I tried that but noticed that it won't solve the problem I need to solve now
:-/
> 
> When shutting down a worker thread, things need to happen in the following
order
> (See https://codereview.chromium.org/218953002/ for more details):
> 
> (1) IDBRequest::stop() is called.
> (2) IDBRequest::m_result is cleared and ~IDBDatabase() is called.
> (3) Call didStopWorkerRunLoop() in the Chromium side.
> (4) Other shutdown logic of the worker thread.
> 
> If we delay (2) to the next event loop, we also need to delay (3) to the next
of
> the next event loop. This means that we need multiple event loops just for
> shutting down a worker thread, which sounds bad.

Maybe we should defer the work until the end of the microtask instead of through
the event loop?

> abarth: Do you think that the patch set 3 is complicated? I guess it's a
common
> thing to take a snapshot of a hashset before starting iterating the hash set.

The problem is that we might not deliver stop() notifications to everyone who
registered themselves.  If a new ActiveDOMObject is created during the
iteration, it won't receive the stop notification.

[haraken, 2014-04-24 01:00:04]

Thanks for review, and I'm sorry for iterating much on this CL.

On 2014/04/23 17:54:22, abarth wrote:
> On 2014/04/23 07:31:51, haraken wrote:
> > On 2014/04/23 03:23:37, abarth wrote:
> > > On 2014/04/23 01:31:17, haraken wrote:
> > > > What do you think?
> > > 
> > > Maybe we should create a ReleaseSoon templated subclass of Task that would
> > make
> > > it easy to deref an object via the event loop?  Chromium has such a thing.
> > 
> > I tried that but noticed that it won't solve the problem I need to solve now
> :-/
> > 
> > When shutting down a worker thread, things need to happen in the following
> order
> > (See https://codereview.chromium.org/218953002/ for more details):
> > 
> > (1) IDBRequest::stop() is called.
> > (2) IDBRequest::m_result is cleared and ~IDBDatabase() is called.
> > (3) Call didStopWorkerRunLoop() in the Chromium side.
> > (4) Other shutdown logic of the worker thread.
> > 
> > If we delay (2) to the next event loop, we also need to delay (3) to the
next
> of
> > the next event loop. This means that we need multiple event loops just for
> > shutting down a worker thread, which sounds bad.
> 
> Maybe we should defer the work until the end of the microtask instead of
through
> the event loop?

hmm, I've not yet experimented but it looks too involved. It's easy to move the
IDB thing (i.e. (2) and (3)) into the microtask, but hard to move the
complicated shutdown logic of worker threads (i.e. (4)) into the microtask. A
more modest option would be to move (2) and (3) to the microtask and move (4) to
the next event loop, but it will a bit complicate the shutdown logic as well.

> > abarth: Do you think that the patch set 3 is complicated? I guess it's a
> common
> > thing to take a snapshot of a hashset before starting iterating the hash
set.
> 
> The problem is that we might not deliver stop() notifications to everyone who
> registered themselves.  If a new ActiveDOMObject is created during the
> iteration, it won't receive the stop notification.

Currently creation of new ActiveDOMObjects is forbidden during the iteration (We
have an RELEASE_ASSERT). So the situation is as follows:

(a) ActiveDOMObjects cannot be created nor destructed during the iteration. (We
are now here.)
(b) ActiveDOMObjects cannot be created but can be destructed during the
iteration. (This CL is going to reach here.)
(c) ActiveDOMObjects can be created and destructed during the iteration. (We
might want to reach here in the future.)

If you think that (b) is half-baked and (c) is fine, I can implement (c) in this
CL.

[haraken, 2014-04-24 10:37:50]

On 2014/04/24 01:00:04, haraken wrote:
> Thanks for review, and I'm sorry for iterating much on this CL.
> 
> On 2014/04/23 17:54:22, abarth wrote:
> > On 2014/04/23 07:31:51, haraken wrote:
> > > On 2014/04/23 03:23:37, abarth wrote:
> > > > On 2014/04/23 01:31:17, haraken wrote:
> > > > > What do you think?
> > > > 
> > > > Maybe we should create a ReleaseSoon templated subclass of Task that
would
> > > make
> > > > it easy to deref an object via the event loop?  Chromium has such a
thing.
> > > 
> > > I tried that but noticed that it won't solve the problem I need to solve
now
> > :-/
> > > 
> > > When shutting down a worker thread, things need to happen in the following
> > order
> > > (See https://codereview.chromium.org/218953002/ for more details):
> > > 
> > > (1) IDBRequest::stop() is called.
> > > (2) IDBRequest::m_result is cleared and ~IDBDatabase() is called.
> > > (3) Call didStopWorkerRunLoop() in the Chromium side.
> > > (4) Other shutdown logic of the worker thread.
> > > 
> > > If we delay (2) to the next event loop, we also need to delay (3) to the
> next
> > of
> > > the next event loop. This means that we need multiple event loops just for
> > > shutting down a worker thread, which sounds bad.
> > 
> > Maybe we should defer the work until the end of the microtask instead of
> through
> > the event loop?
> 
> hmm, I've not yet experimented but it looks too involved. It's easy to move
the
> IDB thing (i.e. (2) and (3)) into the microtask, but hard to move the
> complicated shutdown logic of worker threads (i.e. (4)) into the microtask. A
> more modest option would be to move (2) and (3) to the microtask and move (4)
to
> the next event loop, but it will a bit complicate the shutdown logic as well.
> 
> > > abarth: Do you think that the patch set 3 is complicated? I guess it's a
> > common
> > > thing to take a snapshot of a hashset before starting iterating the hash
> set.
> > 
> > The problem is that we might not deliver stop() notifications to everyone
who
> > registered themselves.  If a new ActiveDOMObject is created during the
> > iteration, it won't receive the stop notification.
> 
> Currently creation of new ActiveDOMObjects is forbidden during the iteration
(We
> have an RELEASE_ASSERT). So the situation is as follows:
> 
> (a) ActiveDOMObjects cannot be created nor destructed during the iteration.
(We
> are now here.)
> (b) ActiveDOMObjects cannot be created but can be destructed during the
> iteration. (This CL is going to reach here.)
> (c) ActiveDOMObjects can be created and destructed during the iteration. (We
> might want to reach here in the future.)
> 
> If you think that (b) is half-baked and (c) is fine, I can implement (c) in
this
> CL.

tkent@ invented a better solution to disconnect IDB backends more elegantly
(https://codereview.chromium.org/257573002/). That CL makes this CL unnecessary,
so closing this issue.

[abarth-chromium, 2014-04-24 15:01:13]

On 2014/04/24 10:37:50, haraken wrote:
> tkent@ invented a better solution to disconnect IDB backends more elegantly
> (https://codereview.chromium.org/257573002/). That CL makes this CL
unnecessary,
> so closing this issue.

Thanks!  Sorry for the resistance here.

[haraken, 2014-05-08 23:34:20]

(Reopening this CL)

abarth@: We hit another issue
(https://code.google.com/p/chromium/issues/detail?id=371458) because
ActiveDOMObject cannot be destructed while iterating
ContextLifeCycleNotifier::m_activeDOMObjects. The issue is that a sweeping phase
can be triggered during ActiveDOMObject::stop() and the sweeping phase destructs
other ActiveDOMObjects.

There are two solutions:

(a) Refactor the code so that the sweeping phase is not called in
ActiveDOMObject::stop().
(b) Allow ActiveDOMObjects to get destructed while iterating the list of
ActiveDOMObjects.

I think (a) is fragile. In order to ensure that the sweeping phase is never
called in ActiveDOMObject::stop(), we need to disallow any object allocation in
stop(). This sounds a bit too restrictive.

So I think (b) is better and we need a CL something like this, but what do you
think?

[abarth-chromium, 2014-05-09 03:20:06]

On 2014/05/08 23:34:20, haraken wrote:
> So I think (b) is better and we need a CL something like this, but what do you
> think?

Ok, I relent.  These get removed during the sweep because these references are
weak?  Presumably weak collections need removal-resistant iterators in general
because of this issue...

[abarth-chromium, 2014-05-09 03:20:38]

Do you still like the approach in this CL or do you plan to upload a new CL?

[haraken, 2014-05-09 03:35:13]

On 2014/05/09 03:20:06, abarth wrote:
> On 2014/05/08 23:34:20, haraken wrote:
> > So I think (b) is better and we need a CL something like this, but what do
you
> > think?
> 
> Ok, I relent.  These get removed during the sweep because these references are
> weak?  Presumably weak collections need removal-resistant iterators in general
> because of this issue...

In my understanding, it's not related to weak or strong.

(1) Some ActiveDOMObjects become unreachable. (It doesn't matter whether they
are strong-referenced or weak-referenced.)
(2) ActiveDOMObject::stop() is called. stop() can allocate something on the
heap, which can trigger a GC.
(3) The GC destructs unreachable ActiveDOMObjects. Then ~ActiveDOMObject() is
called during stop().

> Do you still like the approach in this CL or do you plan to upload a new CL?

Yes, I like the approach in the patch set 3, but I'm happy to improve it based
on your comments :)

[abarth-chromium, 2014-05-09 04:34:12]

On 2014/05/09 03:35:13, haraken wrote:
> On 2014/05/09 03:20:06, abarth wrote:
> > On 2014/05/08 23:34:20, haraken wrote:
> > > So I think (b) is better and we need a CL something like this, but what do
> you
> > > think?
> > 
> > Ok, I relent.  These get removed during the sweep because these references
are
> > weak?  Presumably weak collections need removal-resistant iterators in
general
> > because of this issue...
> 
> In my understanding, it's not related to weak or strong.
> 
> (1) Some ActiveDOMObjects become unreachable. (It doesn't matter whether they
> are strong-referenced or weak-referenced.)
> (2) ActiveDOMObject::stop() is called. stop() can allocate something on the
> heap, which can trigger a GC.
> (3) The GC destructs unreachable ActiveDOMObjects. Then ~ActiveDOMObject() is
> called during stop().

If the Vector is trong, none of the ActiveDOMObjects referenced in the Vector
will be unreachable because they're referenced by the Vector...

[tkent, 2014-05-09 04:39:36]

On 2014/05/09 03:35:13, haraken wrote:
> (1) Some ActiveDOMObjects become unreachable. (It doesn't matter whether they
> are strong-referenced or weak-referenced.)

Do you mean an ActiveDOMObject is unreachable, but it is in the observer list of
the context?

[abarth-chromium, 2014-05-09 04:48:11]

On 2014/05/09 04:39:36, tkent wrote:
> On 2014/05/09 03:35:13, haraken wrote:
> > (1) Some ActiveDOMObjects become unreachable. (It doesn't matter whether
they
> > are strong-referenced or weak-referenced.)
> 
> Do you mean an ActiveDOMObject is unreachable, but it is in the observer list
of
> the context?

I think that can happen only if the observer list is weak.  Maybe another
solution is to make it strong so they're never finalized during iteration.

[haraken, 2014-05-09 04:50:18]

On 2014/05/09 04:34:12, abarth wrote:
> On 2014/05/09 03:35:13, haraken wrote:
> > On 2014/05/09 03:20:06, abarth wrote:
> > > On 2014/05/08 23:34:20, haraken wrote:
> > > > So I think (b) is better and we need a CL something like this, but what
do
> > you
> > > > think?
> > > 
> > > Ok, I relent.  These get removed during the sweep because these references
> are
> > > weak?  Presumably weak collections need removal-resistant iterators in
> general
> > > because of this issue...
> > 
> > In my understanding, it's not related to weak or strong.
> > 
> > (1) Some ActiveDOMObjects become unreachable. (It doesn't matter whether
they
> > are strong-referenced or weak-referenced.)
> > (2) ActiveDOMObject::stop() is called. stop() can allocate something on the
> > heap, which can trigger a GC.
> > (3) The GC destructs unreachable ActiveDOMObjects. Then ~ActiveDOMObject()
is
> > called during stop().
> 
> If the Vector is trong, none of the ActiveDOMObjects referenced in the Vector
> will be unreachable because they're referenced by the Vector...

ah, I understand what you mean.

Yes, if we move ActiveDOMObjects to oilpan's heap and use
HeapHashSet<WeakMember<ActiveDOMObject>> for m_activeDOMObjects, then the issue
will be gone. (Oilpan's GC visits on-stack weak pointers strongly, so it doesn't
happen that ~ActiveDOMObject() is called while iterating m_activeDOMObjects.)

Given that the crash is happening only in oilpan builds, as you mentioned, a
right fix will be moving ActiveDOMObjects to oilpan's heap and use
HeapHashSet<WeakMember<ActiveDOMObject>>. I'll take a look.

[haraken, 2014-05-09 04:55:11]

On 2014/05/09 04:48:11, abarth wrote:
> On 2014/05/09 04:39:36, tkent wrote:
> > On 2014/05/09 03:35:13, haraken wrote:
> > > (1) Some ActiveDOMObjects become unreachable. (It doesn't matter whether
> they
> > > are strong-referenced or weak-referenced.)
> > 
> > Do you mean an ActiveDOMObject is unreachable, but it is in the observer
list
> of
> > the context?
> 
> I think that can happen only if the observer list is weak.  Maybe another
> solution is to make it strong so they're never finalized during iteration.

- In non-oilpan, it's not easy to make m_activeDOMObjects strong, because
ActiveDOMObject cannot inherit from RefCounted. See my comment in
https://codereview.chromium.org/247253002/#msg6

- In oilpan, we can simply make m_activeDOMObjects weak. It is safe because
oilpan's GC visits on-stack weak pointers strongly, which guarantees that
ActiveDOMObjects are not destructed during iteration.

- The problem we're seeing happens in oilpan only. We don't have any call path
that calls ~ActiveDOMObject() in stop() in non-oilpan. This means that a right
fix for us is to move ActiveDOMObjects to the heap and make m_activeDOMObjects
weak.

[tkent, 2014-05-09 04:59:34]

On 2014/05/09 04:50:18, haraken wrote:
> Given that the crash is happening only in oilpan builds, as you mentioned, a
> right fix will be moving ActiveDOMObjects to oilpan's heap and use
> HeapHashSet<WeakMember<ActiveDOMObject>>. I'll take a look.

crbug.com/371458 is enable_oilpan=0 build.  Your approach needs to enable Oilpan
by default for all of ActiveDOMObjects.

[haraken, 2014-05-09 05:04:38]

On 2014/05/09 04:59:34, tkent wrote:
> On 2014/05/09 04:50:18, haraken wrote:
> > Given that the crash is happening only in oilpan builds, as you mentioned, a
> > right fix will be moving ActiveDOMObjects to oilpan's heap and use
> > HeapHashSet<WeakMember<ActiveDOMObject>>. I'll take a look.
> 
> crbug.com/371458 is enable_oilpan=0 build.  Your approach needs to enable
Oilpan
> by default for all of ActiveDOMObjects.

Right... Even if enable_oilpan is 0, oilpan's GC is running, so we have the
issue in non-oilpan as well.

[tkent, 2014-05-09 05:10:41]

On 2014/05/09 05:04:38, haraken wrote:
> On 2014/05/09 04:59:34, tkent wrote:
> > On 2014/05/09 04:50:18, haraken wrote:
> > > Given that the crash is happening only in oilpan builds, as you mentioned,
a
> > > right fix will be moving ActiveDOMObjects to oilpan's heap and use
> > > HeapHashSet<WeakMember<ActiveDOMObject>>. I'll take a look.
> > 
> > crbug.com/371458 is enable_oilpan=0 build.  Your approach needs to enable
> Oilpan
> > by default for all of ActiveDOMObjects.
> 
> Right... Even if enable_oilpan is 0, oilpan's GC is running, so we have the
> issue in non-oilpan as well.

I agree that HeapHashSet<WeakMember<ActiveDOMObject>> is the right approach.
IMO, we should disable Oilpan for modules/notification, which is the only
instance of ActiveDOMObject&&GarbageCollected, in short term.

[haraken, 2014-05-09 05:16:23]

On 2014/05/09 05:10:41, tkent wrote:
> On 2014/05/09 05:04:38, haraken wrote:
> > On 2014/05/09 04:59:34, tkent wrote:
> > > On 2014/05/09 04:50:18, haraken wrote:
> > > > Given that the crash is happening only in oilpan builds, as you
mentioned,
> a
> > > > right fix will be moving ActiveDOMObjects to oilpan's heap and use
> > > > HeapHashSet<WeakMember<ActiveDOMObject>>. I'll take a look.
> > > 
> > > crbug.com/371458 is enable_oilpan=0 build.  Your approach needs to enable
> > Oilpan
> > > by default for all of ActiveDOMObjects.
> > 
> > Right... Even if enable_oilpan is 0, oilpan's GC is running, so we have the
> > issue in non-oilpan as well.
> 
> I agree that HeapHashSet<WeakMember<ActiveDOMObject>> is the right approach.
> IMO, we should disable Oilpan for modules/notification, which is the only
> instance of ActiveDOMObject&&GarbageCollected, in short term.

Yeah, in short term (e.g., until the branch cut), that will work. However, it
means that we cannot enable oilpan for any ActiveDOMObject until we move the
ActiveDOMObject class to the heap. That sounds not realistic.

- One approach is this CL.

- Another (probably worse) approach is to disable oilpan's GC in stop().

- If there is a way to make m_activeDOMObjects strong in non-oilpan, it would be
the best.

[Mads Ager (chromium), 2014-05-09 06:53:01]

On 2014/05/09 05:16:23, haraken wrote:
> On 2014/05/09 05:10:41, tkent wrote:
> > On 2014/05/09 05:04:38, haraken wrote:
> > > On 2014/05/09 04:59:34, tkent wrote:
> > > > On 2014/05/09 04:50:18, haraken wrote:
> > > > > Given that the crash is happening only in oilpan builds, as you
> mentioned,
> > a
> > > > > right fix will be moving ActiveDOMObjects to oilpan's heap and use
> > > > > HeapHashSet<WeakMember<ActiveDOMObject>>. I'll take a look.
> > > > 
> > > > crbug.com/371458 is enable_oilpan=0 build.  Your approach needs to
enable
> > > Oilpan
> > > > by default for all of ActiveDOMObjects.
> > > 
> > > Right... Even if enable_oilpan is 0, oilpan's GC is running, so we have
the
> > > issue in non-oilpan as well.
> > 
> > I agree that HeapHashSet<WeakMember<ActiveDOMObject>> is the right approach.
> > IMO, we should disable Oilpan for modules/notification, which is the only
> > instance of ActiveDOMObject&&GarbageCollected, in short term.
> 
> Yeah, in short term (e.g., until the branch cut), that will work. However, it
> means that we cannot enable oilpan for any ActiveDOMObject until we move the
> ActiveDOMObject class to the heap. That sounds not realistic.
> 
> - One approach is this CL.
> 
> - Another (probably worse) approach is to disable oilpan's GC in stop().
> 
> - If there is a way to make m_activeDOMObjects strong in non-oilpan, it would
be
> the best.

The long term right solution is to completely get rid of this set of observers.
With Oilpan an execution context does not need to know who are observing it.
Instead ContextLifecycleObserver should add a WeakMember to anything that
inherits from it and the notification should be driven by oilpan weak
processing. Therefore, LifecycleContext and LifecycleNotifier should be
completely removed. Here is a HeapTest that you can put into HeapTest.cpp if you
want to play with it:

template<typename T>
class LifecycleObserver : public GarbageCollectedMixin {
public:
    LifecycleObserver(T* context) : m_context(context) { }

   virtual void contextDestroyed() { }

   T* context() const { return m_context; }

   void processWeakMembers(Visitor* visitor)
   {
        if (visitor->isAlive(m_context))
            return;
        m_context = nullptr;
        contextDestroyed();
   }

   virtual void trace(Visitor* visitor)
   {
        visitor->registerWeakMembers<LifecycleObserver,
&LifecycleObserver::processWeakMembers>(this);
   }

protected:
    WeakMember<T> m_context;
};

class Context : public GarbageCollected<Context> {
public:
    void trace(Visitor*) { }
};

class ActiveObject : public GarbageCollected<ActiveObject>, public
LifecycleObserver<Context> {
    USING_GARBAGE_COLLECTED_MIXIN(ActiveObject);
public:
    ActiveObject(Context* context) : LifecycleObserver(context),
m_notified(false) { }

    virtual void contextDestroyed() OVERRIDE
    {
        m_notified = true;
    }

    void trace(Visitor* visitor)
    {
        LifecycleObserver::trace(visitor);
    }

    bool m_notified;
};


TEST(HeapTest, LifecycleObserver)
{
    Persistent<Context> context = new Context();
    Persistent<ActiveObject> activeObject = new ActiveObject(context);
    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
    EXPECT_FALSE(activeObject->m_notified);
    context.clear();
    Heap::collectGarbage(ThreadState::NoHeapPointersOnStack);
    EXPECT_TRUE(activeObject->m_notified);
}

That doesn't really help right now though, but I thought it important to mention
how this will be simplified longer term

Currently, I see only the two options already listed:

1) do not ship anything that is an ActiveObject in the oilpan heap until all
ActiveObjects are in the oilpan heap and ship them all together, or
2) land this patch to allow modification during iteration

It would be nice to be able to ship in smaller increments, so I find 2) the most
appealing. We should make sure to add comments stating that this is only needed
because of Oilpan. With oilpan this set will be completely removed. If we back
out of Oilpan we should undo this change.

[haraken, 2014-05-09 07:26:21]

Thanks Mads for the clarification! Added a FIXME to the CL.

> The long term right solution is to completely get rid of this set of
observers.
> With Oilpan an execution context does not need to know who are observing it.
> Instead ContextLifecycleObserver should add a WeakMember to anything that
> inherits from it and the notification should be driven by oilpan weak
> processing.

I'm not sure if we can remove the observer list in long term. The weak
processing will work for sending notifications for stop(), but in order to send
notifications for resume() and suspend(), we'll still need to maintain the
observer list in ContextLifecycleNotifier.

PTAL.

[Mads Ager (chromium), 2014-05-09 07:53:02]

On 2014/05/09 07:26:21, haraken wrote:
> Thanks Mads for the clarification! Added a FIXME to the CL.
> 
> > The long term right solution is to completely get rid of this set of
> observers.
> > With Oilpan an execution context does not need to know who are observing it.
> > Instead ContextLifecycleObserver should add a WeakMember to anything that
> > inherits from it and the notification should be driven by oilpan weak
> > processing.
> 
> I'm not sure if we can remove the observer list in long term. The weak
> processing will work for sending notifications for stop(), but in order to
send
> notifications for resume() and suspend(), we'll still need to maintain the
> observer list in ContextLifecycleNotifier.

Yes, that's right. If you are registering for events that are not tied to death
of the observee you do need the list of weak pointers to the registered
observers in the observee.

[haraken, 2014-05-09 07:56:30]

abarth@, tkent@: Would you take a look at the patch set 4? The diff from the
patch set 3 is just I added a FIXME.

- If you think the approach is OK, I'd like to land the patch set 4.
- If you don't think the approach is OK, I'll revert modules/notifications back
to non-oilpan at the moment and then reconsider how to address this issue.

[Mads Ager (chromium), 2014-05-09 08:05:43]

https://codereview.chromium.org/247253002/diff/60001/Source/core/dom/ContextL...
File Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/247253002/diff/60001/Source/core/dom/ContextL...
Source/core/dom/ContextLifecycleNotifier.cpp:73: // during the iteration in a
case where resume() allocates an on-heap object
NIT: I think you should leave out the part of the comment that talks about
allocation. Based on the stack trace that we are seeing I don't think this is
related to allocation. I think the point is that a DatabaseThread is being
stopped via an ActiveDOMObject stop call:

Main thread:

1) Document::detach
2) notifyStoppingActiveDOMObjects
3) DatabaseContext::stop
4) DatabaseContext::stopDatabases
5) Post task to database thread and block until it is terminated; this is a safe
point for GC

Database thread:

6) DatabaseThread::cleanupDatabaseThreadCompleted
7) ThreadState::detach
8) Heap::collectAllGarbage (to run all destructors); this sets pending sweep for
all threads including the main thread.
9) TaskSynchronizer::taskCompleted()

Main thread:

10) wake up from safepoint; sweep requested; sweep and finalize dead
ActiveDOMObjects

So, when we have a database thread we are use that we will get a sweep here even
if there are no allocations.

[Mads Ager (chromium), 2014-05-09 08:07:41]

On 2014/05/09 08:05:43, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/247253002/diff/60001/Source/core/dom/ContextL...
> File Source/core/dom/ContextLifecycleNotifier.cpp (right):
> 
>
https://codereview.chromium.org/247253002/diff/60001/Source/core/dom/ContextL...
> Source/core/dom/ContextLifecycleNotifier.cpp:73: // during the iteration in a
> case where resume() allocates an on-heap object
> NIT: I think you should leave out the part of the comment that talks about
> allocation. Based on the stack trace that we are seeing I don't think this is
> related to allocation. I think the point is that a DatabaseThread is being
> stopped via an ActiveDOMObject stop call:
> 
> Main thread:
> 
> 1) Document::detach
> 2) notifyStoppingActiveDOMObjects
> 3) DatabaseContext::stop
> 4) DatabaseContext::stopDatabases
> 5) Post task to database thread and block until it is terminated; this is a
safe
> point for GC
> 
> Database thread:
> 
> 6) DatabaseThread::cleanupDatabaseThreadCompleted
> 7) ThreadState::detach
> 8) Heap::collectAllGarbage (to run all destructors); this sets pending sweep
for
> all threads including the main thread.
> 9) TaskSynchronizer::taskCompleted()
> 
> Main thread:
> 
> 10) wake up from safepoint; sweep requested; sweep and finalize dead
> ActiveDOMObjects
> 
> So, when we have a database thread we are use that we will get a sweep here
even
> if there are no allocations.

s/we are use/we are sure/

[haraken, 2014-05-09 08:12:36]

https://codereview.chromium.org/247253002/diff/60001/Source/core/dom/ContextL...
File Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/247253002/diff/60001/Source/core/dom/ContextL...
Source/core/dom/ContextLifecycleNotifier.cpp:73: // during the iteration in a
case where resume() allocates an on-heap object
On 2014/05/09 08:05:44, Mads Ager (chromium) wrote:
> NIT: I think you should leave out the part of the comment that talks about
> allocation. Based on the stack trace that we are seeing I don't think this is
> related to allocation. I think the point is that a DatabaseThread is being
> stopped via an ActiveDOMObject stop call:
> 
> Main thread:
> 
> 1) Document::detach
> 2) notifyStoppingActiveDOMObjects
> 3) DatabaseContext::stop
> 4) DatabaseContext::stopDatabases
> 5) Post task to database thread and block until it is terminated; this is a
safe
> point for GC
> 
> Database thread:
> 
> 6) DatabaseThread::cleanupDatabaseThreadCompleted
> 7) ThreadState::detach
> 8) Heap::collectAllGarbage (to run all destructors); this sets pending sweep
for
> all threads including the main thread.
> 9) TaskSynchronizer::taskCompleted()
> 
> Main thread:
> 
> 10) wake up from safepoint; sweep requested; sweep and finalize dead
> ActiveDOMObjects
> 
> So, when we have a database thread we are use that we will get a sweep here
even
> if there are no allocations.

Yeah, your observation is correct. An allocation would be another case that will
cause the same issue. For simplicity, I just removed the part from the comment.

[Mads Ager (chromium), 2014-05-09 08:49:23]

Thanks, this looks as good as it can get for shipping ActiveDOMObjects
incrementally with Oilpan.

LGTM from me. Adam should approve though.

[tkent, 2014-05-09 09:02:52]

> - If you don't think the approach is OK, I'll revert modules/notifications
back

Please revert modules/notifications and let's discuss later.

[haraken, 2014-05-09 09:10:11]

On 2014/05/09 09:02:52, tkent wrote:
> > - If you don't think the approach is OK, I'll revert modules/notifications
> back
> 
> Please revert modules/notifications and let's discuss later.

hmm, I noticed we'll also need to revert modules/mediasource and
modules/encryptedmedia as well since they are on heap and ActiveDOMObjects. Do
you want to do that? I'm OK with trying that before the branch cut.

[tkent, 2014-05-09 09:57:12]

On 2014/05/09 09:10:11, haraken wrote:
> hmm, I noticed we'll also need to revert modules/mediasource and
> modules/encryptedmedia as well since they are on heap and ActiveDOMObjects. Do
> you want to do that? I'm OK with trying that before the branch cut.

What classes in modules/{mediasource,encryptedmdia} are already olpaned and
ActiveDOMObject?  I couldn't find them.

[abarth-chromium, 2014-05-09 13:54:39]

https://codereview.chromium.org/247253002/diff/80001/Source/core/dom/ContextL...
File Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/247253002/diff/80001/Source/core/dom/ContextL...
Source/core/dom/ContextLifecycleNotifier.cpp:115: }
I'm worried that ActiveDOMObjects created during this iteration will never get
the stop notification.  Can we forbid additions during these iterations but
allow removals?

[haraken, 2014-05-09 14:05:14]

tkent-san:
> What classes in modules/{mediasource,encryptedmdia} are already olpaned and
> ActiveDOMObject?  I couldn't find them.

Regarding mediasource, MediaSourceBase is GarbageCollected and ActiveDOMObject.

Regarding encryptedmedia, I'm sorry I noticed that the CL is not yet landed
(https://codereview.chromium.org/257503003/).

https://codereview.chromium.org/247253002/diff/80001/Source/core/dom/ContextL...
File Source/core/dom/ContextLifecycleNotifier.cpp (right):

https://codereview.chromium.org/247253002/diff/80001/Source/core/dom/ContextL...
Source/core/dom/ContextLifecycleNotifier.cpp:115: }
On 2014/05/09 13:54:39, abarth wrote:
> I'm worried that ActiveDOMObjects created during this iteration will never get
> the stop notification.  Can we forbid additions during these iterations but
> allow removals?

Creation during iteration is already forbidden by the RELEASE_ASSERT(m_iterating
!= IteratingOverContextObservers) at line 49.

- Previously both creation and destruction are forbidden.
- This CL makes destruction OK. The creation is still forbidden.

[abarth-chromium, 2014-05-09 15:37:32]

Ok, LGTM

Thanks!

[haraken, 2014-05-09 15:40:48]

tkent-san: Let me land this CL at the moment, since I'm not sure if I can be
online until next Monday. I'll rework on this if you have more comments.

[haraken, 2014-05-09 15:40:56]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-09 15:41:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/247253002/80001

[commit-bot: I haz the power, 2014-05-09 16:56:42]

Change committed as 173760