[webkit-reviews] review denied: [Bug 88601] Worker tear-down can re-enter JSC during GC finalization pt. 2 : [Attachment 147149] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 12 17:14:49 PDT 2012


David Levin <levin at chromium.org> has denied  review:
Bug 88601: Worker tear-down can re-enter JSC during GC finalization pt. 2
https://bugs.webkit.org/show_bug.cgi?id=88601

Attachment 147149: Patch
https://bugs.webkit.org/attachment.cgi?id=147149&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=147149&action=review


Alexey is very familiar with the code since he was the original author. The
rest of us just came along and added to it (aka made it more complicated :(  ).


> Source/WebCore/workers/WorkerMessagingProxy.cpp:377
> +    // Executes workerObjectDestroyedInternal() on parent context's thread.

This comment is misleading because this method is executed on the Worker Object
thread already. 

All this does is delay the object destruction.

I strongly believe that "m_workerObject = 0;" should be set here as the
WorkObject is done when this method returns.

> Source/WebCore/workers/WorkerMessagingProxy.cpp:378
> +   
m_scriptExecutionContext->postTask(WorkerObjectDestroyedTask::create(this));

Personally I prefer using createCallbackTask which would have less boilerplate
code. (Much of the code in here predates that and in turn inspired it.)

For example see: WorkerMessagingProxy::postConsoleMessageToWorkerObject

You'd have to make WorkerMessagingProxy::workerObjectDestroyedInternal static
and take a WorkerMessagingProxy.

> Source/WebCore/workers/WorkerMessagingProxy.h:96
> +	   friend class WorkerObjectDestroyedTask;

Sort ideally (since ordering doesn't matter).


More information about the webkit-reviews mailing list