Bug 25902 – Need to implement WorkerContext.close()

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

[Andrew Wilson]

There's still some argument about whether we need to support close events on Worker and WorkerContext arguments, since they expose information about garbage collection.

However, we should still support an explicit WorkerContext.close() API to allow workers to shut themselves down.

[Andrew Wilson]

Created attachment 30590 [details]
Implementation of WorkerContext.close() + layout test

[Eric Seidel (no email)]

Comment on attachment 30590 [details]
Implementation of WorkerContext.close() + layout test

Looks good to me.  I think the test would be slightly more clear if you called worker.postMessge('ping') *before* calling close, to show that the ping worked before the close.

This looks fine though.  Marking r+, someone else will have to land this though because I'm about to get on a plane.

[Alexey Proskuryakov]

Comment on attachment 30590 [details]
Implementation of WorkerContext.close() + layout test

Looks like this has a race condition in the following situation:
1) Worker thread decides to close itself, calling stop(), and subsequently destroying WorkerContext object, notifying the proxy, and exiting the thread itself.
2) At the same time, a message is sent from the main thread to the worker thread. Since it takes time to deliver a workerContextDestroyed notification, this message will be added to deleted WorkerContext's queue.

I think that the right way to implement close() is send a custom message to the proxy, asking it to terminate the context in response, and to wait for this in some "death row" state that prevents any other communication with the proxy.

Please add a test for this situation (if at all possible, one that would crash with this patch). Please also add a test for a situation where the worker decides to close itself from an XHR onreadystatechange callback, not in response to a message from the main thread. It would be nice to verify that this doesn't introduce leaks.

[Andrew Wilson]

I'll add the requested tests.

I'm not certain that there's a race condition - the reason is that the event queue for the worker (WorkerRunLoop) is associated with the WorkerThread, not with the WorkerContext. So you can still safely invoke m_workerThread->runLoop().postTask() from postMessageToWorkerContext() even after the WorkerContext itself has gone away. There also should be no leak, as once the WorkerThread is freed, the WorkerRunLoop and any events left on the queue will also be freed (note that the MessageQueue may have some items left on it anyway, since MessageQueue::kill doesn't empty the queue).

[Alexey Proskuryakov]

Indeed, I was wrong about the race condition.

Does this implementation of close() do what the spec says? Omitting MessagePort related steps, it is:
1) Queue a task to fire a simple event called close at the WorkerGlobalScope object.
2) Set the worker's WorkerGlobalScope object's closing flag to true (which means that new tasks do not get added to event loop, but those already in the loop will be processed).

[Andrew Wilson]

In the first case, no, we don't fire off a close event - the reason why is because I'm still working to convince IanH that we should remove the close event from WorkerGlobalScope (the reason is that we don't want to expose the internal behavior of garbage collection via the close event, and generating a close event *only* in response to an explicit close() invocation seems of dubious utility).

Thanks for pointing out the spec language around proper handling of events that are already in the queue - I missed that behavior since it's described in a different part of the spec.

It seems like the best way to do that is to have close send an event via the queue which itself calls WorkerThread::stop(). Any timer/XHR events that come in after that event will get dropped. Do you see any problems with that approach?

[Alexey Proskuryakov]

Sounds good to me.

[Andrew Wilson]

Following up with this - turns out that just queueing up a task to invoke close() won't work. The reason is that WorkerRunLoop does not guarantee ordering in the dispatch of pending events (MessageQueue.waitForMessageFilteredWithTimeout() will only return MessageQueueTimeout if the queue is empty, meaning that timer events are only fired if/when the queue is empty.

I'm discussing with ianh whether we should change close() to just drop pending events (if a worker wants different functionality, it could just use setTimeout(close, 0);

[Andrew Wilson]

Created attachment 30752 [details]
proposed patch with requested changes

Updated patch with requested tests. The latest spec for WorkerContext.close() discards all pending events, so I've updated the code and tests to reflect this.

Note that there's a minor issue with pending XMLHttpRequest events - those aren't always delivered via the event queue, so if you call WorkerContext.close() from an XHR callback you can still get another event callback invoked before the thread exits. I'll log a separate bug for this.

[Darin Adler]

Comment on attachment 30752 [details]
proposed patch with requested changes

I normally prefer early return to nesting work inside if statements. So in close and postMessage we'd put:

    if (m_closing)
        return;

At the start of the function, and then go on with the work of the function.

I much prefer change logs with per-function comments.

Given Alexey's previous comments this looks good. r=me

[Adam Barth]

Will land.

[Adam Barth]

Sending        LayoutTests/ChangeLog
Adding         LayoutTests/fast/workers/resources/worker-close.js
Sending        LayoutTests/fast/workers/resources/worker-common.js
Adding         LayoutTests/fast/workers/worker-close-expected.txt
Adding         LayoutTests/fast/workers/worker-close.html
Adding         LayoutTests/http/tests/xmlhttprequest/workers/close-expected.txt
Adding         LayoutTests/http/tests/xmlhttprequest/workers/close.html
Adding         LayoutTests/http/tests/xmlhttprequest/workers/resources/close.js
Sending        WebCore/ChangeLog
Sending        WebCore/workers/WorkerContext.cpp
Sending        WebCore/workers/WorkerContext.h
Sending        WebCore/workers/WorkerContext.idl
Sending        WebCore/workers/WorkerMessagingProxy.cpp
Transmitting file data .............
Committed revision 44319.