[ServiceWorker] Add NULL check of ExecutionContext in FetchBodyStream::readAsync

[ServiceWorker] Add NULL check of ExecutionContext in FetchBodyStream::readAsync

scriptState->executionContext() returns NULL in the following situation.

[Main thread]
- WorkerThread::stop()
 - m_workerGlobalScope->script()->scheduleExecutionTermination();
  - v8::V8::TerminateExecution(m_isolate);

[Worker thread]
- FetchBodyStream::readAsync()
 - ScriptPromiseResolver::create()
  ---> v8::Promise::Resolver::New()
     - v8::internal::Execution::Call
      - v8::internal::Invoke
       - JS
        - v8::internal::__RT_impl_Runtime_StackGuard
         - v8::internal::Isolate::TerminateExecution()
            Sets TerminationException.
 - scriptState->executionContext()
  - toExecutionContext()
   - V8WorkerGlobalScope::findInstanceInPrototypeChain()
    - V8PerIsolateData::findInstanceInPrototypeChain()
     - V8PerIsolateData::findInstanceInPrototypeChain()
      - v8::Object::FindInstanceInPrototypeChain()
       - ON_BAILOUT(isolate,
                    "v8::Object::FindInstanceInPrototypeChain()",
                    return Local<v8::Object>());
        - IsExecutionTerminatingCheck()
          Returns true.

BUG=409755
TEST=run_webkit_tests http/tests/serviceworker/request.html

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

[horo, 2014-09-03 15:56:43]

horo@chromium.org changed reviewers:
+ yhirano@chromium.org

[horo, 2014-09-03 15:56:44]

yhirano@
Could you please review?

[yhirano, 2014-09-04 04:34:27]

Does that mean every V8 code execution on workerthread could clear scriptState?
Is this the only case?

[horo, 2014-09-04 04:58:46]

> Does that mean every V8 code execution on workerthread could clear
scriptState?
I'm not familiar with V8, but I think so.
Is this unexpected behavior?

[yhirano, 2014-09-04 07:31:09]

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

[yhirano, 2014-09-04 07:32:10]

On 2014/09/04 04:58:46, horo wrote:
> > Does that mean every V8 code execution on workerthread could clear
> scriptState?
> I'm not familiar with V8, but I think so.
> Is this unexpected behavior?
No, I should have noticed.
I grepped Resolver::create in modules/serviceworkers.
ServiceWorkerClients::getAll may have the same problem.

I don't like inserting |if (scriptState->executionContext())| in many places,
but I don't have a good idea.
+haraken who may have a better idea.

[haraken, 2014-09-04 08:13:03]

https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchBodyStream.cpp (right):

https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchBodyStream.cpp:30: if (m_hasRead)

I think you should check m_scriptState->contextIsEmpty() here.

We normally follow the following programming rule:

class A {
  A(ScriptState* scriptState) : m_scriptState(scriptState) { }

  void someMethodTriggeredAsynchronously() {
    if (m_scriptState->contextIsEmpty())
      return;
    ScriptState::Scope scope(m_scriptState.get());
    // do the actual work.
  }

  RefPtr<ScriptState> m_scriptState;
};

[horo, 2014-09-04 09:53:17]

Patchset #2 (id:20001) has been deleted

[horo, 2014-09-04 09:54:29]

https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/FetchBodyStream.cpp (right):

https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/FetchBodyStream.cpp:30: if (m_hasRead)
On 2014/09/04 08:13:03, haraken wrote:
> 
> I think you should check m_scriptState->contextIsEmpty() here.
> 
> We normally follow the following programming rule:
> 
> class A {
>   A(ScriptState* scriptState) : m_scriptState(scriptState) { }
> 
>   void someMethodTriggeredAsynchronously() {
>     if (m_scriptState->contextIsEmpty())
>       return;
>     ScriptState::Scope scope(m_scriptState.get());
>     // do the actual work.
>   }
> 
>   RefPtr<ScriptState> m_scriptState;
> };

Done.

[yhirano, 2014-09-04 11:10:49]

On 2014/09/04 09:54:29, horo wrote:
>
https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
> File Source/modules/serviceworkers/FetchBodyStream.cpp (right):
> 
>
https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
> Source/modules/serviceworkers/FetchBodyStream.cpp:30: if (m_hasRead)
> On 2014/09/04 08:13:03, haraken wrote:
> > 
> > I think you should check m_scriptState->contextIsEmpty() here.
> > 
> > We normally follow the following programming rule:
> > 
> > class A {
> >   A(ScriptState* scriptState) : m_scriptState(scriptState) { }
> > 
> >   void someMethodTriggeredAsynchronously() {
> >     if (m_scriptState->contextIsEmpty())
> >       return;
> >     ScriptState::Scope scope(m_scriptState.get());
> >     // do the actual work.
> >   }
> > 
> >   RefPtr<ScriptState> m_scriptState;
> > };
> 
> Done.

[yhirano, 2014-09-04 11:11:50]

On 2014/09/04 11:10:49, yhirano wrote:
> On 2014/09/04 09:54:29, horo wrote:
> >
>
https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
> > File Source/modules/serviceworkers/FetchBodyStream.cpp (right):
> > 
> >
>
https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
> > Source/modules/serviceworkers/FetchBodyStream.cpp:30: if (m_hasRead)
> > On 2014/09/04 08:13:03, haraken wrote:
> > > 
> > > I think you should check m_scriptState->contextIsEmpty() here.
> > > 
> > > We normally follow the following programming rule:
> > > 
> > > class A {
> > >   A(ScriptState* scriptState) : m_scriptState(scriptState) { }
> > > 
> > >   void someMethodTriggeredAsynchronously() {
> > >     if (m_scriptState->contextIsEmpty())
> > >       return;
> > >     ScriptState::Scope scope(m_scriptState.get());
> > >     // do the actual work.
> > >   }
> > > 
> > >   RefPtr<ScriptState> m_scriptState;
> > > };
> > 
> > Done.
(Sorry, the last message was sent by mistake.)

But readAsync is not a method triggered asynchronously, is it?

[horo, 2014-09-04 12:12:44]

On 2014/09/04 11:11:50, yhirano wrote:
> On 2014/09/04 11:10:49, yhirano wrote:
> > On 2014/09/04 09:54:29, horo wrote:
> > >
> >
>
https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
> > > File Source/modules/serviceworkers/FetchBodyStream.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/535193002/diff/1/Source/modules/serviceworker...
> > > Source/modules/serviceworkers/FetchBodyStream.cpp:30: if (m_hasRead)
> > > On 2014/09/04 08:13:03, haraken wrote:
> > > > 
> > > > I think you should check m_scriptState->contextIsEmpty() here.
> > > > 
> > > > We normally follow the following programming rule:
> > > > 
> > > > class A {
> > > >   A(ScriptState* scriptState) : m_scriptState(scriptState) { }
> > > > 
> > > >   void someMethodTriggeredAsynchronously() {
> > > >     if (m_scriptState->contextIsEmpty())
> > > >       return;
> > > >     ScriptState::Scope scope(m_scriptState.get());
> > > >     // do the actual work.
> > > >   }
> > > > 
> > > >   RefPtr<ScriptState> m_scriptState;
> > > > };
> > > 
> > > Done.
> (Sorry, the last message was sent by mistake.)
> 
> But readAsync is not a method triggered asynchronously, is it?

Ah, yes.
I uploaded patch set 3 same as patch set 1.

[yhirano, 2014-09-05 06:17:16]

https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchBodyStream.cpp (right):

https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchBodyStream.cpp:34: m_responseType = type;
Can you protect the ExecutionContext by placing a
RefPtrWillBeRawPtr<ExecutionContext> here?
Then we don't have to call scriptState->executionContext() in the latter part.
We need
 - A comment explaining why we need to save the execution context
 - ASSERT(!scriptState->contextIsEmpty())

[haraken, 2014-09-05 06:40:57]

Actually I'm not sure if adding a null check is a right fix here.

After sending a terminate notification to a worker, any V8 API on the worker
thread can fail. Thus we have to stop everything on the worker thread that can
call V8 APIs. In this particular case, I think a right fix would be to stop the
worker from reading the file stream before sending the terminate notification to
the worker.

https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchBodyStream.cpp (right):

https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchBodyStream.cpp:34: m_responseType = type;
On 2014/09/05 06:17:16, yhirano wrote:
> Can you protect the ExecutionContext by placing a
> RefPtrWillBeRawPtr<ExecutionContext> here?
> Then we don't have to call scriptState->executionContext() in the latter part.
> We need
>  - A comment explaining why we need to save the execution context
>  - ASSERT(!scriptState->contextIsEmpty())

It looks strange to use a protecting pointer for ExecutionContext. Why is it
possible that the ExecutionContext is gone during readAsync()?

[horo, 2014-09-11 06:15:27]

On 2014/09/05 06:40:57, haraken wrote:
> Actually I'm not sure if adding a null check is a right fix here.
> 
> After sending a terminate notification to a worker, any V8 API on the worker
> thread can fail. Thus we have to stop everything on the worker thread that can
> call V8 APIs. In this particular case, I think a right fix would be to stop
the
> worker from reading the file stream before sending the terminate notification
to
> the worker.
> 
>
https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
> File Source/modules/serviceworkers/FetchBodyStream.cpp (right):
> 
>
https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
> Source/modules/serviceworkers/FetchBodyStream.cpp:34: m_responseType = type;
> On 2014/09/05 06:17:16, yhirano wrote:
> > Can you protect the ExecutionContext by placing a
> > RefPtrWillBeRawPtr<ExecutionContext> here?
> > Then we don't have to call scriptState->executionContext() in the latter
part.
> > We need
> >  - A comment explaining why we need to save the execution context
> >  - ASSERT(!scriptState->contextIsEmpty())
> 
> It looks strange to use a protecting pointer for ExecutionContext. Why is it
> possible that the ExecutionContext is gone during readAsync()?

The problem is not related to the file stream.

The problem is
- In the main thread
 - v8::V8::TerminateExecution() is called to stop JavaScript execution such as
while(1) loop.
- In the worker thread
 - ScriptPromiseResolver::create() calls JavaScript "PromiseCreate()" but it
fails and TerminationException is set.
 - scriptState->executionContext() returns NULL because TerminationException is
set.

I think we have to check the result of the every call which runs JavaScript.
So we should check whether ScriptPromiseResolver::create() succeeded or not.

[yhirano, 2014-09-11 11:04:17]

On 2014/09/11 06:15:27, horo wrote:
> On 2014/09/05 06:40:57, haraken wrote:
> > Actually I'm not sure if adding a null check is a right fix here.
> > 
> > After sending a terminate notification to a worker, any V8 API on the worker
> > thread can fail. Thus we have to stop everything on the worker thread that
can
> > call V8 APIs. In this particular case, I think a right fix would be to stop
> the
> > worker from reading the file stream before sending the terminate
notification
> to
> > the worker.
> > 
> >
>
https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
> > File Source/modules/serviceworkers/FetchBodyStream.cpp (right):
> > 
> >
>
https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
> > Source/modules/serviceworkers/FetchBodyStream.cpp:34: m_responseType = type;
> > On 2014/09/05 06:17:16, yhirano wrote:
> > > Can you protect the ExecutionContext by placing a
> > > RefPtrWillBeRawPtr<ExecutionContext> here?
> > > Then we don't have to call scriptState->executionContext() in the latter
> part.
> > > We need
> > >  - A comment explaining why we need to save the execution context
> > >  - ASSERT(!scriptState->contextIsEmpty())
> > 
> > It looks strange to use a protecting pointer for ExecutionContext. Why is it
> > possible that the ExecutionContext is gone during readAsync()?
> 
> The problem is not related to the file stream.
> 
> The problem is
> - In the main thread
>  - v8::V8::TerminateExecution() is called to stop JavaScript execution such as
> while(1) loop.
> - In the worker thread
>  - ScriptPromiseResolver::create() calls JavaScript "PromiseCreate()" but it
> fails and TerminationException is set.
>  - scriptState->executionContext() returns NULL because TerminationException
is
> set.
> 
> I think we have to check the result of the every call which runs JavaScript.
> So we should check whether ScriptPromiseResolver::create() succeeded or not.

Checking after every call is simple but messy. Checking only before each
operation that needs an nonempty context is error-prone. I really want a good
idea.

[haraken, 2014-09-11 12:14:28]

haraken@chromium.org changed reviewers:
+ dcarney@chromium.org, jochen@chromium.org

[haraken, 2014-09-11 12:14:28]

On 2014/09/11 11:04:17, yhirano wrote:
> On 2014/09/11 06:15:27, horo wrote:
> > On 2014/09/05 06:40:57, haraken wrote:
> > > Actually I'm not sure if adding a null check is a right fix here.
> > > 
> > > After sending a terminate notification to a worker, any V8 API on the
worker
> > > thread can fail. Thus we have to stop everything on the worker thread that
> can
> > > call V8 APIs. In this particular case, I think a right fix would be to
stop
> > the
> > > worker from reading the file stream before sending the terminate
> notification
> > to
> > > the worker.
> > > 
> > >
> >
>
https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
> > > File Source/modules/serviceworkers/FetchBodyStream.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/535193002/diff/60001/Source/modules/servicewo...
> > > Source/modules/serviceworkers/FetchBodyStream.cpp:34: m_responseType =
type;
> > > On 2014/09/05 06:17:16, yhirano wrote:
> > > > Can you protect the ExecutionContext by placing a
> > > > RefPtrWillBeRawPtr<ExecutionContext> here?
> > > > Then we don't have to call scriptState->executionContext() in the latter
> > part.
> > > > We need
> > > >  - A comment explaining why we need to save the execution context
> > > >  - ASSERT(!scriptState->contextIsEmpty())
> > > 
> > > It looks strange to use a protecting pointer for ExecutionContext. Why is
it
> > > possible that the ExecutionContext is gone during readAsync()?
> > 
> > The problem is not related to the file stream.
> > 
> > The problem is
> > - In the main thread
> >  - v8::V8::TerminateExecution() is called to stop JavaScript execution such
as
> > while(1) loop.
> > - In the worker thread
> >  - ScriptPromiseResolver::create() calls JavaScript "PromiseCreate()" but it
> > fails and TerminationException is set.
> >  - scriptState->executionContext() returns NULL because TerminationException
> is
> > set.
> > 
> > I think we have to check the result of the every call which runs JavaScript.
> > So we should check whether ScriptPromiseResolver::create() succeeded or not.
> 
> Checking after every call is simple but messy. Checking only before each
> operation that needs an nonempty context is error-prone. I really want a good
> idea.

This is a known issue... Once the termination signal is sent to a worker thread,
any V8 API called in the worker thread can return an empty handle. Dan or Jochen
might have an idea on this issue.

In this particular case of FileStream, I think a right fix would be to stop the
streaming before sending the termination signal so that the readAsync won't get
called after the termination signal is sent. Although I agree that it's not nice
that we don't have a general solution to the above problem, it's not nice either
that we're sending a termination signal to the worker thread before stopping the
work (i.e., file streaming) on the worker.

[dcarney, 2014-09-11 12:29:23]

s particular case of FileStream, I think a right fix would be to stop the
> streaming before sending the termination signal so that the readAsync won't
get
> called after the termination signal is sent. Although I agree that it's not
nice
> that we don't have a general solution to the above problem, it's not nice
either
> that we're sending a termination signal to the worker thread before stopping
the
> work (i.e., file streaming) on the worker.

this sounds like the way to go.  in general, termination should be the last
thing to happen.  although TerminateExecution is generally problematic, the
crash rate due to shutting down workers is extremely low, so it's one we pretty
much ignore.

[horo, 2014-09-11 12:43:21]

> In this particular case of FileStream, I think a right fix would be to stop
the
> streaming before sending the termination signal so that the readAsync won't
get
> called after the termination signal is sent. Although I agree that it's not
nice
> that we don't have a general solution to the above problem, it's not nice
either
> that we're sending a termination signal to the worker thread before stopping
the
> work (i.e., file streaming) on the worker.

readAsync() is called synchronously when "response.text()" is called in JS.
So this is not the case of FileStream that you are saying.

This crash occurs in this situation
V8::TerminateExecution() is called in the main thread while the worker thread is
executing FetchBodyStream::readAsync() around the line 30~36.

[haraken, 2014-09-11 12:59:10]

On 2014/09/11 12:43:21, horo wrote:
> > In this particular case of FileStream, I think a right fix would be to stop
> the
> > streaming before sending the termination signal so that the readAsync won't
> get
> > called after the termination signal is sent. Although I agree that it's not
> nice
> > that we don't have a general solution to the above problem, it's not nice
> either
> > that we're sending a termination signal to the worker thread before stopping
> the
> > work (i.e., file streaming) on the worker.
> 
> readAsync() is called synchronously when "response.text()" is called in JS.
> So this is not the case of FileStream that you are saying.
> 
> This crash occurs in this situation
> V8::TerminateExecution() is called in the main thread while the worker thread
is
> executing FetchBodyStream::readAsync() around the line 30~36.

Sorry, I now understand your situation.

I don't think your fix is right but I cannot come up with any better fix right
now... We should probably land your fix in short term with a FIXME.

BTW, why are you checking the execution context after creating a
ScriptPromiseResolver? I guess we need to check it before creating a
ScriptPromiseResolver.

[horo, 2014-09-11 14:17:39]

Patchset #4 (id:80001) has been deleted

[horo, 2014-09-11 15:37:15]

> I don't think your fix is right but I cannot come up with any better fix right
> now... We should probably land your fix in short term with a FIXME.

Done

> BTW, why are you checking the execution context after creating a
> ScriptPromiseResolver? I guess we need to check it before creating a
> ScriptPromiseResolver

Moved to before creating a ScriptPromiseResolver.

[haraken, 2014-09-11 16:20:02]

LGTM as a short-term fix.

It's still possible that the termination signal is sent to the worker thread
after the ExecutionContext check and causes crashes.

https://codereview.chromium.org/535193002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/535193002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:23: // main thread.

Let's elaborate the comment a bit more.

When the main thread sends a V8::TerminateExecution() signal to a worker thread,
any V8 API on the worker thread starts returning an empty handle. This can
happen in Body::readAsync. To avoid the situation, we first check the
ExecutionContext and return immediately if it's already gone (which means that
the V8::TerminateExecution() signal has been sent to this worker thread).

https://codereview.chromium.org/535193002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:26: if (executionContext == 0)

if (!executionContext)

[horo, 2014-09-11 16:49:55]

https://codereview.chromium.org/535193002/diff/100001/Source/modules/servicew...
File Source/modules/serviceworkers/Body.cpp (right):

https://codereview.chromium.org/535193002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:23: // main thread.
On 2014/09/11 16:20:01, haraken wrote:
> 
> Let's elaborate the comment a bit more.
> 
> When the main thread sends a V8::TerminateExecution() signal to a worker
thread,
> any V8 API on the worker thread starts returning an empty handle. This can
> happen in Body::readAsync. To avoid the situation, we first check the
> ExecutionContext and return immediately if it's already gone (which means that
> the V8::TerminateExecution() signal has been sent to this worker thread).

Done.

https://codereview.chromium.org/535193002/diff/100001/Source/modules/servicew...
Source/modules/serviceworkers/Body.cpp:26: if (executionContext == 0)
On 2014/09/11 16:20:01, haraken wrote:
> 
> if (!executionContext)

Done.

[yhirano, 2014-09-12 01:12:23]

lgtm

[horo, 2014-09-12 01:23:13]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-09-12 01:24:08]

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

[commit-bot: I haz the power, 2014-09-12 03:08:43]

Committed patchset #5 (id:120001) as 181872