Bug 88601 – Worker tear-down can re-enter JSC during GC finalization pt. 2

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

[Mark Hahnenberg]

Filing a separate bug for ~Worker as described in bug 88449.

[Mark Hahnenberg]

Created attachment 146441 [details]
Patch

[Geoffrey Garen]

Comment on attachment 146441 [details]
Patch

Pretty hard to tell whether this code is correct :(.

[Mark Hahnenberg]

(In reply to comment #2)
> (From update of attachment 146441 [details])
> Pretty hard to tell whether this code is correct :(.

According to the Chromium EWS bot, it's not correct :-(

[Adam Barth]

Comment on attachment 146441 [details]
Patch

Yeah, looks like this is causing a bunch of flaky crashes.

[Mark Hahnenberg]

Created attachment 147149 [details]
Patch

[Geoffrey Garen]

Comment on attachment 147149 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147149&action=review

r=me

I'm happy if the Chromium EWS is happy.

> Source/WebCore/ChangeLog:8
> +        No new tests. 

Please explain why.

[Geoffrey Garen]

Should we close bug 88823 now?

[Mark Hahnenberg]

(In reply to comment #7)
> Should we close bug 88823 now?

I want to get Dave Levin's input on this patch since he seems to be the most familiar with how all this Worker stuff interacts. If this patch seems good to him, we can RESOLVED WONTFIX the other one.

[David Levin]

Comment on attachment 147149 [details]
Patch

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

[David Levin]

r- due to misplacement of "m_workerObject = 0;"

[Geoffrey Garen]

> r- due to misplacement of "m_workerObject = 0;"

Eagerly setting m_workerObject = 0 causes the proxy object to delete itself while it still has a pending destroy message, causing a crash.

Do you have a suggestion for an alternative?

[David Levin]

(In reply to comment #11)
> > r- due to misplacement of "m_workerObject = 0;"
> 
> Eagerly setting m_workerObject = 0 causes the proxy object to delete itself while it still has a pending destroy message, causing a crash.

This is in workerObjectDestroyed which is the method called from ~Worker (Object). If m_workerObject isn't set to 0, then you're accessing freed memory which will cause a corruption.

Note that WorkerMessagingProxy doesn't hold a reference to Worker which is good because Worker basically owns WorkerMessagingProxy.

> 
> Do you have a suggestion for an alternative?

I've discussed this a lot with Mark during the day. I though setting m_workObject was fine here.  Anyway he should have a pretty solid grasp on this now. I can be available on irc to discuss as needed (tomorrow).

[Mark Hahnenberg]

Created attachment 147467 [details]
Patch

[David Levin]

Comment on attachment 147467 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147467&action=review

Consider changing "can" to "may" (not critical but nice).

> Source/WebCore/workers/WorkerMessagingProxy.cpp:357
>      m_workerObject = 0;

What about setting m_askedToTerminate?

Actually.... I think I'm ok with not setting that here. I think it will be ok since the exact moment on workerObjectDestruction isn't really exposed anywhere.

> Source/WebCore/workers/WorkerMessagingProxy.cpp:363
> +    proxy->m_canBeDestroyed = true;

may is more correct than can (imo of course).

[Mark Hahnenberg]

Patch landed in http://trac.webkit.org/changeset/120328