Bug 28702 – Workers do not exit after async operations

• Status: NEW
• Component: WebCore JavaScript

[Andrew Wilson]

The dedicated worker code has a mechanism in place where DedicatedWorkerContext reports pending activity back to the parent Worker object every time it receives a message. The idea is that if the parent worker object is no longer reachable, and the worker thread goes idle, WebKit will shut down the worker thread because it has been orphaned.

The problem is that this mechanism (WorkerMessagingProxy::reportPendingActivity()) only triggers when a message is received - if the worker performs some async operation, there's nothing to notify the parent worker object when the async operation completes and the worker goes idle again unless another message is sent to the worker.

The net result is (effectively) a memory leak - the worker context hangs around in an unreachable state until the parent page closes, because it appears that the page always has pending activity.

BTW - the HTML5 spec does not make any guarantees about when an unreachable Worker is shut down - once a Worker is unreachable, it's up to the UA to decide whether to shut it down or not.

[Alexey Proskuryakov]

I think that we had code that was supposed to prevent this; some of that was apparently removed in <http://trac.webkit.org/changeset/46507#file6>. I'm not sure if we ever had this completely right though.

[Andrew Wilson]

(In reply to comment #1)
> I think that we had code that was supposed to prevent this; some of that was
> apparently removed in <http://trac.webkit.org/changeset/46507#file6>. I'm not
> sure if we ever had this completely right though.

That code wasn't removed - it was just moved out of WorkerScriptController (since it's not needed for SharedWorkers) and into DedicatedWorkerContext.

The original mechanism to shutdown orphaned workers still works - the problem is that when we added async APIs to WorkerGlobalScope (setTimeout(), XHR) we didn't add any additional mechanism to report pending activity when those async processes ended.

[Alexey Proskuryakov]

(In reply to comment #2)
> That code wasn't removed - it was just moved out of WorkerScriptController
> (since it's not needed for SharedWorkers) and into DedicatedWorkerContext.

The code reported pending activity after any JavaScript evaluation - but in its new place, it only reports after importScripts().

[Andrew Wilson]

(In reply to comment #3)
> (In reply to comment #2)
> > That code wasn't removed - it was just moved out of WorkerScriptController
> > (since it's not needed for SharedWorkers) and into DedicatedWorkerContext.
> 
> The code reported pending activity after any JavaScript evaluation - but in its
> new place, it only reports after importScripts().

When I looked at the code, it did not appear to me that XHR and setTimeout() callbacks were routed through WorkerScriptController. Only the initial script load/evaluation went through that code - async callbacks went through the normal message loop event processing mechanism.

If this is incorrect, I'll update the code to do the right thing - just point me at where setTimeout handlers invoke WorkerScriptController::evaluate(). I only found two calls to evaluate() - in WorkerThread::workerThread (initial worker script load) and in WorkerContext::importScripts() so I don't see how pending activity could have been reported in these other cases because WorkerScriptController isn't involved.

[Andrew Wilson]

After talking with ap on IRC:

1) The worker code refactoring broke setTimeout("code to evaluate") - ScheduledAction.cpp invokes scriptController->evaluate(), and we no longer call reportPendingActivity() from there (responsibility for doing this was moved to the caller).

2) The setTimeout(function() {...}) code was already broken, since it never went through WorkerScriptController.

3) XHR callbacks have always been broken AFAIK since they just invoke their listener code directly and have never gone through WorkerScriptController.

It seems like the right solution is to have WorkerRunLoop::run() report pending activity after each event is processed - any other solution seems fragile since it requires extra work every time we add a new async operation (DB transactions, appcache notifications, etc).