Invoke WorkerScriptLoader's m_finishedCallback callback safely.

Source/core/workers/WorkerScriptLoader.cpp

 /*
  * Copyright (C) 2009 Apple Inc. All Rights Reserved.
  * Copyright (C) 2009, 2011 Google Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
@@ skipping 29 matching lines @@
 #include "wtf/RefPtr.h"
 
 namespace blink {
 
 WorkerScriptLoader::WorkerScriptLoader()
     : m_responseCallback(nullptr)
     , m_finishedCallback(nullptr)
     , m_failed(false)
     , m_identifier(0)
     , m_appCacheID(0)
-    , m_finishing(false)
     , m_requestContext(WebURLRequest::RequestContextWorker)
 {
 }
 
 WorkerScriptLoader::~WorkerScriptLoader()
 {
 }
 
 void WorkerScriptLoader::loadSynchronously(ExecutionContext& executionContext, const KURL& url, CrossOriginRequestPolicy crossOriginRequestPolicy)
 {
@@ skipping 27 matching lines @@
     if (!request)
         return;
 
     ThreadableLoaderOptions options;
     options.crossOriginRequestPolicy = crossOriginRequestPolicy;
 
     ResourceLoaderOptions resourceLoaderOptions;
     resourceLoaderOptions.allowCredentials = AllowStoredCredentials;
 
     m_threadableLoader = ThreadableLoader::create(executionContext, this, *request, options, resourceLoaderOptions);
+    if (m_failed)
+        notifyFinished();
+    // Do nothing here since notifyFinished() could delete |this|.
 }
 
 const KURL& WorkerScriptLoader::responseURL() const
 {
     ASSERT(!failed());
     return m_responseURL;
 }
 
 PassOwnPtr<ResourceRequest> WorkerScriptLoader::createResourceRequest()
 {
@@ skipping 63 matching lines @@
 }
 
 void WorkerScriptLoader::didFailRedirectCheck()
 {
     notifyError();
 }
 
 void WorkerScriptLoader::notifyError()
 {
     m_failed = true;
-    notifyFinished();
+    // notifyError() could be called before ThreadableLoader::create() returns
+    // e.g. from didFail(), and in that case m_threadableLoader is not yet set
+    // (i.e. still null).
+    // Since the callback invocation in notifyFinished() potentially delete

[haraken, 2015/07/02 06:18:06]

Another way to avoid this would be to make WorkerScriptLoader RefCounted and
protect |this|. I'm not sure which is better -- deferring the decision to
kinuko-san.

[Takashi Toyoshima, 2015/07/02 07:33:56]

Yep, originally this class was RefCounted, and I changed it in
https://codereview.chromium.org/1190133002. That's why this regression happens
:(

I prefer to stop using RefCounted as possible since using RefCounted makes
object lifecycle management vague. But I'll follow reviewer's final decision.

+    // |this| object, the callback invocation should be postponed until the
+    // create() call returns. See loadAsynchronously() for the postponed call.
+    if (m_threadableLoader)
+        notifyFinished();
 }
 
 void WorkerScriptLoader::cancel()
 {
     if (m_threadableLoader)
         m_threadableLoader->cancel();
 }
 
 String WorkerScriptLoader::script()
 {
     return m_script.toString();
 }
 
 void WorkerScriptLoader::notifyFinished()

[haraken, 2015/07/02 06:18:06]

If this method is invoked only in a case where some failure occurs, we might
want to rename the method to notifyFailed or something like that.

[Takashi Toyoshima, 2015/07/02 07:33:56]

Good point.
Maybe we should use notifyFinished() even in didFinishLoading() to invoke
m_finishedCallback and make this method to be consistent with the method name.
That would also contribute to remove redundant code.
Also, having notifyError() as a public method looks curious. I'd make some
changes around these points in the next patch set.

 {
-    if (!m_finishedCallback || m_finishing)
+    ASSERT(m_failed);
+    if (!m_finishedCallback)
         return;
 
-    m_finishing = true;
-    if (m_finishedCallback)
-        (*m_finishedCallback)();
+    OwnPtr<Closure> callback = m_finishedCallback.release();
+    (*callback)();
 }
 
 void WorkerScriptLoader::processContentSecurityPolicy(const ResourceResponse& response)
 {
     // Per http://www.w3.org/TR/CSP2/#processing-model-workers, if the Worker's
     // URL is not a GUID, then it grabs its CSP from the response headers
     // directly.  Otherwise, the Worker inherits the policy from the parent
     // document (which is implemented in WorkerMessagingProxy, and
     // m_contentSecurityPolicy should be left as nullptr to inherit the policy).
     if (!response.url().protocolIs("blob") && !response.url().protocolIs("file") && !response.url().protocolIs("filesystem")) {
         m_contentSecurityPolicy = ContentSecurityPolicy::create();
         m_contentSecurityPolicy->setOverrideURLForSelf(response.url());
         m_contentSecurityPolicy->didReceiveHeaders(ContentSecurityPolicyResponseHeaders(response));
     }
 }
 
 } // namespace blink