Bug 88601 - Worker tear-down can re-enter JSC during GC finalization pt. 2
Summary: Worker tear-down can re-enter JSC during GC finalization pt. 2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-07 18:20 PDT by Mark Hahnenberg
Modified: 2012-06-14 13:19 PDT (History)
5 users (show)

See Also:


Attachments
Patch (4.23 KB, patch)
2012-06-07 18:28 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (3.76 KB, patch)
2012-06-12 13:49 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (4.80 KB, patch)
2012-06-13 19:06 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 2012-06-07 18:20:28 PDT
Filing a separate bug for ~Worker as described in bug 88449.
Comment 1 Mark Hahnenberg 2012-06-07 18:28:29 PDT
Created attachment 146441 [details]
Patch
Comment 2 Geoffrey Garen 2012-06-07 22:42:08 PDT
Comment on attachment 146441 [details]
Patch

Pretty hard to tell whether this code is correct :(.
Comment 3 Mark Hahnenberg 2012-06-08 09:06:18 PDT
(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 :-(
Comment 4 Adam Barth 2012-06-08 11:34:11 PDT
Comment on attachment 146441 [details]
Patch

Yeah, looks like this is causing a bunch of flaky crashes.
Comment 5 Mark Hahnenberg 2012-06-12 13:49:46 PDT
Created attachment 147149 [details]
Patch
Comment 6 Geoffrey Garen 2012-06-12 14:04:29 PDT
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.
Comment 7 Geoffrey Garen 2012-06-12 14:04:57 PDT
Should we close bug 88823 now?
Comment 8 Mark Hahnenberg 2012-06-12 14:06:40 PDT
(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.
Comment 9 David Levin 2012-06-12 17:14:49 PDT
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).
Comment 10 David Levin 2012-06-12 17:15:15 PDT
r- due to misplacement of "m_workerObject = 0;"
Comment 11 Geoffrey Garen 2012-06-12 22:39:25 PDT
> 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?
Comment 12 David Levin 2012-06-12 23:37:59 PDT
(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).
Comment 13 Mark Hahnenberg 2012-06-13 19:06:12 PDT
Created attachment 147467 [details]
Patch
Comment 14 David Levin 2012-06-13 19:39:13 PDT
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).
Comment 15 Mark Hahnenberg 2012-06-14 13:18:12 PDT
Patch landed in http://trac.webkit.org/changeset/120328