Bug 171775 – WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mark Lam]

DeferredPromise::callFunction() is a private method and is only called by DeferredPromise::resolve() and DeferredPromise::reject().  Currently, all of the clients of resolve() and reject() are just ignoring any uncaught exceptions.  So, callFunction() should just clear any uncaught exceptions before returning.

<rdar://problem/30975761>

[Mark Lam]

Created attachment 309293 [details]
proposed patch.

Let's try this on the EWS.

[youenn fablet]

Comment on attachment 309293 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=309293&action=review

> Source/WebCore/bindings/js/JSDOMPromise.cpp:64
> +    scope.clearException();

The case http/tests/fetch/fetch-in-worker-crash.html is crashing is probably because of terminated execution exception.
Shouldn't we do similarly to DeferredPromise::reject and not handle any other exception than terminated execution?

[Joseph Pecoraro]

Comment on attachment 309293 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=309293&action=review

>> Source/WebCore/bindings/js/JSDOMPromise.cpp:64
>> +    scope.clearException();
> 
> The case http/tests/fetch/fetch-in-worker-crash.html is crashing is probably because of terminated execution exception.
> Shouldn't we do similarly to DeferredPromise::reject and not handle any other exception than terminated execution?

So my understanding is that any DeferredPromise (hopefully soon to be named DOMDeferredPromise) will have the non-user-defined resolve/reject functions. Also, it will be a promise created by the engine, so there is no user code that knows about this promise getting resolved / rejected. If that is the case we should be able to Assert that the exception is in some known set (Terminated, Out of Memory, Stack Exhausted).

Since, I believe, there is no user code being run it is probably correct to swallow the exception. But in the case of Out of Memory and Stack Exhausted we may be leaving the page in a bad state (the page may depend on this promise being resolved/rejected to move forward). Not sure if there is anything we can or should do in this case.

[Geoffrey Garen]

Comment on attachment 309293 [details]
proposed patch.

r=me

Since there's no API to pass an exception to a client, I think it makes sense to catch and clear the exception unconditionally. It's true that this might lead to a broken webpage, but I can't see a way to improve upon that.

[Mark Lam]

Created attachment 309401 [details]
patch for landing w/ added assertions.

I've added assertions that the unhandled exception must be one of the runtime exceptions we expect can happen before clearing it.  Will let the EWS confirm that this is all kosher before landing.

[Mark Lam]

Created attachment 309405 [details]
real patch for landing.

Geoff convinced me that the StackOverflow and OutOfMemory error checks are not meaningful.  We don't expect out own code to be operating near the boundaries of the stack limit nor near max memory usage.  I've removed those 2 assertions as well as the hacks to test if the exception is of those.

[youenn fablet]

(In reply to Mark Lam from comment #6)
> Created attachment 309405 [details]
> real patch for landing.
> 
> Geoff convinced me that the StackOverflow and OutOfMemory error checks are
> not meaningful.  We don't expect out own code to be operating near the
> boundaries of the stack limit nor near max memory usage.  I've removed those
> 2 assertions as well as the hacks to test if the exception is of those.

Agreeing with the approach.
A similar check is done in DeferredPromise::reject(Exception&& exception) and DeferredPromise::reject(ExceptionCode ec, const String& message).
It seems confusing that the checks are not written the same way if they are equivalent.
Or is there a behavioral difference?

[Mark Lam]

(In reply to youenn fablet from comment #7)
> (In reply to Mark Lam from comment #6)
> > Created attachment 309405 [details]
> > real patch for landing.
> > 
> > Geoff convinced me that the StackOverflow and OutOfMemory error checks are
> > not meaningful.  We don't expect out own code to be operating near the
> > boundaries of the stack limit nor near max memory usage.  I've removed those
> > 2 assertions as well as the hacks to test if the exception is of those.
> 
> Agreeing with the approach.
> A similar check is done in DeferredPromise::reject(Exception&& exception)
> and DeferredPromise::reject(ExceptionCode ec, const String& message).
> It seems confusing that the checks are not written the same way if they are
> equivalent.
> Or is there a behavioral difference?

Hmmm.  You have a point.  Now I'm wondering why we're trying to re-enter the VM with a TerminatedException still in play.  I'll investigate.

[Mark Lam]

Created attachment 309534 [details]
proposed patch.

After considering Youenn's feedback, I realized that it is wrong to clear the exception.  I've prepared a new patch with a new solution.

[Mark Lam]

Created attachment 309535 [details]
proposed patch rebased.

[Mark Lam]

Created attachment 309541 [details]
proposed patch w/ ASSERT_UNUSED build fix.

Let's try this on the EWS first.

[Mark Lam]

Comment on attachment 309541 [details]
proposed patch w/ ASSERT_UNUSED build fix.

EWS bots are happy.  Let's get a review.

[Geoffrey Garen]

Comment on attachment 309541 [details]
proposed patch w/ ASSERT_UNUSED build fix.

Can the VM client -- like the Worker runloop -- do this check instead, before calling into JSC?

There are two benefits to doing this check in the client:

(1) JSC can do expensive things that do not pass through the Interpreter. You wouldn't want a worker to continue doing that work after being terminated.

(2) There's no clear context inside Interpreter.cpp to explain why a terminated execution exception should get special treatment. The only real reason it should get special treatment is that that's behavior that workers want. It's much clearer to express that idea by implementing that behavior in workers.

[Mark Lam]

(In reply to Geoffrey Garen from comment #13)
> Comment on attachment 309541 [details]
> proposed patch w/ ASSERT_UNUSED build fix.
> 
> Can the VM client -- like the Worker runloop -- do this check instead,
> before calling into JSC?

I looked into that, but:
1. WorkerRunLoop::runInMode() just invokes WorkerRunLoop::Task::performTask(), which then invokes m_task.performTask().
   We can't gate JS execution at the WorkerRunLoop::Task::performTask() level because it has no knowledge of whether the task will run JS or not.  Not all tasks run JS.
   We don't want to gate all tasks because the run loop relies on eventually getting to a CleanupTask task to properly shutdown the worker (see WorkerThread::stop()).

2. m_task is an instance of some ScriptExecutionContext::Task which is instantiated all over.  Not all of these call into JS.
   To properly hate JS entry at each instance of ScriptExecutionContext::Task::performTask(), we'll need to audit every instantiation of it.
   And that doesn't even guarantee that we'll catch all the ones in the future.

> There are two benefits to doing this check in the client:
> 
> (1) JSC can do expensive things that do not pass through the Interpreter.
> You wouldn't want a worker to continue doing that work after being
> terminated.

This is unfortunate, but it's hard to tell which task in the worker's runLoop queue needs to be serviced (for proper clean up), and which one can be thrown out.  Running thru all of them is safer.

> (2) There's no clear context inside Interpreter.cpp to explain why a
> terminated execution exception should get special treatment. The only real
> reason it should get special treatment is that that's behavior that workers
> want. It's much clearer to express that idea by implementing that behavior
> in workers.

This is needed not only for workers, but also for ObjC clients that may have requested terminated script execution.  The client in this case, may use a different task dispatching scheme than our workers.  It may be similarly tricky for the client to catch all the places where it needs to gate JS entry.  Making our VM honor the pending TerminatedExecutionException, makes it much easier for clients to not run into the issue of running the VM in an inconsistent state.

[Geoffrey Garen]

>    We can't gate JS execution at the WorkerRunLoop::Task::performTask()
> level because it has no knowledge of whether the task will run JS or not. 
> Not all tasks run JS.
>    We don't want to gate all tasks because the run loop relies on eventually
> getting to a CleanupTask task to properly shutdown the worker (see
> WorkerThread::stop()).

WorkerGlobalScope::close() says this:

    // Let current script run to completion but prevent future script evaluations.
    // After m_closing is set, all the tasks in the queue continue to be fetched but only
    // tasks with isCleanupTask()==true will be executed.
    m_closing = true;
    postTask({ ScriptExecutionContext::Task::CleanupTask, [] (ScriptExecutionContext& context) {

WorkerRunLoop::Task::performTask() does this:

    if ((!context->isClosing() && !runLoop.terminated()) || m_task.isCleanupTask())
        m_task.performTask(*context);

It seems like this code is already engineered to short-circuit tasks after being asked to shut down, while still allowing cleanup tasks.

I think you should work within this design and try to understand why a worker ends up running a task that executes JavaScript even after being asked to terminate. If necessary, you can add a termination check here to short-circuit execution.

But it's not currently clear why the existing check is insufficient. Perhaps what you've discovered is that the microtask queue must also check for termination, and clear pending tasks upon termination?

> > There are two benefits to doing this check in the client:
> > 
> > (1) JSC can do expensive things that do not pass through the Interpreter.
> > You wouldn't want a worker to continue doing that work after being
> > terminated.
> 
> This is unfortunate, but it's hard to tell which task in the worker's
> runLoop queue needs to be serviced (for proper clean up), and which one can
> be thrown out.  Running thru all of them is safer.

Let's make it easy to tell. It appears, based on the code above, that it is already easy to tell. But maybe I'm missing something.

> > (2) There's no clear context inside Interpreter.cpp to explain why a
> > terminated execution exception should get special treatment. The only real
> > reason it should get special treatment is that that's behavior that workers
> > want. It's much clearer to express that idea by implementing that behavior
> > in workers.
> 
> This is needed not only for workers, but also for ObjC clients that may have
> requested terminated script execution.  The client in this case, may use a
> different task dispatching scheme than our workers.  It may be similarly
> tricky for the client to catch all the places where it needs to gate JS
> entry.  Making our VM honor the pending TerminatedExecutionException, makes
> it much easier for clients to not run into the issue of running the VM in an
> inconsistent state.

Our ObjC and C APIs clear pending exceptions before returning. So this change won't affect them.

[Mark Lam]

(In reply to Geoffrey Garen from comment #15)
> >    We can't gate JS execution at the WorkerRunLoop::Task::performTask()
> > level because it has no knowledge of whether the task will run JS or not. 
> > Not all tasks run JS.
> >    We don't want to gate all tasks because the run loop relies on eventually
> > getting to a CleanupTask task to properly shutdown the worker (see
> > WorkerThread::stop()).
> 
> WorkerGlobalScope::close() says this:
> 
>     // Let current script run to completion but prevent future script
> evaluations.
>     // After m_closing is set, all the tasks in the queue continue to be
> fetched but only
>     // tasks with isCleanupTask()==true will be executed.
>     m_closing = true;
>     postTask({ ScriptExecutionContext::Task::CleanupTask, []
> (ScriptExecutionContext& context) {
> 
> WorkerRunLoop::Task::performTask() does this:
> 
>     if ((!context->isClosing() && !runLoop.terminated()) ||
> m_task.isCleanupTask())
>         m_task.performTask(*context);
> 
> It seems like this code is already engineered to short-circuit tasks after
> being asked to shut down, while still allowing cleanup tasks.
> 
> I think you should work within this design and try to understand why a
> worker ends up running a task that executes JavaScript even after being
> asked to terminate. If necessary, you can add a termination check here to
> short-circuit execution.
> 
> But it's not currently clear why the existing check is insufficient. Perhaps
> what you've discovered is that the microtask queue must also check for
> termination, and clear pending tasks upon termination?

Hmmm, now how did I miss that?  Anyway, the sequence of events of the race at worker shutdown is this:

1. Worker is running something in JS.
2. Main thread decides to terminate the worker.
3. Main thread invokes m_workerGlobalScope->script()->scheduleExecutionTermination().
4. Main thread postTaskAndTerminate() a shutdown task that will removeAllEventListeners(), etc to clean up the worker.
5. Main thread invokes m_runLoop.terminate().

At (3), a TerminatedExecutionException to be thrown in the worker, and causing it to bail out of JS.
Between (3) and (5), there could have been more tasks queued up in the worker that may want to execute JS code.

The fix is simply to have WorkerThread::stop() call scheduleExecutionTermination() last.  That way, the !runLoop.terminated() will ensure that the worker never re-enters the VM once it has bailed due to a TerminatedExecutionException.

[Mark Lam]

Created attachment 309644 [details]
proposed patch with better fix.

[Geoffrey Garen]

Comment on attachment 309644 [details]
proposed patch with better fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=309644&action=review

r=me

> Source/WebCore/workers/WorkerThread.cpp:267
> +    // We need to terminate the runLoop before we

Looks like this comment got cut off mid-sentence. I would say "Prevent a while (1) { } loop from keeping the worker alive forever."

[Mark Lam]

Comment on attachment 309644 [details]
proposed patch with better fix.

View in context: https://bugs.webkit.org/attachment.cgi?id=309644&action=review

>> Source/WebCore/workers/WorkerThread.cpp:267
>> +    // We need to terminate the runLoop before we
> 
> Looks like this comment got cut off mid-sentence. I would say "Prevent a while (1) { } loop from keeping the worker alive forever."

The second line doesn't belong there.  I was just transplanting the comment from above (at line 237).

[Build Bot]

Comment on attachment 309644 [details]
proposed patch with better fix.

Attachment 309644 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3714268

New failing tests:
fast/workers/worker-terminate-forever.html
fast/workers/worker-document-leak.html

[Build Bot]

Created attachment 309653 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309644 [details]
proposed patch with better fix.

Attachment 309644 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3714310

New failing tests:
fast/workers/worker-terminate-forever.html
fast/workers/worker-document-leak.html

[Build Bot]

Created attachment 309659 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 309644 [details]
proposed patch with better fix.

Attachment 309644 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3714299

New failing tests:
fast/workers/worker-close-more.html
fast/workers/worker-terminate-forever.html

[Build Bot]

Created attachment 309660 [details]
Archive of layout-test-results from ews113 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Mark Lam]

Thanks for the review.  Landed in r216635: <http://trac.webkit.org/r216635> before I saw the test failures.  So, rolled out in r216638 while I investigate the issue.

[Mark Lam]

That was so dumb.  I didn't see the return at the bottom of the "if (m_workerGlobalScope)" case.  Naturally, JS code never terminates now because we now never call scheduleExecutionTermination().  Hence, this fix is invalid.

I'll see how m_runLoop.terminate() gets called if not here in WorkerThread::stop() when m_workerGlobalScope is non-null.

[Mark Lam]

Created attachment 309671 [details]
patch for landing.

The idea behind the fix was sound.  The placement of the call to scheduleExecutionTermination() was not.  Basically, we should put it before the return statement that we didn't notice in the last patch.

As to why the non-null m_workerGlobalScope path does not explicitly call m_runLoop.terminate(), it is because m_runLoop.postTaskAndTerminate() will do the equivalent of calling terminate() after the "postTask" part.  So, the final fix is the same as the one that Geoff reviewed i.e. put the call to scheduleExecutionTermination() after the call to m_runLoop.postTaskAndTerminate() which terminates the runloop ... but before the return statement this time.

I'll wait for the EWS to confirm that all is well before I land this time.

[Mark Lam]

The bots are all green.  Landed in r216677: <http://trac.webkit.org/r216677>.

[Mark Lam]

Re-opening because the fix causes a crash:
https://build.webkit.org/results/Apple%20Sierra%20Release%20WK1%20(Tests)/r216684%20(1509)/results.html

[Matt Lewis]

Reverted r216677 for reason:

Patch caused layout test crashes.

Committed r216707: <http://trac.webkit.org/changeset/216707>

[Mark Lam]

*** Bug 171990 has been marked as a duplicate of this bug. ***

[Mark Lam]

Created attachment 309911 [details]
proposed patch.

Hopefully I'll have fixed it for real this time.  Let's test this on the EWS bots first.

[Build Bot]

Attachment 309911 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:43:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use after free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Saam Barati]

Comment on attachment 309911 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=309911&action=review

> Source/JavaScriptCore/runtime/ExceptionScope.cpp:59
> +    auto currentStack = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(100, 1));

Lets make this an option since there are multiple places that use the literal "100"

> Source/WebCore/ChangeLog:13
> +        As a result, before run loop is terminate, the worker thread may observe the

terminate => terminated

> Source/WebCore/ChangeLog:20
> +        We'll fix the above race by changing WorkerRunLoop::Task::performTask() to check

I don't understand how this is a race. Can you explain?

[Mark Lam]

Comment on attachment 309911 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=309911&action=review

Thanks for the review.

>> Source/JavaScriptCore/runtime/ExceptionScope.cpp:59
>> +    auto currentStack = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(100, 1));
> 
> Lets make this an option since there are multiple places that use the literal "100"

Fixed.

>> Source/WebCore/ChangeLog:13
>> +        As a result, before run loop is terminate, the worker thread may observe the
> 
> terminate => terminated

Fixed.

>> Source/WebCore/ChangeLog:20
>> +        We'll fix the above race by changing WorkerRunLoop::Task::performTask() to check
> 
> I don't understand how this is a race. Can you explain?

I explained this offline but I'll record it here for others who may be curious.  Basically,
1. In WorkerRunLoop's message queue, there may already be multiple tasks queued up that will each execute some JS code.
2. In WorkerThread::stop(), between the call to scheduleExecutionTermination() and the call to postTaskAndTerminate(), the main thread gets pre-empted and the worker thread runs.
3. The worker thread sees the TerminatedExecutionException and unwinds out of the JS code it's running for a task.
4. The WorkerRunLoop see that the run loop hasn't been terminated yet (because the main loop hasn't called postTaskAndTerminate() yet), and runs the next task in its queue.
5. The next task runs some JS code, and therefore re-enters the VM while the TerminatedExecutionException is still pending.  This is the issue we're fixing with this patch.

[Mark Lam]

Created attachment 309938 [details]
patch for landing.

Let's get one more round of EWS testing for confidence before landing.

[Build Bot]

Attachment 309938 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:43:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use after free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 309938 [details]
patch for landing.

Bots are green.  Let's land this.

[WebKit Commit Bot]

Comment on attachment 309938 [details]
patch for landing.

Clearing flags on attachment: 309938

Committed r216801: <http://trac.webkit.org/changeset/216801>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This seems to have caused crashes on the leaks bot: https://build.webkit.org/results/Apple%20Sierra%20(Leaks)/r216802%20(1031)/results.html

I'm not really sure about the state of al the related bugs to comfortably roll back, but we need to figure it out soon.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 172072

[Alexey Proskuryakov]

This caused dozens of crashes on ASan and GuardMalloc, so rolling out.

[Simon Fraser (smfr)]

This is a good example of a patch that triggers a full rebuild of WebCore, but should not.

[Mark Lam]

(In reply to Simon Fraser (smfr) from comment #45)
> This is a good example of a patch that triggers a full rebuild of WebCore,
> but should not.

Details on "should not" please.  I didn't see anything that was superflous in the patch.

[Mark Lam]

Created attachment 310075 [details]
proposed patch with ASan fix.

[Build Bot]

Attachment 310075 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:43:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: use after free  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Thanks for the review.  Landed in r216876:<http://trac.webkit.org/r216876>.