Deactivate IDBTransactions created within Microtasks

Deactivate IDBTransactions created within Microtasks

When microtasks are run outside the event loop (i.e. they are resolved
from C++ not from JS), wrap the execution in a V8RecursionScope to
ensure that IDBTransactions are deactivated.

R=adamk@chromium.org,haraken@chromium.org
BUG=390704, 411532
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=182468

[jsbell, 2014-09-10 21:41:48]

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

[jsbell, 2014-09-10 21:41:49]

Prospective chain of patches is:

1. https://codereview.chromium.org/555633002 - ExecutionContext->ScriptState in
IDBTransaction ctor
2. https://codereview.chromium.org/429453010 - drop ExecutionContext dependency
from V8RecursionScope
3. https://codereview.chromium.org/555163005 (this patch) - wrap Microtask
execution with V8RecursionScope

haraken@, adamk@ - please take a look?

Biggest questions/issues are in #2.

[haraken, 2014-09-11 01:22:21]

On 2014/09/10 21:41:49, jsbell wrote:
> Prospective chain of patches is:
> 
> 1. https://codereview.chromium.org/555633002 - ExecutionContext->ScriptState
in
> IDBTransaction ctor
> 2. https://codereview.chromium.org/429453010 - drop ExecutionContext
dependency
> from V8RecursionScope
> 3. https://codereview.chromium.org/555163005 (this patch) - wrap Microtask
> execution with V8RecursionScope
> 
> haraken@, adamk@ - please take a look?
> 
> Biggest questions/issues are in #2.

As mentioned in another thread, the 1 and 2 look good. I'd like to defer the
decision for 3 to adamk@.

[adamk, 2014-09-12 18:50:12]

lgtm

[jsbell, 2014-09-13 00:07:58]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-09-13 00:08:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555163005/1

[commit-bot: I haz the power, 2014-09-13 00:48:51]

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

[commit-bot: I haz the power, 2014-09-13 00:48:52]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[jsbell, 2014-09-13 04:18:40]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-09-13 04:19:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/555163005/1

[commit-bot: I haz the power, 2014-09-13 04:59:52]

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

[commit-bot: I haz the power, 2014-09-13 04:59:53]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[jsbell, 2014-09-15 17:57:57]

Bleah. The twiddling around with #2 exposed a shutdown ordering issue here and I
didn't re-test the combined patches. :(

What's happening: 
* At some point AFTER we call V8PerIsolateData::willBeDestroyed() a Task is
processed - probably a shutdown task.
* MicrotaskRunner::didProcessTask() is called, which calls
Microtask::performCheckpoint()
* c/o this CL, that results in a V8RecursionScope being created
* that calls V8PerIsolateData::ensureIDBPendingTransactionMonitor(), which
creates one
* Oilpan (correctly) says "nope nope nope nope" since that's got a heap handle

One fix would be to move the V8PerIsolateData::willBeDestroyed() into
WorkerThread::cleanup() as discussed in #2, but that involves passing the
Isolate* along in the closure. IMHO a better approach would be to leave that
alone and key off of something to early-exit from the above chain since it's not
useful work. Alternately we could remove the task observer earlier - after
WorkerThread::stopInitialize() but before WorkerThread::cleanup() - but doing
that within one of the shutdown tasks looks messy.

To kick off the discussion (and try bots), I've uploaded a CL that adds a
"pendingDestruction" flag to the V8PerIsolateData. I haven't come up with
anything cleaner but I'm not thrilled by it so alternate suggestions welcome.

[jsbell, 2014-09-16 17:09:04]

@haraken - can you provide input on the most recent change/comment?

(Sorry, I should have pinged you specifically before.)

And per try bots it looks like the inspector/debugger's view of microtasks
changed. Wheee. That's presumably related to the new use of V8RecursionScope,
not shutdown. I'll dig in.

[jsbell, 2014-09-16 22:58:07]

jsbell@chromium.org changed reviewers:
+ aandrey@chromium.org, yurys@chromium.org

[jsbell, 2014-09-16 22:58:07]

Re: the async/debugger failures

It looks like the introduction of a V8RecursionScope inside
Microtask::performCheckpoint() bumps the recursion level by 1, which breaks the
check in AsyncCallStackTracker::setCurrentAsyncCallChain(), so various async
stacks don't get reported.

+aandrey, +yurys, in case either of them see an obvious fix.

Still trying to wrap my head around this...

[jsbell, 2014-09-19 23:54:27]

Theory: we can simply do the "end-of-task-or-microtask" work towards the end of
Microtask::performCheckpoint() without using V8RecursionScope:

* If we're calling performCheckpoint() then we're doing a checkpoint due to C++
behavior not script behavior, and we're calling into script. So we will want to
do the end-of-task-or-microtask work anyway.

* We weren't previously using a V8RecursionScope, so it must be correct that we
don't need to add one here EXCEPT to get the end-of-task-or-microtask behavior.

I uploaded a hack to see what the try bots say. Assuming that looks good, I'll
try and come up with a sensible refactor.

[jsbell, 2014-09-22 02:31:15]

On 2014/09/19 23:54:27, jsbell wrote:
> Theory: we can simply do the "end-of-task-or-microtask" work towards the end
of
> Microtask::performCheckpoint() without using V8RecursionScope:
> 
> * If we're calling performCheckpoint() then we're doing a checkpoint due to
C++
> behavior not script behavior, and we're calling into script. So we will want
to
> do the end-of-task-or-microtask work anyway.
> 
> * We weren't previously using a V8RecursionScope, so it must be correct that
we
> don't need to add one here EXCEPT to get the end-of-task-or-microtask
behavior.
> 
> I uploaded a hack to see what the try bots say. Assuming that looks good, I'll
> try and come up with a sensible refactor.

Ugh, nope. We need the scope so that console logging doesn't trigger end-of-task
(ie the bug this is fixing)

I'll try (ie look at the code to see if it sensible) to alter the async stack
collection to capture if depth 0 or depth 1 and running microtasks.

[jsbell, 2014-09-22 17:24:12]

On 2014/09/22 02:31:15, jsbell wrote:
> I'll try (ie look at the code to see if it sensible) to alter the async stack
> collection to capture if depth 0 or depth 1 and running microtasks.

That seems to work and I think I've convinced myself it's correct.

yurys@, aandrey@, adamk@ - please take a look?

I'm not thrilled with pushing all of this knowledge into the
AsyncCallStackTracker, but it's already aware of the recursion count and since
it's dealing with async stacks having it be aware of microtasks is not
unreasonable. Suggestions welcome.

[jsbell, 2014-09-22 17:26:29]

https://codereview.chromium.org/555163005/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/555163005/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:421: if (chain &&
(!V8RecursionScope::recursionLevel(toIsolate(context)) ||
(V8RecursionScope::recursionLevel(toIsolate(context)) == 1 &&
Microtask::performingCheckpoint(toIsolate(context))))) {
That's a lot of calls to toIsolate(context).

I could pull `v8::Isolate* isolate = toIsolate(context);` out above the if(),
but I'm not sure if this is called frequently with an empty chain and/or how
convoluted we want to make this method to maintain performance - or if that's at
all important.

[adamk, 2014-09-22 18:30:58]

I'll leave it to aandrey/yurys to comment on whether it's reasonable for
AsyncCallStackTracker to have all this logic in it. I agree that it seems
correct.

https://codereview.chromium.org/555163005/diff/80001/Source/core/dom/Microtas...
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/555163005/diff/80001/Source/core/dom/Microtas...
Source/core/dom/Microtask.cpp:55:
ModuleProxy::moduleProxy().didLeaveScriptContextForRecursionScope(isolate);
I'm not familiar with this thing. Can you explain why this isn't part of the
work V8RecursionScope does?

https://codereview.chromium.org/555163005/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/555163005/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:421: if (chain &&
(!V8RecursionScope::recursionLevel(toIsolate(context)) ||
(V8RecursionScope::recursionLevel(toIsolate(context)) == 1 &&
Microtask::performingCheckpoint(toIsolate(context))))) {
On 2014/09/22 at 17:26:29, jsbell wrote:
> That's a lot of calls to toIsolate(context).
> 
> I could pull `v8::Isolate* isolate = toIsolate(context);` out above the if(),
but I'm not sure if this is called frequently with an empty chain and/or how
convoluted we want to make this method to maintain performance - or if that's at
all important.

I'd recommend pulling it out just to clean up the code.

[jsbell, 2014-09-22 18:49:51]

Thanks for catching that!

https://codereview.chromium.org/555163005/diff/80001/Source/core/dom/Microtas...
File Source/core/dom/Microtask.cpp (right):

https://codereview.chromium.org/555163005/diff/80001/Source/core/dom/Microtas...
Source/core/dom/Microtask.cpp:55:
ModuleProxy::moduleProxy().didLeaveScriptContextForRecursionScope(isolate);
On 2014/09/22 18:30:57, adamk wrote:
> I'm not familiar with this thing. Can you explain why this isn't part of the
> work V8RecursionScope does?

Whoops, that's accidentally left in from an earlier experiment. That is what
V8RecursionScope does in its dtor, so it's completely redundant here.

https://codereview.chromium.org/555163005/diff/80001/Source/core/inspector/As...
File Source/core/inspector/AsyncCallStackTracker.cpp (right):

https://codereview.chromium.org/555163005/diff/80001/Source/core/inspector/As...
Source/core/inspector/AsyncCallStackTracker.cpp:421: if (chain &&
(!V8RecursionScope::recursionLevel(toIsolate(context)) ||
(V8RecursionScope::recursionLevel(toIsolate(context)) == 1 &&
Microtask::performingCheckpoint(toIsolate(context))))) {
On 2014/09/22 18:30:57, adamk wrote:
> On 2014/09/22 at 17:26:29, jsbell wrote:
> > That's a lot of calls to toIsolate(context).
> > 
> > I could pull `v8::Isolate* isolate = toIsolate(context);` out above the
if(),
> but I'm not sure if this is called frequently with an empty chain and/or how
> convoluted we want to make this method to maintain performance - or if that's
at
> all important.
> 
> I'd recommend pulling it out just to clean up the code.

Done.

[aandrey, 2014-09-23 05:47:21]

lgtm

[jsbell, 2014-09-23 05:47:59]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2014-09-23 05:48:36]

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

[commit-bot: I haz the power, 2014-09-23 06:12:04]

Committed patchset #6 (id:100001) as 182468