Replace ActiveDOMObjects in modules/indexeddb/ with ContextLifecycleObservers

Replace ActiveDOMObjects in modules/indexeddb/ with ContextLifecycleObservers

Per the plan described in https://groups.google.com/a/chromium.org/d/topic/blink-dev/UwRGJgLBElo/discussion,
this CL replaces ActiveDOMObjects in modules/mediastream/ with ContextLifecycleObservers.

m_contextStopped is equivalent with !executionContext().
So this CL removes m_contextStopped.

BUG=589507

[haraken, 2016-02-29 08:25:50]

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

[haraken, 2016-02-29 08:25:51]

PTAL

[haraken, 2016-02-29 08:34:54]

Description was changed from

==========
Replace ActiveDOMObjects in modules/indexeddb/ with ContextLifecycleObservers

Per the plan described in
https://groups.google.com/a/chromium.org/d/topic/blink-dev/UwRGJgLBElo/discus...,
this CL replaces ActiveDOMObjects in modules/mediastream/ with
ContextLifecycleObservers.

BUG=589507
==========

to

==========
Replace ActiveDOMObjects in modules/indexeddb/ with ContextLifecycleObservers

Per the plan described in
https://groups.google.com/a/chromium.org/d/topic/blink-dev/UwRGJgLBElo/discus...,
this CL replaces ActiveDOMObjects in modules/mediastream/ with
ContextLifecycleObservers.

m_contextStopped is equivalent with !executionContext().
So this CL removes m_contextStopped.

BUG=589507
==========

[haraken, 2016-02-29 11:22:22]

haraken@chromium.org changed reviewers:
+ sigbjornf@opera.com

[haraken, 2016-02-29 11:22:23]

Looks like the bots are not happy -- will take a look tomorrow.

[jsbell, 2016-02-29 18:35:44]

IIRC some of the `if (m_contextDestroyed) return;` guards are because the
notification order is indeterminate, so various IDB objects notify each other
where order is sensitive, then set a flag to avoid duplicate processing.
(Hopefully that's what the failing unit tests are exercising!)

[sof, 2016-03-01 07:04:26]

https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):

https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:383:
abort(IGNORE_EXCEPTION);
afaict, the abort() call here used to be a no-op. It no longer is as
executionContext() isn't cleared by the time contextDestroyed() notifications
run.

[haraken, 2016-03-01 07:05:46]

On 2016/03/01 07:04:26, sof wrote:
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):
> 
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:383:
> abort(IGNORE_EXCEPTION);
> afaict, the abort() call here used to be a no-op. It no longer is as
> executionContext() isn't cleared by the time contextDestroyed() notifications
> run.

Yes, that's the problem. I'm now trying to explicitly clear LifecycleObserver's
context in contextDestroyed() (https://codereview.chromium.org/1752693002), but
a couple of tests are failing.

[sof, 2016-03-01 07:37:22]

On 2016/03/01 07:05:46, haraken wrote:
> On 2016/03/01 07:04:26, sof wrote:
> >
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:383:
> > abort(IGNORE_EXCEPTION);
> > afaict, the abort() call here used to be a no-op. It no longer is as
> > executionContext() isn't cleared by the time contextDestroyed()
notifications
> > run.
> 
> Yes, that's the problem. I'm now trying to explicitly clear
LifecycleObserver's
> context in contextDestroyed() (https://codereview.chromium.org/1752693002),
but
> a couple of tests are failing.

I seem to vaguely remember something like that being attempted while shaking up
lifecycle notifier/observer handling some time ago. As part of your series of
improvements, if you want/need to go digging for potential clues.

[haraken, 2016-03-01 08:06:18]

On 2016/03/01 07:37:22, sof wrote:
> On 2016/03/01 07:05:46, haraken wrote:
> > On 2016/03/01 07:04:26, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> > > File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
(right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> > > third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:383:
> > > abort(IGNORE_EXCEPTION);
> > > afaict, the abort() call here used to be a no-op. It no longer is as
> > > executionContext() isn't cleared by the time contextDestroyed()
> notifications
> > > run.
> > 
> > Yes, that's the problem. I'm now trying to explicitly clear
> LifecycleObserver's
> > context in contextDestroyed() (https://codereview.chromium.org/1752693002),
> but
> > a couple of tests are failing.
> 
> I seem to vaguely remember something like that being attempted while shaking
up
> lifecycle notifier/observer handling some time ago. As part of your series of
> improvements, if you want/need to go digging for potential clues.

Yeah, I think this needs to be fixed.

We already have a bunch of code that checks 'if(!executionContext())'. Currently
the behavior depends on the timing when a GC is triggered and collects the
ContextLifecycleObserver, which is not nice.

[haraken, 2016-03-01 09:40:26]

On 2016/03/01 08:06:18, haraken wrote:
> On 2016/03/01 07:37:22, sof wrote:
> > On 2016/03/01 07:05:46, haraken wrote:
> > > On 2016/03/01 07:04:26, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> > > > File third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp
> (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1745933002/diff/1/third_party/WebKit/Source/m...
> > > > third_party/WebKit/Source/modules/indexeddb/IDBTransaction.cpp:383:
> > > > abort(IGNORE_EXCEPTION);
> > > > afaict, the abort() call here used to be a no-op. It no longer is as
> > > > executionContext() isn't cleared by the time contextDestroyed()
> > notifications
> > > > run.
> > > 
> > > Yes, that's the problem. I'm now trying to explicitly clear
> > LifecycleObserver's
> > > context in contextDestroyed()
(https://codereview.chromium.org/1752693002),
> > but
> > > a couple of tests are failing.
> > 
> > I seem to vaguely remember something like that being attempted while shaking
> up
> > lifecycle notifier/observer handling some time ago. As part of your series
of
> > improvements, if you want/need to go digging for potential clues.
> 
> Yeah, I think this needs to be fixed.
> 
> We already have a bunch of code that checks 'if(!executionContext())'.
Currently
> the behavior depends on the timing when a GC is triggered and collects the
> ContextLifecycleObserver, which is not nice.

I played around it for a while and realized that this is not as easy work as I
expected. There are many classes that inherit from ContextLifecycleObserver with
an assumption that ContextLifecycleObserver::m_context is never null. Those
classes should stop using ContextLifecycleObserver and instead just hold
Member<ExecutionContext>.

Also there are a lot of code that depends on the weak processing on
ContextLifecycleObserver::m_context (i.e., if(!executionContext())). To make the
behavior deterministic, we should clear the m_context explicitly, but it will
break classes that put the above assumption.

Hmm.