Oilpan: IDBCursor should be detached from IDBRequest when the IDBRequest stops

Oilpan: IDBCursor should be detached from IDBRequest when the IDBRequest stops

This CL fixes the following crash that is happening in oilpan bots flakily.

03:06:22.168 12926 worker/6 storage/indexeddb/cursor-basics.html crashed, (stderr lines):
03:06:22.168 12926   [6:653:0328/030621:3422205696:FATAL:indexed_db_dispatcher.cc(71)] Check failed: false. Re-instantiating TLS IndexedDBDispatcher.
03:06:22.168 12926   #0 0x7f46a76a8ea1 base::debug::StackTrace::StackTrace()
03:06:22.168 12926   #1 0x7f46a772265d logging::LogMessage::~LogMessage()
03:06:22.168 12926   #2 0x7f469cf602f6 content::IndexedDBDispatcher::ThreadSpecificInstance()
03:06:22.168 12926   #3 0x7f469cf78487 content::WebIDBCursorImpl::~WebIDBCursorImpl()
03:06:22.168 12926   #4 0x7f469cf78536 content::WebIDBCursorImpl::~WebIDBCursorImpl()
03:06:22.168 12926   #5 0x7f46a06bb02a WTF::OwnedPtrDeleter<>::deletePtr()
03:06:22.168 12926   #6 0x7f46a09150c3 WTF::OwnPtr<>::~OwnPtr()
03:06:22.168 12926   #7 0x7f46a09120a0 WebCore::IDBCursor::~IDBCursor()
03:06:22.168 12926   #8 0x7f46a0915bce WebCore::IDBCursorWithValue::~IDBCursorWithValue()
03:06:22.168 12926   #9 0x7f46a0915c00 WebCore::IDBCursorWithValue::~IDBCursorWithValue()
03:06:22.168 12926   #10 0x7f46a0911376 WebCore::IDBCursor::deref()
03:06:22.168 12926   #11 0x7f46a09118ff WTF::derefIfNotNull<>()
03:06:22.168 12926   #12 0x7f46a09115bb WTF::RefPtr<>::~RefPtr()
03:06:22.168 12926   #13 0x7f46a0910282 WebCore::IDBAny::~IDBAny()
03:06:22.168 12926   #14 0x7f46a06bb179 WTF::RefCounted<>::deref()
03:06:22.168 12926   #15 0x7f46a06baead WTF::derefIfNotNull<>()
03:06:22.168 12926   #16 0x7f46a09316a4 WTF::RefPtr<>::clear()
03:06:22.168 12926   #17 0x7f46a092eb91 WebCore::IDBRequest::checkForReferenceCycle()
03:06:22.168 12926   #18 0x7f46a092c096 WebCore::IDBRequest::deref()
03:06:22.168 12926   #19 0x7f46a092c033 WebCore::IDBRequest::derefEventTarget()
03:06:22.168 12926   #20 0x7f46a0494a79 WebCore::EventTarget::deref()
03:06:22.168 12926   #21 0x7f46a049552e WTF::derefIfNotNull<>()
03:06:22.168 12926   #22 0x7f46a083fe5d WTF::RefPtr<>::~RefPtr()
03:06:22.169 12926   #23 0x7f46a17aaf7a WebCore::Event::~Event()
03:06:22.169 12926   #24 0x7f46a046f8d9 WebCore::GarbageCollectedFinalized<>::finalizeGarbageCollectedObject()
03:06:22.169 12926   #25 0x7f46a046f833 WebCore::FinalizerTraitImpl<>::finalize()
03:06:22.169 12926   #26 0x7f46a046f5ac WebCore::FinalizerTrait<>::finalize()
03:06:22.169 12926   #27 0x7f46963f028b WebCore::HeapObjectHeader::finalize()
03:06:22.169 12926   #28 0x7f46963f0305 WebCore::FinalizedHeapObjectHeader::finalize()
03:06:22.169 12926   #29 0x7f46963f2ac8 WebCore::HeapPage<>::finalize()
03:06:22.169 12926   #30 0x7f46963f3a64 WebCore::HeapPage<>::sweep()
03:06:22.169 12926   #31 0x7f46963f4e2b WebCore::ThreadHeap<>::sweep()
03:06:22.169 12926   #32 0x7f46963f9f32 WebCore::ThreadState::performPendingSweep()
03:06:22.169 12926   #33 0x7f46963f9c9e WebCore::ThreadState::leaveSafePoint()
03:06:22.169 12926   #34 0x7f46963f1ae9 WebCore::ThreadState::SafePointScope::~SafePointScope()
03:06:22.169 12926   #35 0x7f46963f2a56 WebCore::GCScope::~GCScope()
03:06:22.169 12926   #36 0x7f46963f13d9 WebCore::Heap::collectGarbage()
03:06:22.169 12926   #37 0x7f46963f1402 WebCore::Heap::collectAllGarbage()
03:06:22.169 12926   #38 0x7f46963f8d41 WebCore::ThreadState::cleanup()
03:06:22.169 12926   #39 0x7f46963f8e1e WebCore::ThreadState::detach()
03:06:22.169 12926   #40 0x7f46a1fafad1 WebCore::WorkerThread::workerThread()
03:06:22.169 12926   #41 0x7f46a1faf7d6 WebCore::WorkerThread::workerThreadStart()
03:06:22.169 12926   #42 0x7f4698634f21 WTF::threadEntryPoint()
03:06:22.169 12926   #43 0x7f46986354fd WTF::wtfThreadEntryPoint()
03:06:22.169 12926   #44 0x7f469a330e9a start_thread

The cause of the crash is as follows:

(1) ~WorkerScriptController() calls m_world->dispose().
(2) m_world->dispose() disposes the last reference to an IDBRequest object.
(3) ~WorkerScriptController() calls didStopWorkerRunLoop().
(4) didStopWorkerRunLoop() destructs an IndexedDBDispatcher object in the Chromium side.
(5) At the end of the worker thread execution, ThreadState::detach() is called. ThreadState::detach() triggers a GC.
(6) The GC collects the IDBRequest object.
(7) ~IDBRequest() calls ~IDBCursor(), which touches the IndexedDBDispatcher object, which is already destroyed.

The problem here is that the IDBCursor is not detached from the IDBRequest when the IDBRequest::stop() is called (Note: IDBRequest::stop() is called when the associated execution context is stopped). This CL fixes the crash by clearing out IDBRequest::m_result in IDBRequest::stop().

BUG=340522

[haraken, 2014-03-31 03:45:30]

PTAL

[haraken, 2014-03-31 03:46:49]

If you want to know more about the destruction dependency of IDB objects, you
can refer to https://bugs.webkit.org/show_bug.cgi?id=83104#c14

[zerny-chromium, 2014-03-31 06:33:10]

It seems odd to force multiple GC's before and after didStopWorkerRunLoop. To me
the issue appears to be the WebIDBCursorImpl assuming that it is always torn
down before the dispatcher instance. Could we avoid this assumption? Either
checking that there is indeed a dispatcher, or by invalidating cursors on
dispatcher destruction?

https://codereview.chromium.org/218953002/diff/1/Source/bindings/v8/WorkerScr...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/218953002/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:113: // (3) ThreadState::detach()
This also does a full GC. Do we need this both before and after
didStopWorkerRunLoop?

[Mads Ager (chromium), 2014-03-31 08:56:52]

https://codereview.chromium.org/218953002/diff/1/Source/bindings/v8/WorkerScr...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/218953002/diff/1/Source/bindings/v8/WorkerScr...
Source/bindings/v8/WorkerScriptController.cpp:113: // (3) ThreadState::detach()
On 2014/03/31 06:33:10, zerny-chromium wrote:
> This also does a full GC. Do we need this both before and after
> didStopWorkerRunLoop?

So, we definitely need the detach call and we have to have that where it is now.
It would be really nice if we could make the worker thread invalidate IDBCursors
at this point instead. Maybe we can have a per worker thread weak set of
IDBCursors that we can iterate at this point and invalidate (clear out their
OwnPtr). That is the only thing we actually care about here and therefore,
performing a full garbage collection on all threads in order to invalidate the
(probably very few if any) IDBCursors on this thread seems very heavy weight.

[haraken, 2014-03-31 09:16:34]

Thanks for comments! Actually I've not yet found a solution that makes perfect
sense to me.

Zerny:
> It seems odd to force multiple GC's before and after didStopWorkerRunLoop.

Agreed.


Zerny:
> To me the issue appears to be the WebIDBCursorImpl assuming that it is always
torn
> down before the dispatcher instance. Could we avoid this assumption?

It's doable, but I'm not sure if it's a good idea to do that.

didStopWorkerRunLoop is a message from Blink to Chromium to notify that "Blink
is shutting down, so you can shut down all objects that depend on Blink". In
that sense, I'm not sure if it's a good idea to put some odd assumption on the
notification.


Mads:
> performing a full garbage collection on all threads in order to invalidate the
> (probably very few if any) IDBCursors on this thread seems very heavy weight.

Agreed.


Mads:
> So, we definitely need the detach call and we have to have that where it is
now.
> It would be really nice if we could make the worker thread invalidate
IDBCursors
> at this point instead. Maybe we can have a per worker thread weak set of
> IDBCursors that we can iterate at this point and invalidate (clear out their
> OwnPtr).

This is also doable, but I suspect it's a layering violation to put IDB code
into WorkerThread. We cannot include modules/indexeddb/ in core/workers/.

Once possible work-around would be to implement WorkerThreadObserver and make
IDBCursor and IDBRequest the observers, but it sounds a bit over-killing.

What do you think?

[Mads Ager (chromium), 2014-03-31 09:30:17]

On 2014/03/31 09:16:34, haraken wrote:
> Thanks for comments! Actually I've not yet found a solution that makes perfect
> sense to me.
> 
> Zerny:
> > It seems odd to force multiple GC's before and after didStopWorkerRunLoop.
> 
> Agreed.
> 
> 
> Zerny:
> > To me the issue appears to be the WebIDBCursorImpl assuming that it is
always
> torn
> > down before the dispatcher instance. Could we avoid this assumption?
> 
> It's doable, but I'm not sure if it's a good idea to do that.
> 
> didStopWorkerRunLoop is a message from Blink to Chromium to notify that "Blink
> is shutting down, so you can shut down all objects that depend on Blink". In
> that sense, I'm not sure if it's a good idea to put some odd assumption on the
> notification.
> 
> 
> Mads:
> > performing a full garbage collection on all threads in order to invalidate
the
> > (probably very few if any) IDBCursors on this thread seems very heavy
weight.
> 
> Agreed.
> 
> 
> Mads:
> > So, we definitely need the detach call and we have to have that where it is
> now.
> > It would be really nice if we could make the worker thread invalidate
> IDBCursors
> > at this point instead. Maybe we can have a per worker thread weak set of
> > IDBCursors that we can iterate at this point and invalidate (clear out their
> > OwnPtr).
> 
> This is also doable, but I suspect it's a layering violation to put IDB code
> into WorkerThread. We cannot include modules/indexeddb/ in core/workers/.
> 
> Once possible work-around would be to implement WorkerThreadObserver and make
> IDBCursor and IDBRequest the observers, but it sounds a bit over-killing.
> 
> What do you think?

Can we reuse ContextLifecycleObserver for this? When the worker context is going
away, we notify all requests so they can invalidate their cursors? We might have
to explicitly dispatch the context going away call (instead of tying it to the
actual death of the context).

So, maybe make IDBRequest a ContextLifecycleObserver. During worker shutdown,
call contextShutdown on the worker context which will explicitly iterate the
observers and call their contextDestroyed methods?

[haraken, 2014-03-31 09:33:05]

On 2014/03/31 09:30:17, Mads Ager (chromium) wrote:
> On 2014/03/31 09:16:34, haraken wrote:
> > Thanks for comments! Actually I've not yet found a solution that makes
perfect
> > sense to me.
> > 
> > Zerny:
> > > It seems odd to force multiple GC's before and after didStopWorkerRunLoop.
> > 
> > Agreed.
> > 
> > 
> > Zerny:
> > > To me the issue appears to be the WebIDBCursorImpl assuming that it is
> always
> > torn
> > > down before the dispatcher instance. Could we avoid this assumption?
> > 
> > It's doable, but I'm not sure if it's a good idea to do that.
> > 
> > didStopWorkerRunLoop is a message from Blink to Chromium to notify that
"Blink
> > is shutting down, so you can shut down all objects that depend on Blink". In
> > that sense, I'm not sure if it's a good idea to put some odd assumption on
the
> > notification.
> > 
> > 
> > Mads:
> > > performing a full garbage collection on all threads in order to invalidate
> the
> > > (probably very few if any) IDBCursors on this thread seems very heavy
> weight.
> > 
> > Agreed.
> > 
> > 
> > Mads:
> > > So, we definitely need the detach call and we have to have that where it
is
> > now.
> > > It would be really nice if we could make the worker thread invalidate
> > IDBCursors
> > > at this point instead. Maybe we can have a per worker thread weak set of
> > > IDBCursors that we can iterate at this point and invalidate (clear out
their
> > > OwnPtr).
> > 
> > This is also doable, but I suspect it's a layering violation to put IDB code
> > into WorkerThread. We cannot include modules/indexeddb/ in core/workers/.
> > 
> > Once possible work-around would be to implement WorkerThreadObserver and
make
> > IDBCursor and IDBRequest the observers, but it sounds a bit over-killing.
> > 
> > What do you think?
> 
> Can we reuse ContextLifecycleObserver for this? When the worker context is
going
> away, we notify all requests so they can invalidate their cursors? We might
have
> to explicitly dispatch the context going away call (instead of tying it to the
> actual death of the context).
> 
> So, maybe make IDBRequest a ContextLifecycleObserver. During worker shutdown,
> call contextShutdown on the worker context which will explicitly iterate the
> observers and call their contextDestroyed methods?

This sounds like a reasonable approach. Let me give it a try.

[haraken, 2014-04-01 05:56:13]

Mads:
> Can we reuse ContextLifecycleObserver for this? When the worker context is
going
> away, we notify all requests so they can invalidate their cursors? We might
have
> to explicitly dispatch the context going away call (instead of tying it to the
> actual death of the context).
> 
> So, maybe make IDBRequest a ContextLifecycleObserver. During worker shutdown,
> call contextShutdown on the worker context which will explicitly iterate the
> observers and call their contextDestroyed methods?

I uploaded a proof-of-concept CL, but it looks a bit too hacky to me. What do
you think?

https://codereview.chromium.org/218953002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/218953002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:203: clearScript();

I'm afraid this is very hacky. This code is the way it is to make sure that
things happen in the following order:

(1) DOMWrapperWorld is disposed.
(2) contextWillBeDestroyed() is called on IDBRequest objects, and they die.
(3) didStopWorkerRunLoop() notification is sent to the Chromium side.
(4) ThreadState::detach() is called.
(5) Isolate is disposed.

https://codereview.chromium.org/218953002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/218953002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:374: checkForReferenceCycle();

I need to reconsider what I should do here. Currently this code results in an
infinite loop in some layout tests.

[haraken, 2014-04-01 06:08:03]

> performing a full garbage collection on all threads in order to invalidate the
> (probably very few if any) IDBCursors on this thread seems very heavy weight.

I agree that it's not a good idea to trigger a full GC just for collecting IDB
objects, but the performance impact of doing a full GC might not be as bad as we
imagine. Given that we need to cause a full GC in ThreadState::detach(), we
anyway need to stop the main thread when shutting down a worker thread. If we
anyway need to stop the main thread, it won't matter if we trigger one full GC
or two full GCs (the second full GC will be very light since there will be few
objects to collect).

[Mads Ager (chromium), 2014-04-01 11:38:37]

https://codereview.chromium.org/218953002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerGlobalScope.cpp (right):

https://codereview.chromium.org/218953002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerGlobalScope.cpp:203: clearScript();
On 2014/04/01 05:56:14, haraken wrote:
> 
> I'm afraid this is very hacky. This code is the way it is to make sure that
> things happen in the following order:
> 
> (1) DOMWrapperWorld is disposed.
> (2) contextWillBeDestroyed() is called on IDBRequest objects, and they die.
> (3) didStopWorkerRunLoop() notification is sent to the Chromium side.
> (4) ThreadState::detach() is called.
> (5) Isolate is disposed.

This code is one big hair ball already where things have to happen in exactly
the right order for things to die properly. I'm not sure this is much worse than
it was before? Maybe some comments on the shutdown order would be in order.

https://codereview.chromium.org/218953002/diff/20001/Source/modules/indexeddb...
File Source/modules/indexeddb/IDBRequest.cpp (right):

https://codereview.chromium.org/218953002/diff/20001/Source/modules/indexeddb...
Source/modules/indexeddb/IDBRequest.cpp:374: checkForReferenceCycle();
On 2014/04/01 05:56:14, haraken wrote:
> 
> I need to reconsider what I should do here. Currently this code results in an
> infinite loop in some layout tests.

What we want to ensure in this call is that any cursors are detached from the
cursor backend before the dispatcher goes away.

All of these objects for this worker thread are going to die now, so as far as I
can tell, what you need to do here is m_backend.clear() on the IDBCursor object.

That will detach the IDBCursor from the backend while the dispatcher is still
alive. Then we will kill the dispatcher. Then we GC and that will kill these
objects which is now safe since we have detached from the backend before the
dispatcher got killed?

[Mads Ager (chromium), 2014-04-01 11:43:00]

On 2014/04/01 06:08:03, haraken wrote:
> > performing a full garbage collection on all threads in order to invalidate
the
> > (probably very few if any) IDBCursors on this thread seems very heavy
weight.
> 
> I agree that it's not a good idea to trigger a full GC just for collecting IDB
> objects, but the performance impact of doing a full GC might not be as bad as
we
> imagine. Given that we need to cause a full GC in ThreadState::detach(), we
> anyway need to stop the main thread when shutting down a worker thread. If we
> anyway need to stop the main thread, it won't matter if we trigger one full GC
> or two full GCs (the second full GC will be very light since there will be few
> objects to collect).

Unfortunately, that is not the way it works. :)

We are performing mark/sweep garbage collection, so the garbage collection
complexity is linear in the size of the entire heap, not in the amount of live
data or in the amount of dead data. So garbage collections with a large heap is
going to be costly and we don't want to force it more than we have to here.

[haraken, 2014-04-01 14:38:05]

On 2014/04/01 11:38:37, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/218953002/diff/20001/Source/core/workers/Work...
> File Source/core/workers/WorkerGlobalScope.cpp (right):
> 
>
https://codereview.chromium.org/218953002/diff/20001/Source/core/workers/Work...
> Source/core/workers/WorkerGlobalScope.cpp:203: clearScript();
> On 2014/04/01 05:56:14, haraken wrote:
> > 
> > I'm afraid this is very hacky. This code is the way it is to make sure that
> > things happen in the following order:
> > 
> > (1) DOMWrapperWorld is disposed.
> > (2) contextWillBeDestroyed() is called on IDBRequest objects, and they die.
> > (3) didStopWorkerRunLoop() notification is sent to the Chromium side.
> > (4) ThreadState::detach() is called.
> > (5) Isolate is disposed.
> 
> This code is one big hair ball already where things have to happen in exactly
> the right order for things to die properly. I'm not sure this is much worse
than
> it was before? Maybe some comments on the shutdown order would be in order.

Yeah. This CL is slightly making the situation worse in that it is introducing
contextWillBeDestroyed().

I think we need to rethink about the destruction dependency of when shutting
down a worker thread. There we have a lot of tricks:

- didStartWorkerRunLoop is called in workerThread(), but didStopWorkerRunLoop is
called in ~WorkerScriptController().
- WorkerGlobalScope::dispose() cannot clear WorkerGlobalScope::m_thread.
- LifecycleObserver::contextDestroyed() of DOM objects created from a worker
thread is invoked after the WorkerScriptController is destructed. (We can do
nothing in LifecycleObserver::contextDestroyed().)
- ...

Let me take a detailed look tomorrow.

>
https://codereview.chromium.org/218953002/diff/20001/Source/modules/indexeddb...
> File Source/modules/indexeddb/IDBRequest.cpp (right):
> 
>
https://codereview.chromium.org/218953002/diff/20001/Source/modules/indexeddb...
> Source/modules/indexeddb/IDBRequest.cpp:374: checkForReferenceCycle();
> On 2014/04/01 05:56:14, haraken wrote:
> > 
> > I need to reconsider what I should do here. Currently this code results in
an
> > infinite loop in some layout tests.
> 
> What we want to ensure in this call is that any cursors are detached from the
> cursor backend before the dispatcher goes away.
> 
> All of these objects for this worker thread are going to die now, so as far as
I
> can tell, what you need to do here is m_backend.clear() on the IDBCursor
object.
> 
> That will detach the IDBCursor from the backend while the dispatcher is still
> alive. Then we will kill the dispatcher. Then we GC and that will kill these
> objects which is now safe since we have detached from the backend before the
> dispatcher got killed?

Yeah, I think you're right. It looks like I also need to clear a couple of
RefPtrs to IDB objects (e.g., IDBRequest::m_result, IDBRequest::m_source etc).

> We are performing mark/sweep garbage collection, so the garbage collection
> complexity is linear in the size of the entire heap, not in the amount of live
> data or in the amount of dead data. So garbage collections with a large heap
is
> going to be costly and we don't want to force it more than we have to here.

ah, understood. Thanks for the clarification.

[haraken, 2014-04-01 14:54:15]

+sof who knows a lot about worker thread destruction :)

[sof, 2014-04-01 16:07:46]

On 2014/04/01 14:54:15, haraken wrote:
> +sof who knows a lot about worker thread destruction :)

I didn't see this explained/discussed, but could the effect of calling
clearScript() (i.e., invoke didStopWorkerRunLoop()) be delayed until after
ThreadState::detach() ? Have it be moved into WorkerThread::workerThread() just
like the call to workerGlobalScopeDestroyed() ended up being.

[sof, 2014-04-01 20:08:54]

On 2014/04/01 16:07:46, sof wrote:
> On 2014/04/01 14:54:15, haraken wrote:
> > +sof who knows a lot about worker thread destruction :)
> 
> I didn't see this explained/discussed, but could the effect of calling
> clearScript() (i.e., invoke didStopWorkerRunLoop()) be delayed until after
> ThreadState::detach() ? Have it be moved into WorkerThread::workerThread()
just
> like the call to workerGlobalScopeDestroyed() ended up being.

No, that wouldn't work or solve anything.

[haraken, 2014-04-02 01:31:04]

On 2014/04/01 20:08:54, sof wrote:
> On 2014/04/01 16:07:46, sof wrote:
> > On 2014/04/01 14:54:15, haraken wrote:
> > > +sof who knows a lot about worker thread destruction :)
> > 
> > I didn't see this explained/discussed, but could the effect of calling
> > clearScript() (i.e., invoke didStopWorkerRunLoop()) be delayed until after
> > ThreadState::detach() ? Have it be moved into WorkerThread::workerThread()
> just
> > like the call to workerGlobalScopeDestroyed() ended up being.
> 
> No, that wouldn't work or solve anything.

Thanks sof for the input. The destruction dependency in worker threads are very
complicated. I'm trying to draw a diagram. (I welcome your idea:-)

[haraken, 2014-04-11 01:28:26]

mads@: Updated a CL. I think a right fix to this issue would be just to clear
IDBRequest::m_result when the IDBRequest object is stopped. See the CL
description for more details. PTAL.

jsbell@: I'd be happy if you could also check if this is a right thing to do :)

[haraken, 2014-04-11 01:29:23]

I confirmed that this CL fixes all flaky crashes in IDB tests in oilpan builds.

[sof, 2014-04-11 06:24:39]

That cuts a knot. I cannot see an unsafe use of m_result that would result from
it.

[Mads Ager (chromium), 2014-04-11 06:32:03]

On 2014/04/11 01:28:26, haraken wrote:
> mads@: Updated a CL. I think a right fix to this issue would be just to clear
> IDBRequest::m_result when the IDBRequest object is stopped. See the CL
> description for more details. PTAL.
> 
> jsbell@: I'd be happy if you could also check if this is a right thing to do
:)

That is indeed a lot simpler and makes sense to me. Let's see what jsbell
thinks. :)

[jsbell, 2014-04-11 16:34:21]

lgtm

I'm glad I was in meetings and didn't see the churn on this. :)  Yeah, the only
non-obvious use of m_result is in the getResultCursor() which is primarily used
when attempting to break the cursor/request RefPtr cycle when wrappers are GC'd.
The only gotcha would be if clearing m_result let the cursor be freed (and it's
not an ActiveDOMObject, so that's fine) which dropped the final reference to the
request out from under the caller, but by definition it's got a ref. So... all
good. Thanks!

[haraken, 2014-04-11 16:38:59]

On 2014/04/11 16:34:21, jsbell wrote:
> lgtm
> 
> I'm glad I was in meetings and didn't see the churn on this. :)  Yeah, the
only
> non-obvious use of m_result is in the getResultCursor() which is primarily
used
> when attempting to break the cursor/request RefPtr cycle when wrappers are
GC'd.
> The only gotcha would be if clearing m_result let the cursor be freed (and
it's
> not an ActiveDOMObject, so that's fine) which dropped the final reference to
the
> request out from under the caller, but by definition it's got a ref. So... all
> good. Thanks!

Thanks for review!

[haraken, 2014-04-11 16:39:04]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-04-11 16:39:15]

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

[commit-bot: I haz the power, 2014-04-11 17:11:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-11 17:12:00]

Try jobs failed on following builders:
  tryserver.blink on linux_blink_rel

[haraken, 2014-04-14 04:04:21]

hmm, the CL breaks a couple of IDB tests...

The issue is as follows:

(1) ContextLifecycleNotifier::notifyStoppingActiveDOMObjects() iterates a set of
ActiveDOMObjects and calls stop() for them.
(2) IDBRequest::stop() clears IDBRequest::m_result.
(3) Clearing IDBRequest::m_result triggers ~IDBDatabase().
(4) Because IDBDatabase is an ActiveDOMObject, ~IDBDatabase() tries to remove
itself from the set of ActiveDOMObjects. This is not allowed because
modification to the set confuses the iteration that ContextLifecycleNotifier is
currently doing. So we are asserted.

We've hit the same issue before when fixing a bug of XHR. I think a right fix
would be to improve ActiveDOMObject's architecture so that the iteration can
tolerate modification during the iteration. I'll take a look if it's doable.

[jsbell, 2014-08-11 22:52:31]

What's the status of this CL? I seem to recall that some work was done to
handling modification of the ActiveDOMObject list during iteration, but did
enough happen to unblock this change, and/or did this get rethought?

[haraken, 2014-08-12 01:04:19]

On 2014/08/11 22:52:31, jsbell wrote:
> What's the status of this CL? I seem to recall that some work was done to
> handling modification of the ActiveDOMObject list during iteration, but did
> enough happen to unblock this change, and/or did this get rethought?

This issue is gone, since IDBRequest::checkForReferenceCycle() is gone when
Oilpan was enabled by default for indexeddb/.

[haraken, 2014-08-12 01:04:53]

On 2014/08/12 01:04:19, haraken wrote:
> On 2014/08/11 22:52:31, jsbell wrote:
> > What's the status of this CL? I seem to recall that some work was done to
> > handling modification of the ActiveDOMObject list during iteration, but did
> > enough happen to unblock this change, and/or did this get rethought?
> 
> This issue is gone, since IDBRequest::checkForReferenceCycle() is gone when
> Oilpan was enabled by default for indexeddb/.

Closing the issue.