Changeset 216677 in webkit


Ignore:
Timestamp:
May 11, 2017, 8:26:03 AM (8 years ago)
Author:
mark.lam@apple.com
Message:

WorkerThread::stop() should call scheduleExecutionTermination() last.
​https://bugs.webkit.org/show_bug.cgi?id=171775
<rdar://problem/30975761>

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

Increased the number of frames captured in VM::nativeStackTraceOfLastThrow()
from 25 to 100. From experience, I found that 25 is sometimes not sufficient
for our debugging needs.

Also added VM::throwingThread() to track which thread an exception was thrown in.
This may be useful if the client is entering the VM from different threads.

  • runtime/ExceptionScope.cpp:

(JSC::ExceptionScope::unexpectedExceptionMessage):
(JSC::ExceptionScope::releaseAssertIsTerminatedExecutionException):

  • runtime/ExceptionScope.h:

(JSC::ExceptionScope::exception):
(JSC::ExceptionScope::unexpectedExceptionMessage):

  • runtime/VM.cpp:

(JSC::VM::throwException):

  • runtime/VM.h:

(JSC::VM::throwingThread):
(JSC::VM::clearException):

Source/WebCore:

Currently, WorkerThread::stop() calls scheduleExecutionTermination() to terminate
JS execution first, followed by posting a cleanup task to the worker, and lastly,
it invokes terminate() on the WorkerRunLoop.

As a result, before run loop is terminate, the worker thread may observe the
TerminatedExecutionException in JS code, bail out, see another JS task to run,
re-enters the VM to run said JS code, and fails with an assertion due to the
TerminatedExecutionException still being pending on VM entry.

WorkerRunLoop::Task::performTask() already has a check to only allow a task to
run if and only if !runLoop.terminated() and the task is not a clean up task.
We'll fix the above race by ensuring that having WorkerThread::stop() terminate
the run loop before it scheduleExecutionTermination() which throws the
TerminatedExecutionException. This way, by the time JS code unwinds out of the
VM due to the TerminatedExecutionException, runLoop.terminated() is guaranteed
to be true and thereby prevents re-entry into the VM.

This issue is covered by an existing test that I just unskipped in TestExpectations.

  • bindings/js/JSDOMPromiseDeferred.cpp:

(WebCore::DeferredPromise::callFunction):

  • workers/WorkerThread.cpp:

(WebCore::WorkerThread::stop):

LayoutTests:

Location:
trunk
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r216676 r216677  
     12017-05-11  Mark Lam  <mark.lam@apple.com>
     2
     3        WorkerThread::stop() should call scheduleExecutionTermination() last.
     4        https://bugs.webkit.org/show_bug.cgi?id=171775
     5        <rdar://problem/30975761>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        * TestExpectations:
     10
    1112017-05-11  Carlos Garcia Campos  <cgarcia@igalia.com>
    212

  • trunk/LayoutTests/TestExpectations

    r216638 r216677  
    365365
    366366webkit.org/b/161312 imported/w3c/web-platform-tests/html/semantics/document-metadata/the-link-element/document-without-browsing-context.html [ Failure Pass ]
    367 
    368 # rdar://problem/30975761
    369 [ Debug ] http/tests/fetch/fetch-in-worker-crash.html [ Skip ]
    370367
    371368imported/w3c/web-platform-tests/XMLHttpRequest/send-conditional-cors.htm [ Failure ]

  • trunk/Source/JavaScriptCore/ChangeLog

    r216672 r216677  
     12017-05-11  Mark Lam  <mark.lam@apple.com>
     2
     3        WorkerThread::stop() should call scheduleExecutionTermination() last.
     4        https://bugs.webkit.org/show_bug.cgi?id=171775
     5        <rdar://problem/30975761>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        Increased the number of frames captured in VM::nativeStackTraceOfLastThrow()
     10        from 25 to 100.  From experience, I found that 25 is sometimes not sufficient
     11        for our debugging needs.
     12
     13        Also added VM::throwingThread() to track which thread an exception was thrown in.
     14        This may be useful if the client is entering the VM from different threads.
     15
     16        * runtime/ExceptionScope.cpp:
     17        (JSC::ExceptionScope::unexpectedExceptionMessage):
     18        (JSC::ExceptionScope::releaseAssertIsTerminatedExecutionException):
     19        * runtime/ExceptionScope.h:
     20        (JSC::ExceptionScope::exception):
     21        (JSC::ExceptionScope::unexpectedExceptionMessage):
     22        * runtime/VM.cpp:
     23        (JSC::VM::throwException):
     24        * runtime/VM.h:
     25        (JSC::VM::throwingThread):
     26        (JSC::VM::clearException):
     27
    1282017-05-11  JF Bastien  <jfbastien@apple.com>
    229

  • trunk/Source/JavaScriptCore/runtime/ExceptionScope.cpp

    r216638 r216677  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2828
    2929#include "Exception.h"
     30#include "ExceptionHelpers.h"
    3031#include <wtf/StackTrace.h>
    3132#include <wtf/StringPrintStream.h>
     33#include <wtf/Threading.h>
    3234
    3335namespace JSC {
    … …  
    5456    StringPrintStream out;
    5557
    56     out.println("Unexpected exception observed at:");
    57     auto currentStack = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(25, 1));
     58    out.println("Unexpected exception observed on thread ", currentThread(), " at:");
     59    auto currentStack = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(100, 1));
    5860    currentStack->dump(out, "    ");
    5961
    … …  
    6163        return CString();
    6264   
    63     out.println("The exception was thrown from:");
     65    out.println("The exception was thrown from thread ", m_vm.throwingThread(), " at:");
    6466    m_vm.nativeStackTraceOfLastThrow()->dump(out, "    ");
    6567
    … …  
    6971#endif // ENABLE(EXCEPTION_SCOPE_VERIFICATION)
    7072   
     73void ExceptionScope::releaseAssertIsTerminatedExecutionException()
     74{
     75    RELEASE_ASSERT_WITH_MESSAGE(isTerminatedExecutionException(m_vm, exception()), "%s", unexpectedExceptionMessage().data());
     76}
     77
    7178} // namespace JSC

  • trunk/Source/JavaScriptCore/runtime/ExceptionScope.h

    r216638 r216677  
    4343    ALWAYS_INLINE void releaseAssertNoException() { RELEASE_ASSERT_WITH_MESSAGE(!exception(), "%s", unexpectedExceptionMessage().data()); }
    4444
     45    void releaseAssertIsTerminatedExecutionException();
     46
    4547protected:
    4648    ExceptionScope(VM&, ExceptionEventLocation);
    … …  
    6365    ALWAYS_INLINE VM& vm() const { return m_vm; }
    6466    ALWAYS_INLINE Exception* exception() { return m_vm.exception(); }
    65     ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
    6667
    6768    ALWAYS_INLINE void assertNoException() { ASSERT(!exception()); }
    6869    ALWAYS_INLINE void releaseAssertNoException() { RELEASE_ASSERT(!exception()); }
     70
     71    void releaseAssertIsTerminatedExecutionException();
    6972
    7073protected:
    … …  
    7477    ExceptionScope(const ExceptionScope&) = delete;
    7578    ExceptionScope(ExceptionScope&&) = default;
     79
     80    ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
    7681
    7782    VM& m_vm;

  • trunk/Source/JavaScriptCore/runtime/VM.cpp

    r216638 r216677  
    620620
    621621#if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
    622     m_nativeStackTraceOfLastThrow = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(25));
     622    m_nativeStackTraceOfLastThrow = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(100));
     623    m_throwingThread = currentThread();
    623624#endif
    624625}

  • trunk/Source/JavaScriptCore/runtime/VM.h

    r216638 r216677  
    685685#if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
    686686    StackTrace* nativeStackTraceOfLastThrow() const { return m_nativeStackTraceOfLastThrow.get(); }
     687    ThreadIdentifier throwingThread() const { return m_throwingThread; }
    687688#endif
    688689
    … …  
    720721        m_needExceptionCheck = false;
    721722        m_nativeStackTraceOfLastThrow = nullptr;
     723        m_throwingThread = 0;
    722724#endif
    723725        m_exception = nullptr;
    … …  
    767769    mutable bool m_needExceptionCheck { false };
    768770    std::unique_ptr<StackTrace> m_nativeStackTraceOfLastThrow;
     771    ThreadIdentifier m_throwingThread;
    769772#endif
    770773

  • trunk/Source/WebCore/ChangeLog

    r216669 r216677  
     12017-05-11  Mark Lam  <mark.lam@apple.com>
     2
     3        WorkerThread::stop() should call scheduleExecutionTermination() last.
     4        https://bugs.webkit.org/show_bug.cgi?id=171775
     5        <rdar://problem/30975761>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        Currently, WorkerThread::stop() calls scheduleExecutionTermination() to terminate
     10        JS execution first, followed by posting a cleanup task to the worker, and lastly,
     11        it invokes terminate() on the WorkerRunLoop.
     12
     13        As a result, before run loop is terminate, the worker thread may observe the
     14        TerminatedExecutionException in JS code, bail out, see another JS task to run,
     15        re-enters the VM to run said JS code, and fails with an assertion due to the
     16        TerminatedExecutionException still being pending on VM entry.
     17
     18        WorkerRunLoop::Task::performTask() already has a check to only allow a task to
     19        run if and only if !runLoop.terminated() and the task is not a clean up task.
     20        We'll fix the above race by ensuring that having WorkerThread::stop() terminate
     21        the run loop before it scheduleExecutionTermination() which throws the
     22        TerminatedExecutionException.  This way, by the time JS code unwinds out of the
     23        VM due to the TerminatedExecutionException, runLoop.terminated() is guaranteed
     24        to be true and thereby prevents re-entry into the VM.
     25
     26        This issue is covered by an existing test that I just unskipped in TestExpectations.
     27
     28        * bindings/js/JSDOMPromiseDeferred.cpp:
     29        (WebCore::DeferredPromise::callFunction):
     30        * workers/WorkerThread.cpp:
     31        (WebCore::WorkerThread::stop):
     32
    1332017-05-11  Chris Dumez  <cdumez@apple.com>
    234

  • trunk/Source/WebCore/bindings/js/JSDOMPromiseDeferred.cpp

    r216638 r216677  
    11/*
    2  * Copyright (C) 2013, 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    5050        return;
    5151
     52    VM& vm = exec.vm();
     53    auto scope = DECLARE_THROW_SCOPE(vm);
     54
    5255    CallData callData;
    5356    CallType callType = getCallData(function, callData);
    … …  
    5861
    5962    call(&exec, function, callType, callData, jsUndefined(), arguments);
     63
     64    // DeferredPromise should only be used by internal implementations that are well behaved.
     65    // In practice, the only exception we should ever see here is the TerminatedExecutionException.
     66    ASSERT_UNUSED(scope, !scope.exception() || isTerminatedExecutionException(vm, scope.exception()));
    6067
    6168    clear();

  • trunk/Source/WebCore/workers/WorkerThread.cpp

    r216638 r216677  
    11/*
    2  * Copyright (C) 2008 Apple Inc. All Rights Reserved.
     2 * Copyright (C) 2008-2017 Apple Inc. All Rights Reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    235235    LockHolder lock(m_threadCreationMutex);
    236236
    237     // Ensure that tasks are being handled by thread event loop. If script execution weren't forbidden, a while(1) loop in JS could keep the thread alive forever.
    238237    if (m_workerGlobalScope) {
    239         m_workerGlobalScope->script()->scheduleExecutionTermination();
    240 
    241238        m_runLoop.postTaskAndTerminate({ ScriptExecutionContext::Task::CleanupTask, [] (ScriptExecutionContext& context ) {
    242239            WorkerGlobalScope& workerGlobalScope = downcast<WorkerGlobalScope>(context);
    … …  
    263260
    264261        } });
     262       
     263        // Prevent a while (1) { } loop from keeping the worker alive forever by requesting that the worker thread
     264        // throw an uncatchable TerminatedExecutionException.
     265        //
     266        // We should only do this after terminating the runloop via the call to m_runLoop.postTaskAndTerminate()
     267        // above. This ensures that when the worker bails out of JS code due to the TerminatedExecutionException,
     268        // the m_runLoop.terminated() flag will already be set, and WorkerRunLoop::Task::performTask() won't try to
     269        // re-enter the VM with the TerminatedExecutionException still pending thereafter.
     270        m_workerGlobalScope->script()->scheduleExecutionTermination();
    265271        return;
    266272    }
Note: See TracChangeset for help on using the changeset viewer.