Filing a separate bug for ~Worker as described in bug 88449.
Created attachment 146441 [details] Patch
Comment on attachment 146441 [details] Patch Pretty hard to tell whether this code is correct :(.
(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 on attachment 146441 [details] Patch Yeah, looks like this is causing a bunch of flaky crashes.
Created attachment 147149 [details] Patch
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.
Should we close bug 88823 now?
(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 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).
r- due to misplacement of "m_workerObject = 0;"
> 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?
(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).
Created attachment 147467 [details] Patch
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).
Patch landed in http://trac.webkit.org/changeset/120328