Bug 37053 – WorkerGlobalScope.close() should let the currently running script complete execution, then terminate the worker.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Dmitry Titov]

As a result of WHATWG discussion, we need to change the behavior of close() method. This will make 3 implementations (FF, Safari and Chrome) behave the same way.
Basically, close() will return and allow the remaiing JS in the running JS fragment to complete execution with all worker functionality unaffected, but then terminate the worker when it returns from JS.

The terminate() and closing parent context still terminate the worker using 'terminate a worker' algorithm from the spec.

http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-March/025727.html and http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-April/025763.html

[Andrew Wilson]

Note that this may be tricky. There are places in the system (e.g. dispatching MessagePort messages, and I believe certain state transitions for XHR) where we call back into JS multiple times before we exit back out to the event loop.

[Dmitry Titov]

Created attachment 52473 [details]
Patch.

The WorkerContext::close() now disables future execution (but does not interrupt current JS fragment) and posts a cleanup task that will eventually call WorkerThread::stop().
Since WorkerContext::isClosed() returns true, all non-cleanup tasks that may happen to be in the queue before StopWorkerContextTask will be thrown away w/o execution.
Updated test to match.

[Dmitry Titov]

(In reply to comment #1)
> Note that this may be tricky. There are places in the system (e.g. dispatching
> MessagePort messages, and I believe certain state transitions for XHR) where we
> call back into JS multiple times before we exit back out to the event loop.

Good point. In V8 case, we have the checks in right places (running any JS needs to retrieve a proxy/context, they are retrieved as 0 when WorkerScriptController.m_executionForbidden == true.

In JSC case I'm not sure we have this. Will check. Removing r? for now.

[Andrew Wilson]

Comment on attachment 52473 [details]
Patch.


> diff --git a/LayoutTests/fast/workers/worker-close.html b/LayoutTests/fast/workers/worker-close.html
> index fd8dfd8..e6c0733 100644
> --- a/LayoutTests/fast/workers/worker-close.html
> +++ b/LayoutTests/fast/workers/worker-close.html

> +    // Tell the worker to close, then send a followup message which should be delivered.
> +    // However, the 'ping' should not come back as 'pong', since that is a separate message
> +    // and onmessage handler should not be called again after the one with close() exits.

I'm not sure I understand this comment. If the onmessage handler isn't called for the "ping" message, then that means it wasn't delivered, right? Maybe you mean to say that "ping" won't be delivered since the worker will already be closed?

> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,38 @@
> [phone]  Dmitry Titov  <[e-mail]>
> +
> +        (WebCore::WorkerScriptController::):
> +        Added option parameter to forbidExecution (bith JCS and V8 versions) that specifies whether currently running
> +        script should be immediately terminated or left executed until the end.

Typo: should be "both JSC and V8", not "bith JCS and V8".

Overall this looks good - I'd like to see us add one more test:

1) Page starts worker
2) Page sends message to worker to invoke close()
3) Worker invokes close(), sends message back to page, then enters infinite loop
4) Page receives message from step 3, calls worker.terminate().

At this point, we can use the waitUntilWorkerThreadsExit() helper function to ensure that the thread itself actually exits (we'd need to roll a different test for V8 since that DRT function doesn't exist there).

[Dmitry Titov]

Created attachment 52909 [details]
Patch.

Updated patch:

- fixed comments and typos.
- Added a check into WorkerScriptController for JCS to mimic what v8 version also does to prevent re-entry into JS when further execution is forbidden.
- Added 2 new tests, one that Andrew Wilson has suggested and another one for MessagePort events dispatching while calling close().

[Darin Adler]

Comment on attachment 52909 [details]
Patch.

> +        return new CloseWorkerContextTask();

I normally leave out the parentheses in new expressions like this one.

> -    // Notify parent that this context is closed. Parent is responsible for calling WorkerThread::stop().
> -    thread()->workerReportingProxy().workerContextClosed();
> +    // Let current script run to completion  but prevent future script evaluations.

Extra space here before "but".

While I don't understand every bit of the patch thoroughly, it looks good to me, and the tests seem to cover what's changing here. r=me

[Dmitry Titov]

Updated per Darin's comments and landed: http://trac.webkit.org/changeset/57349

[Andrew Wilson]

Just had a chance to review this. I'm concerned that just returning 0 from workerContextWrapper() will lead to crashes in various places. For example, ScheduledAction calls workerContextWrapper() and uses the return value without any further checking (although in theory ScheduledAction shouldn't happen after close(), right?).

My other concern is there are other places in WorkerScriptController.cpp that just use m_workerContextWrapper directly to execute code without checking forbidExecution first, so I'm wondering if any further changes may still be necessary.

[Dmitry Titov]

(In reply to comment #8)
> Just had a chance to review this. I'm concerned that just returning 0 from
> workerContextWrapper() will lead to crashes in various places. For example,
> ScheduledAction calls workerContextWrapper() and uses the return value without
> any further checking (although in theory ScheduledAction shouldn't happen after
> close(), right?).

I looked over all the places that use this call. The ones that do not check for 0 are guaranteed not to be called by virtue of WorkerContext::isClosed() being true and runloop not executing tasks after close(). For example, ScheduledAction is nto executed because it's a timer callback mechanism that gets executed by runloop as well.

The most important consumer of workerContextWrapper() is toJSDOMGlobalObject(..), which normally precedes any reentry into JS (as in event handling). All those places check for 0 and return early.

I pondered to be 'safe' and add checks everywhere but then realized that it in fact can mask other issues (lets say we have a regression and continue execute tasks in the Worker run loop even though isClose flag is set on a context). Crash would be a better trap then not executing something.

> My other concern is there are other places in WorkerScriptController.cpp that
> just use m_workerContextWrapper directly to execute code without checking
> forbidExecution first, so I'm wondering if any further changes may still be
> necessary.

The only other place I can see that doesn't check for m_forbidExecution is WorkerScriptController::setException(). I am not sure it has to be protected, since it's essentially an assignment into a variable. But I'll give it additional look.