Changeset 216876 in webkit


Ignore:
Timestamp:
May 15, 2017, 1:34:13 PM (8 years ago)
Author:
mark.lam@apple.com
Message:

WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
​https://bugs.webkit.org/show_bug.cgi?id=171775
<rdar://problem/30975761>

Reviewed by Filip Pizlo.

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):

  • runtime/ExceptionScope.h:

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

  • runtime/Options.h:
  • Added the unexpectedExceptionStackTraceLimit option.
  • 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 the run loop is terminated, 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 changing WorkerRunLoop::Task::performTask() to check
!context->script()->isTerminatingExecution() instead of !runLoop.terminated().
Since WorkerThread::stop() always scheduleExecutionTermination() before it
terminates the run loop, !context->script()->isTerminatingExecution() implies
!runLoop.terminated().

The only time that runLoop is terminated without scheduleExecutionTermination()
being called is when WorkerThread::stop() is called before the WorkerThread has
finished creating its WorkerGlobalScope. In this scenario, WorkerThread::stop()
will still terminate the run loop. Hence, after the WorkerGlobalScope is created
(in WorkerThread::workerThread()), we will check if the run loop has been
terminated (i.e. stop() was called). If so, we'll scheduleExecutionTermination()
there, and guarantee that if runloop.terminated() is true, then
context->script()->isTerminatingExecution() is also true.

Solutions that were considered but did not work (recorded for future reference):

  1. In WorkerThread::stop(), call scheduleExecutionTermination() only after it posts the cleanup task and terminate the run loop.

This did not work because this creates a race where the worker thread may run
the cleanup task before WorkerThread::stop() finishes. As a result, the
scriptController may be deleted before we get to invoke scheduleExecutionTermination()
on it, thereby resulting in a use after free.

To make this work, we would have to change the life cycle management strategy
of the WorkerScriptController. This is a more risky change that we would
want to take on at this time, and may also not be worth the gain.

  1. Break scheduleExecutionTermination() up into 2 parts i.e. WorkerThread::stop() will:
    1. set the scriptControllers m_isTerminatingExecution flag before posting the cleanup task and terminating the run loop, and
    2. invoke VM::notifyNeedsTermination() after posting the cleanup task and terminating the run loop.

This requires that we protect the liveness of the VM until we can invoke
notifyNeedsTermination() on it.

This did not work because:

  1. We may end up destructing the VM in WorkerThread::stop() i.e. in the main web frame, but only the worker thread holds the JS lock for the VM.

We can make the WorkerThread::stop() acquire the JS lock just before it
releases the protected VM's RefPtr, but that would mean the main thread
may be stuck waiting a bit for the worker thread to release its JSLock.
This is not desirable.

  1. In practice, changing the liveness period of the Worker VM relative to its WorkerScriptController and WorkerGlobalScope also has unexpected ramifications. We observed many worker tests failing with assertion failures and crashes due to this change.

Hence, this approach is also a more risky change than it appears on the
surface, and is not worth exploring at this time.

In the end, changing WorkerRunLoop::Task::performTask() to check for
!scriptController->isTerminatingExecution() is the most straight forward solution
that is easy to prove correct.

Also fixed a race in WorkerThread::workerThread() where it can delete the
WorkerGlobalScope while WorkerThread::stop() is in the midst of accessing it.
We now guard the the nullifying of m_workerGlobalScope with the
m_threadCreationAndWorkerGlobalScopeMutex as well.

UPDATE: the only new thing in this patch for re-landing (vs one previously landed)
is that instead of nullifying m_workerGlobalScope directly (thereby deleting the
WorkerGlobalScope context), we'll swap it out and delete it only after we've
unlocked the m_threadCreationAndWorkerGlobalScopeMutex. This is needed because
the destruction of the WorkerGlobalScope will cause the main thread to race against
the worker thread to delete the WorkerThread object, and the WorkerThread object
owns the mutex that we need to unlock after nullifying the m_workerGlobalScope
field.

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

  • bindings/js/JSDOMPromiseDeferred.cpp:

(WebCore::DeferredPromise::callFunction):

  • bindings/js/WorkerScriptController.cpp:

(WebCore::WorkerScriptController::scheduleExecutionTermination):

  • Added a check to do nothing and return early if the scriptController is already terminating execution.
  • workers/WorkerRunLoop.cpp:

(WebCore::WorkerRunLoop::runInMode):
(WebCore::WorkerRunLoop::runCleanupTasks):
(WebCore::WorkerRunLoop::Task::performTask):

  • workers/WorkerRunLoop.h:
  • Made Task::performTask() private and make Task befriend the WorkerRunLoop class. This ensures that only the WorkerRunLoop may call performTask(). Note: this change only formalizes and hardens a relationship that was already in place before this.
  • workers/WorkerThread.cpp:

(WebCore::WorkerThread::start):
(WebCore::WorkerThread::workerThread):
(WebCore::WorkerThread::stop):

  • workers/WorkerThread.h:
  • Renamed m_threadCreationMutex to m_threadCreationAndWorkerGlobalScopeMutex so that it more accurately describes what it guards.

LayoutTests:

Location:
trunk
Files:
15 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r216874 r216876  
     12017-05-15  Mark Lam  <mark.lam@apple.com>
     2
     3        WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
     4        https://bugs.webkit.org/show_bug.cgi?id=171775
     5        <rdar://problem/30975761>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        * TestExpectations:
     10
    1112017-05-15  Myles C. Maxfield  <mmaxfield@apple.com>
    212

  • trunk/LayoutTests/TestExpectations

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

  • trunk/Source/JavaScriptCore/ChangeLog

    r216837 r216876  
     12017-05-15  Mark Lam  <mark.lam@apple.com>
     2
     3        WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
     4        https://bugs.webkit.org/show_bug.cgi?id=171775
     5        <rdar://problem/30975761>
     6
     7        Reviewed by Filip Pizlo.
     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        * runtime/ExceptionScope.h:
     19        (JSC::ExceptionScope::exception):
     20        (JSC::ExceptionScope::unexpectedExceptionMessage):
     21        * runtime/Options.h:
     22        - Added the unexpectedExceptionStackTraceLimit option.
     23        * runtime/VM.cpp:
     24        (JSC::VM::throwException):
     25        * runtime/VM.h:
     26        (JSC::VM::throwingThread):
     27        (JSC::VM::clearException):
     28
    1292017-05-13  David Kilzer  <ddkilzer@apple.com>
    230

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

    r216826 r216876  
    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(Options::unexpectedExceptionStackTraceLimit(), 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

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

    r216826 r216876  
    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
    … …  
    6363    ALWAYS_INLINE VM& vm() const { return m_vm; }
    6464    ALWAYS_INLINE Exception* exception() { return m_vm.exception(); }
    65     ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
    6665
    6766    ALWAYS_INLINE void assertNoException() { ASSERT(!exception()); }
    … …  
    7473    ExceptionScope(const ExceptionScope&) = delete;
    7574    ExceptionScope(ExceptionScope&&) = default;
     75
     76    ALWAYS_INLINE CString unexpectedExceptionMessage() { return { }; }
    7677
    7778    VM& m_vm;

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

    r216827 r216876  
    393393    v(bool, dumpSimulatedThrows, false, Normal, "Dumps the call stack at each simulated throw for exception scope verification") \
    394394    v(bool, validateExceptionChecks, false, Normal, "Verifies that needed exception checks are performed.") \
     395    v(unsigned, unexpectedExceptionStackTraceLimit, 100, Normal, "Stack trace limit for debugging unexpected exceptions observed in the VM") \
    395396    \
    396397    v(bool, useExecutableAllocationFuzz, false, Normal, nullptr) \

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

    r216826 r216876  
    614614
    615615#if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
    616     m_nativeStackTraceOfLastThrow = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(25));
     616    m_nativeStackTraceOfLastThrow = std::unique_ptr<StackTrace>(StackTrace::captureStackTrace(Options::unexpectedExceptionStackTraceLimit()));
     617    m_throwingThread = currentThread();
    617618#endif
    618619}

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

    r216826 r216876  
    676676#if ENABLE(EXCEPTION_SCOPE_VERIFICATION)
    677677    StackTrace* nativeStackTraceOfLastThrow() const { return m_nativeStackTraceOfLastThrow.get(); }
     678    ThreadIdentifier throwingThread() const { return m_throwingThread; }
    678679#endif
    679680
    … …  
    711712        m_needExceptionCheck = false;
    712713        m_nativeStackTraceOfLastThrow = nullptr;
     714        m_throwingThread = 0;
    713715#endif
    714716        m_exception = nullptr;
    … …  
    758760    mutable bool m_needExceptionCheck { false };
    759761    std::unique_ptr<StackTrace> m_nativeStackTraceOfLastThrow;
     762    ThreadIdentifier m_throwingThread;
    760763#endif
    761764

  • trunk/Source/WebCore/ChangeLog

    r216874 r216876  
     12017-05-15  Mark Lam  <mark.lam@apple.com>
     2
     3        WorkerRunLoop::Task::performTask() should check !scriptController->isTerminatingExecution().
     4        https://bugs.webkit.org/show_bug.cgi?id=171775
     5        <rdar://problem/30975761>
     6
     7        Reviewed by Filip Pizlo.
     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 the run loop is terminated, 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 changing WorkerRunLoop::Task::performTask() to check
     21        !context->script()->isTerminatingExecution() instead of !runLoop.terminated().
     22        Since WorkerThread::stop() always scheduleExecutionTermination() before it
     23        terminates the run loop, !context->script()->isTerminatingExecution() implies
     24        !runLoop.terminated().
     25
     26        The only time that runLoop is terminated without scheduleExecutionTermination()
     27        being called is when WorkerThread::stop() is called before the WorkerThread has
     28        finished creating its WorkerGlobalScope.  In this scenario, WorkerThread::stop()
     29        will still terminate the run loop.  Hence, after the WorkerGlobalScope is created
     30        (in WorkerThread::workerThread()), we will check if the run loop has been
     31        terminated (i.e. stop() was called).  If so, we'll scheduleExecutionTermination()
     32        there, and guarantee that if runloop.terminated() is true, then
     33        context->script()->isTerminatingExecution() is also true.
     34
     35        Solutions that were considered but did not work (recorded for future reference):
     36
     37        1. In WorkerThread::stop(), call scheduleExecutionTermination() only after it
     38           posts the cleanup task and terminate the run loop.
     39
     40           This did not work because this creates a race where the worker thread may run
     41           the cleanup task before WorkerThread::stop() finishes.  As a result, the
     42           scriptController may be deleted before we get to invoke scheduleExecutionTermination()
     43           on it, thereby resulting in a use after free.
     44
     45           To make this work, we would have to change the life cycle management strategy
     46           of the WorkerScriptController.  This is a more risky change that we would
     47           want to take on at this time, and may also not be worth the gain.
     48
     49        2. Break scheduleExecutionTermination() up into 2 parts i.e. WorkerThread::stop()
     50           will:
     51           1. set the scriptControllers m_isTerminatingExecution flag before
     52              posting the cleanup task and terminating the run loop, and
     53           2. invoke VM::notifyNeedsTermination() after posting the cleanup task and
     54              terminating the run loop.
     55
     56           This requires that we protect the liveness of the VM until we can invoke
     57           notifyNeedsTermination() on it.
     58
     59           This did not work because:
     60           1. We may end up destructing the VM in WorkerThread::stop() i.e. in the main
     61              web frame, but only the worker thread holds the JS lock for the VM.
     62
     63              We can make the WorkerThread::stop() acquire the JS lock just before it
     64              releases the protected VM's RefPtr, but that would mean the main thread
     65              may be stuck waiting a bit for the worker thread to release its JSLock.
     66              This is not desirable.
     67
     68           2. In practice, changing the liveness period of the Worker VM relative to its
     69              WorkerScriptController and WorkerGlobalScope also has unexpected
     70              ramifications.  We observed many worker tests failing with assertion
     71              failures and crashes due to this change.
     72
     73           Hence, this approach is also a more risky change than it appears on the
     74           surface, and is not worth exploring at this time.
     75
     76        In the end, changing WorkerRunLoop::Task::performTask() to check for
     77        !scriptController->isTerminatingExecution() is the most straight forward solution
     78        that is easy to prove correct.
     79
     80        Also fixed a race in WorkerThread::workerThread() where it can delete the
     81        WorkerGlobalScope while WorkerThread::stop() is in the midst of accessing it.
     82        We now guard the the nullifying of m_workerGlobalScope with the
     83        m_threadCreationAndWorkerGlobalScopeMutex as well.
     84
     85        UPDATE: the only new thing in this patch for re-landing (vs one previously landed)
     86        is that instead of nullifying m_workerGlobalScope directly (thereby deleting the
     87        WorkerGlobalScope context), we'll swap it out and delete it only after we've
     88        unlocked the m_threadCreationAndWorkerGlobalScopeMutex.  This is needed because
     89        the destruction of the WorkerGlobalScope will cause the main thread to race against
     90        the worker thread to delete the WorkerThread object, and the WorkerThread object
     91        owns the mutex that we need to unlock after nullifying the m_workerGlobalScope
     92        field.
     93
     94        This issue is covered by an existing test that I just unskipped in TestExpectations.
     95
     96        * bindings/js/JSDOMPromiseDeferred.cpp:
     97        (WebCore::DeferredPromise::callFunction):
     98
     99        * bindings/js/WorkerScriptController.cpp:
     100        (WebCore::WorkerScriptController::scheduleExecutionTermination):
     101        - Added a check to do nothing and return early if the scriptController is already
     102          terminating execution.
     103
     104        * workers/WorkerRunLoop.cpp:
     105        (WebCore::WorkerRunLoop::runInMode):
     106        (WebCore::WorkerRunLoop::runCleanupTasks):
     107        (WebCore::WorkerRunLoop::Task::performTask):
     108
     109        * workers/WorkerRunLoop.h:
     110        - Made Task::performTask() private and make Task befriend the WorkerRunLoop class.
     111          This ensures that only the WorkerRunLoop may call performTask().
     112          Note: this change only formalizes and hardens a relationship that was already
     113          in place before this.
     114
     115        * workers/WorkerThread.cpp:
     116        (WebCore::WorkerThread::start):
     117        (WebCore::WorkerThread::workerThread):
     118        (WebCore::WorkerThread::stop):
     119        * workers/WorkerThread.h:
     120        - Renamed m_threadCreationMutex to m_threadCreationAndWorkerGlobalScopeMutex so
     121          that it more accurately describes what it guards.
     122
    11232017-05-15  Myles C. Maxfield  <mmaxfield@apple.com>
    2124

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

    r216826 r216876  
    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/bindings/js/WorkerScriptController.cpp

    r216826 r216876  
    153153void WorkerScriptController::scheduleExecutionTermination()
    154154{
     155    if (m_isTerminatingExecution)
     156        return;
     157
    155158    {
    156159        // The mutex provides a memory barrier to ensure that once

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

    r216826 r216876  
    11/*
    22 * Copyright (C) 2009 Google Inc. All rights reserved.
    3  * Copyright (C) 2016 Apple Inc.  All rights reserved.
     3 * Copyright (C) 2016-2017 Apple Inc.  All rights reserved.
    44 *
    55 * Redistribution and use in source and binary forms, with or without
    … …  
    200200
    201201    case MessageQueueMessageReceived:
    202         task->performTask(*this, context);
     202        task->performTask(context);
    203203        break;
    204204
    … …  
    229229        if (!task)
    230230            return;
    231         task->performTask(*this, context);
     231        task->performTask(context);
    232232    }
    233233}
    … …  
    253253}
    254254
    255 void WorkerRunLoop::Task::performTask(const WorkerRunLoop& runLoop, WorkerGlobalScope* context)
    256 {
    257     if ((!context->isClosing() && !runLoop.terminated()) || m_task.isCleanupTask())
     255void WorkerRunLoop::Task::performTask(WorkerGlobalScope* context)
     256{
     257    ASSERT(context->script());
     258    if ((!context->isClosing() && !context->script()->isTerminatingExecution()) || m_task.isCleanupTask())
    258259        m_task.performTask(*context);
    259260}

  • trunk/Source/WebCore/workers/WorkerRunLoop.h

    r216826 r216876  
    11/*
    22 * Copyright (C) 2009 Google Inc. All rights reserved.
    3  *
     3 * Copyright (C) 2017 Apple Inc.  All rights reserved.
     4 *
    45 * Redistribution and use in source and binary forms, with or without
    56 * modification, are permitted provided that the following conditions are
    … …  
    7172            Task(ScriptExecutionContext::Task&&, const String& mode);
    7273            const String& mode() const { return m_mode; }
    73             void performTask(const WorkerRunLoop&, WorkerGlobalScope*);
    7474
    7575        private:
     76            void performTask(WorkerGlobalScope*);
     77
    7678            ScriptExecutionContext::Task m_task;
    7779            String m_mode;
     80
     81            friend class WorkerRunLoop;
    7882        };
    7983

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

    r216826 r216876  
    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
    … …  
    134134{
    135135    // Mutex protection is necessary to ensure that m_thread is initialized when the thread starts.
    136     LockHolder lock(m_threadCreationMutex);
     136    LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
    137137
    138138    if (m_thread)
    … …  
    161161#endif
    162162
     163    WorkerScriptController* scriptController;
    163164    {
    164         LockHolder lock(m_threadCreationMutex);
     165        // Mutex protection is necessary to ensure that we don't change m_workerGlobalScope
     166        // while WorkerThread::stop() is accessing it. Note that WorkerThread::stop() can
     167        // be called before we've finished creating the WorkerGlobalScope.
     168        LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
    165169        m_workerGlobalScope = createWorkerGlobalScope(m_startupData->m_scriptURL, m_startupData->m_identifier, m_startupData->m_userAgent, m_startupData->m_contentSecurityPolicyResponseHeaders, m_startupData->m_shouldBypassMainWorldContentSecurityPolicy, WTFMove(m_startupData->m_topOrigin), m_startupData->m_timeOrigin);
     170
     171        scriptController = m_workerGlobalScope->script();
    166172
    167173        if (m_runLoop.terminated()) {
    168174            // The worker was terminated before the thread had a chance to run. Since the context didn't exist yet,
    169175            // forbidExecution() couldn't be called from stop().
    170             m_workerGlobalScope->script()->forbidExecution();
     176            scriptController->scheduleExecutionTermination();
     177            scriptController->forbidExecution();
    171178        }
    172179    }
    … …  
    177184        // If the worker was somehow terminated while processing debugger commands.
    178185        if (m_runLoop.terminated())
    179             m_workerGlobalScope->script()->forbidExecution();
    180     }
    181 
    182     WorkerScriptController* script = m_workerGlobalScope->script();
    183     script->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL));
     186            scriptController->forbidExecution();
     187    }
     188
     189    scriptController->evaluate(ScriptSourceCode(m_startupData->m_sourceCode, m_startupData->m_scriptURL));
    184190    // Free the startup data to cause its member variable deref's happen on the worker's thread (since
    185191    // all ref/derefs of these objects are happening on the thread at this point). Note that
    … …  
    197203    ASSERT(m_workerGlobalScope->hasOneRef());
    198204
     205    RefPtr<WorkerGlobalScope> workerGlobalScopeToDelete;
     206    {
     207        // Mutex protection is necessary to ensure that we don't change m_workerGlobalScope
     208        // while WorkerThread::stop is accessing it.
     209        LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
     210
     211        // Delay the destruction of the WorkerGlobalScope context until after we've unlocked the
     212        // m_threadCreationAndWorkerGlobalScopeMutex. This is needed because destructing the
     213        // context will trigger the main thread to race against us to delete the WorkerThread
     214        // object, and the WorkerThread object owns the mutex we need to unlock after this.
     215        workerGlobalScopeToDelete = WTFMove(m_workerGlobalScope);
     216    }
     217
    199218    // The below assignment will destroy the context, which will in turn notify messaging proxy.
    200219    // We cannot let any objects survive past thread exit, because no other thread will run GC or otherwise destroy them.
    201     m_workerGlobalScope = nullptr;
     220    workerGlobalScopeToDelete = nullptr;
    202221
    203222    // Clean up WebCore::ThreadGlobalData before WTF::WTFThreadData goes away!
    … …  
    232251void WorkerThread::stop()
    233252{
    234     // Mutex protection is necessary because stop() can be called before the context is fully created.
    235     LockHolder lock(m_threadCreationMutex);
     253    // Mutex protection is necessary to ensure that m_workerGlobalScope isn't changed by
     254    // WorkerThread::workerThread() while we're accessing it. Note also that stop() can
     255    // be called before m_workerGlobalScope is fully created.
     256    LockHolder lock(m_threadCreationAndWorkerGlobalScopeMutex);
    236257
    237258    // 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.

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

    r216826 r216876  
    11/*
    2  * Copyright (C) 2008, 2016 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
    … …  
    107107
    108108    RefPtr<WorkerGlobalScope> m_workerGlobalScope;
    109     Lock m_threadCreationMutex;
     109    Lock m_threadCreationAndWorkerGlobalScopeMutex;
    110110
    111111    std::unique_ptr<WorkerThreadStartupData> m_startupData;
Note: See TracChangeset for help on using the changeset viewer.