Script streaming: Add an option to make the main thread block (wait for parsing)

Source/bindings/core/v8/ScriptStreamer.cpp

Index: Source/bindings/core/v8/ScriptStreamer.cpp
diff --git a/Source/bindings/core/v8/ScriptStreamer.cpp b/Source/bindings/core/v8/ScriptStreamer.cpp
index 3b1f6f73c1ae8f4aead4888161dd1c381fe8a364..2a2dda9432b870bf6b4f8edc56902f22c37a0b93 100644
--- a/Source/bindings/core/v8/ScriptStreamer.cpp
+++ b/Source/bindings/core/v8/ScriptStreamer.cpp
@@ -224,28 +224,20 @@ void ScriptStreamer::startStreaming(PendingScript& script, Settings* settings, S
         blink::Platform::current()->histogramEnumeration(startedStreamingHistogramName(scriptType), 0, 2);
 }
 
-void ScriptStreamer::streamingComplete()
+void ScriptStreamer::streamingCompleteOnBackgroundThread()
 {
-    ASSERT(isMainThread());
-    // It's possible that the corresponding Resource was deleted before V8
-    // finished streaming. In that case, the data or the notification is not
-    // needed. In addition, if the streaming is suppressed, the non-streaming
-    // code path will resume after the resource has loaded, before the
-    // background task finishes.
-    if (m_detached || m_streamingSuppressed) {
-        deref();
-        return;
-    }
-
-    // We have now streamed the whole script to V8 and it has parsed the
-    // script. We're ready for the next step: compiling and executing the
-    // script.
+    ASSERT(!isMainThread());
+    MutexLocker locker(m_mutex);
     m_parsingFinished = true;
-
-    notifyFinishedToClient();
-
-    // The background thread no longer holds an implicit reference.
-    deref();
+    if (m_blockMainThreadAfterLoading) {
+        // Normally, the main thread is waiting at this point, but it can also
+        // happen that the load is not yet finished (e.g., a parse error). In
+        // that case, notifyFinished will be called eventually and it will not
+        // wait on m_parsingFinishedCondition.
+        m_parsingFinishedCondition.signal();
+    } else {
+        callOnMainThread(WTF::bind(&ScriptStreamer::streamingComplete, this));
+    }
 }
 
 void ScriptStreamer::cancel()
@@ -262,6 +254,7 @@ void ScriptStreamer::cancel()
 
 void ScriptStreamer::suppressStreaming()
 {
+    MutexLocker locker(m_mutex);
     ASSERT(!m_parsingFinished);
     ASSERT(!m_loadingFinished);
     m_streamingSuppressed = true;
@@ -298,7 +291,8 @@ void ScriptStreamer::notifyAppendData(ScriptResource* resource)
         ASSERT(m_task);
         // ScriptStreamer needs to stay alive as long as the background task is
         // running. This is taken care of with a manual ref() & deref() pair;
-        // the corresponding deref() is in streamingComplete.
+        // the corresponding deref() is in streamingComplete or in
+        // notifyFinished.
         ref();
         ScriptStreamingTask* task = new ScriptStreamingTask(m_task.release(), this);
         ScriptStreamerThread::shared()->postTask(task);
@@ -318,10 +312,32 @@ void ScriptStreamer::notifyFinished(Resource* resource)
         suppressStreaming();
     m_stream->didFinishLoading();
     m_loadingFinished = true;
+
+    if (m_blockMainThreadAfterLoading) {
+        // Make the main thead wait until the streaming is complete, to make
+        // sure that the script gets the main thread's attention as early as
+        // possible (for possible compiling, if the client wants to do it
+        // right away). Note that blocking here is not any worse than the
+        // non-streaming code path where the main thread eventually blocks
+        // to parse the script.
+        MutexLocker locker(m_mutex);
+        if (!isFinished()) {

[Sami, 2014/10/15 13:11:51]

Should this be a while loop to guard against spurious wakeups?

[marja, 2014/10/24 13:04:40]

Hmm, dunno, it's a bit of overly defensive programming; m_loadingFinished is set
to true here, and m_parsingFinished is set to true before the cond var is
signaled, so, there shouldn't be other wakeups.

[Sami, 2014/10/24 13:15:35]

I think we're using pthread_cond_wait under the hood, which can sometimes return
without being signaled because of the way it's implemented. I'd suggest making
this loop to be safe.

See here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/synchronizati...

[marja, 2014/10/24 13:19:29]

Oh wow. I wasn't aware of that. TIL. Fixed in the new version.

+            m_parsingFinishedCondition.wait(m_mutex);
+        }
+        ASSERT(isFinished());
+    }
+
     notifyFinishedToClient();
+
+    if (m_blockMainThreadAfterLoading && m_parsingFinished) {
+        // streamingComplete won't be called, so do the ramp-down work
+        // here. Since m_parsingFinished is true, we know that there was a
+        // background task and we need to deref().
+        deref();
+    }
 }
 
-ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::StreamedSource::Encoding encoding, PendingScript::Type scriptType)
+ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::StreamedSource::Encoding encoding, PendingScript::Type scriptType, bool blockMainThreadAfterLoading)
     : m_resource(resource)
     , m_detached(false)
     , m_stream(new SourceStream(this))
@@ -332,7 +348,35 @@ ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::Str
     , m_firstDataChunkReceived(false)
     , m_streamingSuppressed(false)
     , m_scriptType(scriptType)
+    , m_blockMainThreadAfterLoading(blockMainThreadAfterLoading)
+{
+}
+
+void ScriptStreamer::streamingComplete()
 {
+    // The background task is completed; do the necessary ramp-down in the main
+    // thread.
+    ASSERT(isMainThread());
+    // In the blocking mode, the ramp-down is done in notifyFinished.
+    ASSERT(!m_blockMainThreadAfterLoading);
+
+    // It's possible that the corresponding Resource was deleted before V8
+    // finished streaming. In that case, the data or the notification is not
+    // needed. In addition, if the streaming is suppressed, the non-streaming
+    // code path will resume after the resource has loaded, before the
+    // background task finishes.
+    if (m_detached || m_streamingSuppressed) {
+        deref();
+        return;
+    }
+
+    // We have now streamed the whole script to V8 and it has parsed the
+    // script. We're ready for the next step: compiling and executing the
+    // script.
+    notifyFinishedToClient();
+
+    // The background thread no longer holds an implicit reference.
+    deref();
 }
 
 void ScriptStreamer::notifyFinishedToClient()
@@ -346,6 +390,7 @@ void ScriptStreamer::notifyFinishedToClient()
     // function calling notifyFinishedToClient was already scheduled in the task
     // queue and the upper layer decided that it's not interested in the script
     // and called removeClient.
+    MutexLocker locker(m_mutex);
     if (isFinished() && m_client)
         m_client->notifyFinished(m_resource);
 }
@@ -415,7 +460,7 @@ bool ScriptStreamer::startStreamingInternal(PendingScript& script, Settings* set
     // The Resource might go out of scope if the script is no longer needed. We
     // will soon call PendingScript::setStreamer, which makes the PendingScript
     // notify the ScriptStreamer when it is destroyed.
-    RefPtr<ScriptStreamer> streamer = adoptRef(new ScriptStreamer(resource, encoding, scriptType));
+    RefPtr<ScriptStreamer> streamer = adoptRef(new ScriptStreamer(resource, encoding, scriptType, settings->v8ScriptStreamingBlocking()));
 
     // Decide what kind of cached data we should produce while streaming. By
     // default, we generate the parser cache for streamed scripts, to emulate