[ABANDONED] Call Microtask::performCheckpoint for each worker task.

Call Microtask::performCheckpoint for each worker task.

In order to run Promise microtasks in workers, we need to call
Microtask::performCheckpoint at the end of each task.
This CL does that.

BUG=352552
R=adamk@chromium.org

[adamk, 2014-03-31 15:27:36]

As I noted in the bug the call to performCheckpoint seems reasonable to me, but
I'd like to understand more about why the ensureDomInJsContext() is necessary
before landing this. The FIXME sounds very mysterious, and makes me worry that
V8 is in a bad state when we call into it in those cases and the early
ensureDomInJsContext() call is just masking some underlying issue.

https://codereview.chromium.org/219003002/diff/20001/Source/bindings/v8/Worke...
File Source/bindings/v8/WorkerScriptController.cpp (right):

https://codereview.chromium.org/219003002/diff/20001/Source/bindings/v8/Worke...
Source/bindings/v8/WorkerScriptController.cpp:74: // FIXME: Calling
v8::Context::New in ensureDomInJsContext() later may
What circumstances?  What do the crashes look like?

[adamk, 2014-03-31 16:17:26]

Adding rafaelw

[yhirano, 2014-04-01 02:29:12]

On 2014/03/31 15:27:36, adamk wrote:
> As I noted in the bug the call to performCheckpoint seems reasonable to me,
but
> I'd like to understand more about why the ensureDomInJsContext() is necessary
> before landing this. The FIXME sounds very mysterious, and makes me worry that
> V8 is in a bad state when we call into it in those cases and the early
> ensureDomInJsContext() call is just masking some underlying issue.
> 
fast/workers/worker-terminate.html crashes, for example. Please see [1].
It's because WorkerThread::stop is called inside v8::Context::New in
V8PerIsolateDate::ensureDomInJSContext in such test cases.
I don't understand why it occurs.
v8::Context::New returns an empty handle and accessing to it leads to a crash.

> makes me worry that V8 is in a bad state
I tried to detect such "bad" signs and stop calling performCheckpoint, but
failed.

1.
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/18...

[adamk, 2014-04-01 21:59:38]

On 2014/04/01 02:29:12, yhirano wrote:
> On 2014/03/31 15:27:36, adamk wrote:
> > As I noted in the bug the call to performCheckpoint seems reasonable to me,
> but
> > I'd like to understand more about why the ensureDomInJsContext() is
necessary
> > before landing this. The FIXME sounds very mysterious, and makes me worry
that
> > V8 is in a bad state when we call into it in those cases and the early
> > ensureDomInJsContext() call is just masking some underlying issue.
> > 
> fast/workers/worker-terminate.html crashes, for example. Please see [1].
> It's because WorkerThread::stop is called inside v8::Context::New in
> V8PerIsolateDate::ensureDomInJSContext in such test cases.

Can you elaborate? How could WorkerThread::stop be called _inside_
v8::Context::New? Or do you mean the other way around?

> I don't understand why it occurs.
> v8::Context::New returns an empty handle and accessing to it leads to a crash.
>
> > makes me worry that V8 is in a bad state
> I tried to detect such "bad" signs and stop calling performCheckpoint, but
> failed.
> 
> 1.
>
https://storage.googleapis.com/chromium-layout-test-archives/mac_blink_rel/18...

Looking at that error message I see "Error installing extension 'v8/gc'". The v8
code that prints that is in v8/src/bootstrapper.cc (near line 2367), and the
very next thing that happens is that Genesis::InstallExtension will return
false, causing InstallExtensions() to return false, which causes
Bootstrapper::CreateEnvironment() to return an empty handle.

So for some reason it's the failure to install the "gc" extension that's causing
this to crash. The interesting question, I guess, is what's causing the failure
of the gc extension. This could be further tracked down by adding debugging to
figure out when Genesis::CompileScriptCached() to fail when processing the gc
extension. But I think it's important to figure out what's going wrong here,
since I think it's possible that the problem is that V8 is in some bad state
when trying to compile the extension, and that something similar might be
triggered if there were actually any work queued in V8 when we call
Microtask::performCheckpoint() in this state.

[yhirano, 2014-04-04 04:23:10]

> Looking at that error message I see "Error installing extension 'v8/gc'". The
v8
> code that prints that is in v8/src/bootstrapper.cc (near line 2367), and the
> very next thing that happens is that Genesis::InstallExtension will return
> false, causing InstallExtensions() to return false, which causes
> Bootstrapper::CreateEnvironment() to return an empty handle.
>
It's because v8::V8::TerminateExecution is called from **the main thread** in
WorkerScriptController::scheduleExecutionTermination.
I can't detect it reliably from the worker thread, as it happens completely
asynchronously.
I'm wondering why scheduleExecutionTermination actually terminates the execution
rather than scheduling it. Do you know why?

[yhirano, 2014-04-04 11:58:33]

One more thing: Microtask::performCheckPoint uses
V8PerIsolateData::ensureDomInJSContext and it leads to a crash.
A V8PerIsolateData has a V8PerContextData which is initialized in
ensureDomInJSContext.
But a WorkerScriptController has a V8PerContextData, too.
They should be unified, I think.

[yhirano, 2014-04-04 12:07:05]

On 2014/04/04 11:58:33, yhirano wrote:
> One more thing: Microtask::performCheckPoint uses
> V8PerIsolateData::ensureDomInJSContext and it leads to a crash.
> A V8PerIsolateData has a V8PerContextData which is initialized in
> ensureDomInJSContext.
> But a WorkerScriptController has a V8PerContextData, too.
> They should be unified, I think.

Sorry, this was wrong.

[adamk, 2014-04-04 22:36:35]

On 2014/04/04 04:23:10, yhirano wrote:
> > Looking at that error message I see "Error installing extension 'v8/gc'".
The
> v8
> > code that prints that is in v8/src/bootstrapper.cc (near line 2367), and the
> > very next thing that happens is that Genesis::InstallExtension will return
> > false, causing InstallExtensions() to return false, which causes
> > Bootstrapper::CreateEnvironment() to return an empty handle.
> >
> It's because v8::V8::TerminateExecution is called from **the main thread** in
> WorkerScriptController::scheduleExecutionTermination.
> I can't detect it reliably from the worker thread, as it happens completely
> asynchronously.

I'm not clear on why you can't use
WorkerScriptController::isExecutionTerminating(). It seems like guarding
the performCheckpoint() call with that would be sufficient.

> I'm wondering why scheduleExecutionTermination actually terminates the
execution
> rather than scheduling it. Do you know why?

I don't know the history here, but perhaps it's using "schedule" in the method
name because of what V8 actually does for this case (sets a bit that causes the
isolate to throw a special exception).

[yhirano, 2014-04-07 01:54:24]

On 2014/04/04 22:36:35, adamk wrote:
> On 2014/04/04 04:23:10, yhirano wrote:
> > > Looking at that error message I see "Error installing extension 'v8/gc'".
> The
> > v8
> > > code that prints that is in v8/src/bootstrapper.cc (near line 2367), and
the
> > > very next thing that happens is that Genesis::InstallExtension will return
> > > false, causing InstallExtensions() to return false, which causes
> > > Bootstrapper::CreateEnvironment() to return an empty handle.
> > >
> > It's because v8::V8::TerminateExecution is called from **the main thread**
in
> > WorkerScriptController::scheduleExecutionTermination.
> > I can't detect it reliably from the worker thread, as it happens completely
> > asynchronously.
> 
> I'm not clear on why you can't use
> WorkerScriptController::isExecutionTerminating(). It seems like guarding
> the performCheckpoint() call with that would be sufficient.
> 
Sorry I still don't understand.
We can check WorkerScriptController::isExecutionTerminating(), but we can't
forbid the main thread from calling
WorkerScriptController::scheduleExecutionTermination.
In other words, what happens when the following events occur sequentially?

1. The worker thread checks WorkerScriptController::isExecutionTerminating() and
it returns false
2. The main thread calls WorkerScriptController::scheduleExecutionTermination
3. The worker thread calls ensureDomInJSContext (in
Microtask::performCheckpoint)

I think it's possible and it leads to a crash.

[yhirano, 2014-04-07 03:20:24]

> We can check WorkerScriptController::isExecutionTerminating(), but we can't
> forbid the main thread from calling
> WorkerScriptController::scheduleExecutionTermination.
> In other words, what happens when the following events occur sequentially?
> 
> 1. The worker thread checks WorkerScriptController::isExecutionTerminating()
and
> it returns false
> 2. The main thread calls WorkerScriptController::scheduleExecutionTermination
> 3. The worker thread calls ensureDomInJSContext (in
> Microtask::performCheckpoint)
> 
> I think it's possible and it leads to a crash.

A possible solution is to detect the termination after the fact, as PS5 does.

[jochen (gone - plz use gerrit), 2014-04-07 13:06:35]

https://codereview.chromium.org/219003002/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerRunLoop.cpp (right):

https://codereview.chromium.org/219003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerRunLoop.cpp:62: Microtask::performCheckpoint();
for debugger tasks, this will be executed twice: we post a task to the debugger
loop, and a tickle task to the regular loop.

will that cause problems?

[adamk, 2014-04-07 22:52:49]

Thank you very much for the detailed explanation of this termination case, and
for exploring a few different approaches to fixing it. It's now clear to me that
after V8::TerminateExecution() has been called, V8 is in a very strange state,
where APIs that could otherwise never return an empty handle now may do so. In
fact, after looking at PS5, I'm tempted to say that the original approach of
eagerly-allocating the DOM-in-JS context might be cleaner, since it'll always be
created the first time through the runloop anyway (though the approach of always
checking the return value of ensureDOMInJsContext() is semantically correct as
well, given V8's behavior).

In the meantime, however, while investigating http://crbug.com/360426, rafaelw
found a deeper problem with using the V8 microtask queue:
http://crbug.com/360891. I think we'll want to find the solution to that bug
before moving further forward with using V8 Promises from Blink.

[yhirano, 2014-04-08 07:35:13]

https://codereview.chromium.org/219003002/diff/80001/Source/core/workers/Work...
File Source/core/workers/WorkerRunLoop.cpp (right):

https://codereview.chromium.org/219003002/diff/80001/Source/core/workers/Work...
Source/core/workers/WorkerRunLoop.cpp:62: Microtask::performCheckpoint();
On 2014/04/07 13:06:36, jochen wrote:
> for debugger tasks, this will be executed twice: we post a task to the
debugger
> loop, and a tickle task to the regular loop.
> 
> will that cause problems?
I think it won't, but I may misunderstand the situation. Can you describe it a
bit more concretely?

[jochen (gone - plz use gerrit), 2014-04-08 07:48:57]

https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
File Source/core/workers/WorkerRunLoop.cpp (right):

https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
Source/core/workers/WorkerRunLoop.cpp:276: bool posted =
m_debuggerMessageQueue.append(WorkerRunLoopTask::create(*this, task));
here, a debugger task is queued in a separate message queue.

https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
Source/core/workers/WorkerRunLoop.cpp:278:
postTask(TickleDebuggerQueueTask::create(this));
and here, we post a tickle debugger queue task on the regular worker run loop
that will execute one task from the debugger message queue

The tickle task will synchronously execute a debugger task.

Both are WorkerRunLoopTasks, and so the checkpoint is executed twice at the end
of the task

[yhirano, 2014-04-09 05:39:02]

https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
File Source/core/workers/WorkerRunLoop.cpp (right):

https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
Source/core/workers/WorkerRunLoop.cpp:278:
postTask(TickleDebuggerQueueTask::create(this));
> Both are WorkerRunLoopTasks, and so the checkpoint is executed twice at the
end
> of the task

TickleDebuggerQueueTask inherits ExecutionContextTask, not WorkerRunLoopTask.

[jochen (gone - plz use gerrit), 2014-04-09 07:38:47]

On 2014/04/09 05:39:02, yhirano wrote:
>
https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
> File Source/core/workers/WorkerRunLoop.cpp (right):
> 
>
https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
> Source/core/workers/WorkerRunLoop.cpp:278:
> postTask(TickleDebuggerQueueTask::create(this));
> > Both are WorkerRunLoopTasks, and so the checkpoint is executed twice at the
> end
> > of the task
> 
> TickleDebuggerQueueTask inherits ExecutionContextTask, not WorkerRunLoopTask.

Right, but the postTask will wrap it in a WorkerRunLoopTask

[yhirano, 2014-04-09 08:09:30]

On 2014/04/09 07:38:47, jochen wrote:
> On 2014/04/09 05:39:02, yhirano wrote:
> >
>
https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
> > File Source/core/workers/WorkerRunLoop.cpp (right):
> > 
> >
>
https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
> > Source/core/workers/WorkerRunLoop.cpp:278:
> > postTask(TickleDebuggerQueueTask::create(this));
> > > Both are WorkerRunLoopTasks, and so the checkpoint is executed twice at
the
> > end
> > > of the task
> > 
> > TickleDebuggerQueueTask inherits ExecutionContextTask, not
WorkerRunLoopTask.
> 
> Right, but the postTask will wrap it in a WorkerRunLoopTask
Oh, I see, thank you.
Then, there are many WorkerRunLoopTasks, namely:
task: the given task (in postDebuggerTask) which is pushed on the debugger
queue.
task1: pop one task (task2) from the debugger queue and run it
task2: a task in the debugger queue, run by task1

task == task2 doesn't always hold, does it?

The execution might be:
  task1->run
   task1->m_task->performTask
    loop->runDebuggerTask
     task2->run
      task2->m_task->performTask
      Microtask::performCheckpoint
   Microtask::performCheckpoint

There seems no problem.
Because Microtask::performCheckpoint consumes all pending microtasks and task1
merely executes task2, the inner performCheckpoint executes microtasks posted by
task2 and the outer performCheckpoint does nothing.

[jochen (gone - plz use gerrit), 2014-04-10 07:54:16]

On 2014/04/09 08:09:30, yhirano wrote:
> On 2014/04/09 07:38:47, jochen wrote:
> > On 2014/04/09 05:39:02, yhirano wrote:
> > >
> >
>
https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
> > > File Source/core/workers/WorkerRunLoop.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/219003002/diff/90001/Source/core/workers/Work...
> > > Source/core/workers/WorkerRunLoop.cpp:278:
> > > postTask(TickleDebuggerQueueTask::create(this));
> > > > Both are WorkerRunLoopTasks, and so the checkpoint is executed twice at
> the
> > > end
> > > > of the task
> > > 
> > > TickleDebuggerQueueTask inherits ExecutionContextTask, not
> WorkerRunLoopTask.
> > 
> > Right, but the postTask will wrap it in a WorkerRunLoopTask
> Oh, I see, thank you.
> Then, there are many WorkerRunLoopTasks, namely:
> task: the given task (in postDebuggerTask) which is pushed on the debugger
> queue.
> task1: pop one task (task2) from the debugger queue and run it
> task2: a task in the debugger queue, run by task1
> 
> task == task2 doesn't always hold, does it?

correct

> 
> The execution might be:
>   task1->run
>    task1->m_task->performTask
>     loop->runDebuggerTask
>      task2->run
>       task2->m_task->performTask
>       Microtask::performCheckpoint
>    Microtask::performCheckpoint
> 
> There seems no problem.
> Because Microtask::performCheckpoint consumes all pending microtasks and task1
> merely executes task2, the inner performCheckpoint executes microtasks posted
by
> task2 and the outer performCheckpoint does nothing.

great