Invoke WorkerScriptLoader's m_finishedCallback callback safely.

Source/core/workers/WorkerScriptLoader.cpp

Index: Source/core/workers/WorkerScriptLoader.cpp
diff --git a/Source/core/workers/WorkerScriptLoader.cpp b/Source/core/workers/WorkerScriptLoader.cpp
index 15dee1bffecaf174eb8588d8fefcd8e87308edb9..89e190b472133b86c1ed37938fed9675b349c92d 100644
--- a/Source/core/workers/WorkerScriptLoader.cpp
+++ b/Source/core/workers/WorkerScriptLoader.cpp
@@ -47,7 +47,6 @@ WorkerScriptLoader::WorkerScriptLoader()
     , m_failed(false)
     , m_identifier(0)
     , m_appCacheID(0)
-    , m_finishing(false)
     , m_requestContext(WebURLRequest::RequestContextWorker)
 {
 }
@@ -95,6 +94,9 @@ void WorkerScriptLoader::loadAsynchronously(ExecutionContext& executionContext,
     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
@@ -178,7 +180,14 @@ void WorkerScriptLoader::didFailRedirectCheck()
 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()
@@ -194,12 +203,12 @@ String WorkerScriptLoader::script()
 
 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)