Changeset 57349 in webkit


Ignore:
Timestamp:
Apr 9, 2010 12:29:43 PM (14 years ago)
Author:
dimich@chromium.org
Message:

WebCore: WorkerGlobalScope.close() should let the currently running script complete execution, then terminate the worker.
​https://bugs.webkit.org/show_bug.cgi?id=37053

Reviewed by Darin Adler.

WebCore:

Test: fast/workers/worker-close-more.html

  • bindings/js/WorkerScriptController.cpp:

(WebCore::WorkerScriptController::forbidExecution):
(WebCore::WorkerScriptController::):

  • bindings/v8/WorkerScriptController.cpp:

(WebCore::WorkerScriptController::forbidExecution):

  • bindings/v8/WorkerScriptController.h:

(WebCore::WorkerScriptController::):
Added option parameter to forbidExecution (both JCS and V8 versions) that specifies whether currently running
script should be immediately terminated or left executed until the end.

  • bindings/js/WorkerScriptController.h:

(WebCore::WorkerScriptController::workerContextWrapper):
This method now can return 0 instead of context if the further execution of JS is forbidden. Since any JS execution requires
fetching JS global object first, returning 0 here is a good way to prevent re-entry into JS once worker started termination.
V8 version does similar thing already in WorkerScriptController::proxy().

  • workers/DedicatedWorkerContext.cpp:

(WebCore::DedicatedWorkerContext::postMessage):
Removed check that immediately disables postMessage from WorkerContext after close().

  • workers/WorkerContext.cpp:

(WebCore::CloseWorkerContextTask::create):
(WebCore::CloseWorkerContextTask::performTask):
(WebCore::CloseWorkerContextTask::isCleanupTask):
(WebCore::WorkerContext::close):
Use new forbidExecution(LetRunningScriptFinish) to avoid termination of script until it executes and exits.
Post a task to actually terminate the worker once the currently executing JS fragment exits.

  • workers/WorkerThread.cpp:

(WebCore::WorkerThread::workerThread):
(WebCore::WorkerThread::stop):
Use new forbidExecution(TerminateRunningScript) to immediately terminate the JS.

LayoutTests:

  • fast/workers/resources/worker-close.js:
  • fast/workers/worker-close-expected.txt:

Updated tests to expect the script fragment that includes close() to run to completion.

  • fast/workers/worker-close-more-expected.txt: Added.
  • fast/workers/worker-close-more.html: Added.
  • fast/workers/worker-close.html:

Added test to check terminate() after close() and close() in the case of multiple MessagePort messages dispatching.

Location:
trunk
Files:
2 added
12 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r57346 r57349  
     12010-04-09  Dmitry Titov  <dimich@chromium.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        WebCore: WorkerGlobalScope.close() should let the currently running script complete execution, then terminate the worker.
     6        https://bugs.webkit.org/show_bug.cgi?id=37053
     7
     8        * fast/workers/resources/worker-close.js:
     9        * fast/workers/worker-close-expected.txt:
     10        Updated tests to expect the script fragment that includes close() to run to completion.
     11
     12        * fast/workers/worker-close-more-expected.txt: Added.
     13        * fast/workers/worker-close-more.html: Added.
     14        * fast/workers/worker-close.html:
     15        Added test to check terminate() after close() and close() in the case of multiple MessagePort messages dispatching.
     16
    1172010-04-09  Kent Tamura  <tkent@chromium.org>
    218

  • trunk/LayoutTests/fast/workers/resources/worker-close.js

    r46830 r57349  
    1818    } else if (evt.data == "close") {
    1919        close();
    20         postMessage("Should not be delivered");
     20        postMessage("Should be delivered");
    2121    } else if (evt.data == "ping") {
    2222        postMessage("pong");
    … …  
    2626        close();
    2727        nonExistentFunction();  // Undefined function - throws exception
     28    } else if (evt.data == "close_post_loop") {
     29        close();
     30        postMessage("closed");
     31        while(true) {} // Should loop forever.
     32    } else if (evt.data == "take_port") {
     33        messagePort = evt.ports[0];
     34        messagePort.onmessage = function(event) {
     35            close();
     36            postMessage("echo_" + event.data);
     37        }
     38    } else if (evt.data == "start_port") {
     39        messagePort.start();
    2840    } else {
    2941        postMessage("FAIL: Unknown message type: " + evt.data);

  • trunk/LayoutTests/fast/workers/worker-close-expected.txt

    r46830 r57349  
    33PASS: typeof close: function
    44PASS: received message before close
    5 PASS: messages sent after close() are ignored
     5PASS: Received message posted right after close() was invoked: Should be delivered
     6PASS: no messages arrive to worker after JS fragment with close() exits
    67PASS: Error arrived after close: ReferenceError: Can't find variable: nonExistentFunction
    78PASS: close() did not dispatch pending events
     9PASS: Received message after worker closed: Should be delivered
    810DONE
    911

  • trunk/LayoutTests/fast/workers/worker-close.html

    r46830 r57349  
    3636        log("FAIL: received unknown response: " + evt.data);
    3737
    38     // Tell the worker to close, then send a followup message
    39     // which should not be delivered.
     38    // Tell the worker to close, then send a followup message. This message should not be delivered,
     39    // since that would require JS to invoke the onmessage handler, which does not happen after the JS
     40    // fragment with 'close()' in it exits. So, the 'ping' should not come back as 'pong'.
    4041    worker.postMessage("close");
     42    worker.postMessage("ping");
    4143    worker.onmessage = function(evt) {
    42         log("FAIL: Received message after worker closed: " + evt.data);
    43         done();
     44        if (evt.data != "pong")
     45            log("PASS: Received message posted right after close() was invoked: " + evt.data);
     46        else {
     47            log("FAIL: Received a message originated from a handler in the worker after the JS fragment with close() exited" + evt.data);
     48            done();
     49        }
    4450    };
    45     // Make sure that messages don't arrive at the remote end - since they
    46     // can't send back response messages, we'll have the worker throw an
    47     // exception instead (errors are still propagated to the caller even after
    48     // close()).
    49     worker.postMessage("throw");
    50     worker.onerror = function(evt) {
    51         log("FAIL: message delivered after close(): " + evt.message);
    52         done();
    53     }
    5451    timeout = setTimeout(testErrorAfterClose, 1000);
    5552}
    … …  
    5754function testErrorAfterClose()
    5855{
    59     log("PASS: messages sent after close() are ignored");
     56    log("PASS: no messages arrive to worker after JS fragment with close() exits");
    6057    // Test that errors are delivered after close.
    6158    worker = new Worker('resources/worker-close.js');
    … …  
    8784{
    8885    log("PASS: close() did not dispatch pending events");
    89     worker = new Worker('resources/worker-class.js');
     86    worker = new Worker('resources/worker-close.js');
    9087    worker.postMessage("close");
    9188    worker.onmessage = function(evt) {
    92         log("FAIL: Received message after worker closed: " + evt.data);
    93         done();
     89        log("PASS: Received message after worker closed: " + evt.data);
    9490    };
    9591    // Give worker a chance to close first, then terminate it.

  • trunk/WebCore/ChangeLog

    r57346 r57349  
     12010-04-09  Dmitry Titov  <dimich@chromium.org>
     2
     3        Reviewed by Darin Adler.
     4
     5        WebCore: WorkerGlobalScope.close() should let the currently running script complete execution, then terminate the worker.
     6        https://bugs.webkit.org/show_bug.cgi?id=37053
     7
     8        Test: fast/workers/worker-close-more.html
     9
     10        * bindings/js/WorkerScriptController.cpp:
     11        (WebCore::WorkerScriptController::forbidExecution):
     12        (WebCore::WorkerScriptController::):
     13        * bindings/v8/WorkerScriptController.cpp:
     14        (WebCore::WorkerScriptController::forbidExecution):
     15        * bindings/v8/WorkerScriptController.h:
     16        (WebCore::WorkerScriptController::):
     17        Added option parameter to forbidExecution (both JCS and V8 versions) that specifies whether currently running
     18        script should be immediately terminated or left executed until the end.
     19
     20        * bindings/js/WorkerScriptController.h:
     21        (WebCore::WorkerScriptController::workerContextWrapper):
     22        This method now can return 0 instead of context if the further execution of JS is forbidden. Since any JS execution requires
     23        fetching JS global object first, returning 0 here is a good way to prevent re-entry into JS once worker started termination.
     24        V8 version does similar thing already in WorkerScriptController::proxy().
     25
     26        * workers/DedicatedWorkerContext.cpp:
     27        (WebCore::DedicatedWorkerContext::postMessage):
     28        Removed check that immediately disables postMessage from WorkerContext after close().
     29
     30        * workers/WorkerContext.cpp:
     31        (WebCore::CloseWorkerContextTask::create):
     32        (WebCore::CloseWorkerContextTask::performTask):
     33        (WebCore::CloseWorkerContextTask::isCleanupTask):
     34        (WebCore::WorkerContext::close):
     35        Use new forbidExecution(LetRunningScriptFinish) to avoid termination of script until it executes and exits.
     36        Post a task to actually terminate the worker once the currently executing JS fragment exits.
     37
     38        * workers/WorkerThread.cpp:
     39        (WebCore::WorkerThread::workerThread):
     40        (WebCore::WorkerThread::stop):
     41        Use new forbidExecution(TerminateRunningScript) to immediately terminate the JS.
     42
    1432010-04-09  Kent Tamura  <tkent@chromium.org>
    244

  • trunk/WebCore/bindings/js/WorkerScriptController.cpp

    r57192 r57349  
    137137}
    138138
    139 void WorkerScriptController::forbidExecution()
     139void WorkerScriptController::forbidExecution(ForbidExecutionOption option)
    140140{
    141     // This function is called from another thread.
     141    // This function may be called from another thread.
    142142    // Mutex protection for m_executionForbidden is needed to guarantee that the value is synchronized between processors, because
    143143    // if it were not, the worker could re-enter JSC::evaluate(), but with timeout already reset.
    … …  
    145145    MutexLocker lock(m_sharedDataMutex);
    146146    m_executionForbidden = true;
    147     m_globalData->terminator.terminateSoon();
     147    if (option == TerminateRunningScript)
     148        m_globalData->terminator.terminateSoon();
    148149}
    149150

  • trunk/WebCore/bindings/js/WorkerScriptController.h

    r49963 r57349  
    5353        JSWorkerContext* workerContextWrapper()
    5454        {
     55            if (m_executionForbidden)
     56                return 0;
     57
    5558            initScriptIfNeeded();
    5659            return m_workerContextWrapper;
    … …  
    6265        void setException(ScriptValue);
    6366
    64         void forbidExecution();
     67        enum ForbidExecutionOption { TerminateRunningScript, LetRunningScriptFinish };
     68        void forbidExecution(ForbidExecutionOption);
    6569
    6670        JSC::JSGlobalData* globalData() { return m_globalData.get(); }

  • trunk/WebCore/bindings/v8/WorkerScriptController.cpp

    r56580 r57349  
    8787}
    8888
    89 void WorkerScriptController::forbidExecution()
     89void WorkerScriptController::forbidExecution(ForbidExecutionOption option)
    9090{
    91     // This function is called from another thread.
     91    // This function may be called from another thread.
    9292    MutexLocker lock(m_sharedDataMutex);
    9393    m_executionForbidden = true;
    94     v8::V8::TerminateExecution();
     94    if (option == TerminateRunningScript)
     95        v8::V8::TerminateExecution();
    9596}
    9697

  • trunk/WebCore/bindings/v8/WorkerScriptController.h

    r56580 r57349  
    5656        void setException(ScriptValue);
    5757
    58         void forbidExecution();
     58        enum ForbidExecutionOption { TerminateRunningScript, LetRunningScriptFinish };
     59        void forbidExecution(ForbidExecutionOption);
     60
    5961        // Returns WorkerScriptController for the currently executing context. 0 will be returned if the current executing context is not the worker context.
    6062        static WorkerScriptController* controllerForContext();

  • trunk/WebCore/workers/DedicatedWorkerContext.cpp

    r57068 r57349  
    6363void DedicatedWorkerContext::postMessage(PassRefPtr<SerializedScriptValue> message, const MessagePortArray* ports, ExceptionCode& ec)
    6464{
    65     if (isClosing())
    66         return;
    6765    // Disentangle the port in preparation for sending it to the remote context.
    6866    OwnPtr<MessagePortChannelArray> channels = MessagePort::disentanglePorts(ports, ec);

  • trunk/WebCore/workers/WorkerContext.cpp

    r57210 r57349  
    6161namespace WebCore {
    6262
     63class CloseWorkerContextTask : public ScriptExecutionContext::Task {
     64public:
     65    static PassOwnPtr<CloseWorkerContextTask> create()
     66    {
     67        return new CloseWorkerContextTask;
     68    }
     69
     70    virtual void performTask(ScriptExecutionContext *context)
     71    {
     72        ASSERT(context->isWorkerContext());
     73        WorkerContext* workerContext = static_cast<WorkerContext*>(context);
     74        // Notify parent that this context is closed. Parent is responsible for calling WorkerThread::stop().
     75        workerContext->thread()->workerReportingProxy().workerContextClosed();
     76    }
     77
     78    virtual bool isCleanupTask() const { return true; }
     79};
     80
    6381WorkerContext::WorkerContext(const KURL& url, const String& userAgent, WorkerThread* thread)
    6482    : m_url(url)
    … …  
    125143
    126144    m_closing = true;
    127     // Notify parent that this context is closed. Parent is responsible for calling WorkerThread::stop().
    128     thread()->workerReportingProxy().workerContextClosed();
     145    // Let current script run to completion but prevent future script evaluations.
     146    m_script->forbidExecution(WorkerScriptController::LetRunningScriptFinish);
     147    postTask(CloseWorkerContextTask::create());
    129148}
    130149

  • trunk/WebCore/workers/WorkerThread.cpp

    r55896 r57349  
    122122            // The worker was terminated before the thread had a chance to run. Since the context didn't exist yet,
    123123            // forbidExecution() couldn't be called from stop().
    124            m_workerContext->script()->forbidExecution();
     124           m_workerContext->script()->forbidExecution(WorkerScriptController::TerminateRunningScript);
    125125        }
    126126    }
    … …  
    219219    // 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.
    220220    if (m_workerContext) {
    221         m_workerContext->script()->forbidExecution();
     221        m_workerContext->script()->forbidExecution(WorkerScriptController::TerminateRunningScript);
    222222
    223223    // FIXME: Rudely killing the thread won't work when we allow nested workers, because they will try to post notifications of their destruction.
Note: See TracChangeset for help on using the changeset viewer.