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

This reverts r184277.

Reason: ASSERTS fail in debug mode because ScriptStreamer gets destroyed while
holding the mutex.

BUG=
TBR=marja@chromium.org, haraken@chromium.org
NOTRY=true

Review URL: https://codereview.chromium.org/680493002

git-svn-id: svn://svn.chromium.org/blink/trunk@184349 bbb929c8-8fbe-4397-9dbb-9b2b20218538

third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp

@@ -13,7 +13,6 @@
 #include "core/fetch/ScriptResource.h"
 #include "core/frame/Settings.h"
 #include "platform/SharedBuffer.h"
-#include "platform/TraceEvent.h"
 #include "public/platform/Platform.h"
 #include "wtf/MainThread.h"
 #include "wtf/text/TextEncodingRegistry.h"
@@ -225,20 +224,28 @@ void ScriptStreamer::startStreaming(PendingScript& script, Settings* settings, S
         blink::Platform::current()->histogramEnumeration(startedStreamingHistogramName(scriptType), 0, 2);
 }
 
-void ScriptStreamer::streamingCompleteOnBackgroundThread()
+void ScriptStreamer::streamingComplete()
 {
-    ASSERT(!isMainThread());
-    MutexLocker locker(m_mutex);
-    m_parsingFinished = true;
-    if (shouldBlockMainThread()) {
-        // 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));
+    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.
+    m_parsingFinished = true;
+
+    notifyFinishedToClient();
+
+    // The background thread no longer holds an implicit reference.
+    deref();
 }
 
 void ScriptStreamer::cancel()
@@ -255,7 +262,6 @@ void ScriptStreamer::cancel()
 
 void ScriptStreamer::suppressStreaming()
 {
-    MutexLocker locker(m_mutex);
     ASSERT(!m_parsingFinished);
     ASSERT(!m_loadingFinished);
     m_streamingSuppressed = true;
@@ -288,8 +294,7 @@ 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 or in
-        // notifyFinished.
+        // the corresponding deref() is in streamingComplete.
         ref();
         ScriptStreamingTask* task = new ScriptStreamingTask(m_task.release(), this);
         ScriptStreamerThread::shared()->postTask(task);
@@ -313,33 +318,10 @@ void ScriptStreamer::notifyFinished(Resource* resource)
     }
     m_stream->didFinishLoading();
     m_loadingFinished = true;
-
-    if (shouldBlockMainThread()) {
-        // 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.
-        TRACE_EVENT0("v8", "v8.mainThreadWaitingForParserThread");
-        MutexLocker locker(m_mutex);
-        if (!isFinished()) {
-            m_parsingFinishedCondition.wait(m_mutex);
-        }
-        ASSERT(isFinished());
-    }
-
     notifyFinishedToClient();
-
-    if (shouldBlockMainThread() && 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, ScriptStreamingMode mode)
+ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::StreamedSource::Encoding encoding, PendingScript::Type scriptType)
     : m_resource(resource)
     , m_detached(false)
     , m_stream(new SourceStream(this))
@@ -350,37 +332,9 @@ ScriptStreamer::ScriptStreamer(ScriptResource* resource, v8::ScriptCompiler::Str
     , m_haveEnoughDataForStreaming(false)
     , m_streamingSuppressed(false)
     , m_scriptType(scriptType)
-    , m_scriptStreamingMode(mode)
 {
 }
 
-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(!shouldBlockMainThread());
-
-    // 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()
 {
     ASSERT(isMainThread());
@@ -392,7 +346,6 @@ 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);
 }
@@ -421,10 +374,6 @@ bool ScriptStreamer::startStreamingInternal(PendingScript& script, Settings* set
     ASSERT(isMainThread());
     if (!settings || !settings->v8ScriptStreamingEnabled())
         return false;
-    if (settings->v8ScriptStreamingMode() == ScriptStreamingModeOnlyAsyncAndDefer
-        && scriptType == PendingScript::ParsingBlocking)
-        return false;
-
     ScriptResource* resource = script.resource();
     ASSERT(!resource->isLoaded());
     if (!resource->url().protocolIsInHTTPFamily())
@@ -466,7 +415,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, settings->v8ScriptStreamingMode()));
+    RefPtr<ScriptStreamer> streamer = adoptRef(new ScriptStreamer(resource, encoding, scriptType));
 
     // Decide what kind of cached data we should produce while streaming. By
     // default, we generate the parser cache for streamed scripts, to emulate

third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.h

@@ -5,7 +5,6 @@
 #ifndef ScriptStreamer_h
 #define ScriptStreamer_h
 
-#include "bindings/core/v8/ScriptStreamingMode.h"
 #include "core/dom/PendingScript.h"
 #include "wtf/RefCounted.h"
 
@@ -83,7 +82,7 @@ public:
 
     // Called by ScriptStreamingTask when it has streamed all data to V8 and V8
     // has processed it.
-    void streamingCompleteOnBackgroundThread();
+    void streamingComplete();
 
     static void setSmallScriptThresholdForTesting(size_t threshold)
     {
@@ -97,16 +96,10 @@ private:
     // streamed. Non-const for testing.
     static size_t kSmallScriptThreshold;
 
-    ScriptStreamer(ScriptResource*, v8::ScriptCompiler::StreamedSource::Encoding, PendingScript::Type, ScriptStreamingMode);
+    ScriptStreamer(ScriptResource*, v8::ScriptCompiler::StreamedSource::Encoding, PendingScript::Type);
 
-    void streamingComplete();
     void notifyFinishedToClient();
 
-    bool shouldBlockMainThread() const
-    {
-        return m_scriptStreamingMode == ScriptStreamingModeAllPlusBlockParsingBlocking && m_scriptType == PendingScript::ParsingBlocking;
-    }
-
     static const char* startedStreamingHistogramName(PendingScript::Type);
 
     static bool startStreamingInternal(PendingScript&, Settings*, ScriptState*, PendingScript::Type);
@@ -125,9 +118,7 @@ private:
     ScriptResourceClient* m_client;
     WTF::OwnPtr<v8::ScriptCompiler::ScriptStreamingTask> m_task;
     bool m_loadingFinished; // Whether loading from the network is done.
-    // Whether the V8 side processing is done. Will be used by the main thread
-    // and the streamer thread; guarded by m_mutex.
-    bool m_parsingFinished;
+    bool m_parsingFinished; // Whether the V8 side processing is done.
     // Whether we have received enough data to start the streaming.
     bool m_haveEnoughDataForStreaming;
 
@@ -140,13 +131,6 @@ private:
 
     // For recording metrics for different types of scripts separately.
     PendingScript::Type m_scriptType;
-
-    // Streaming mode defines whether the main thread should block and wait for
-    // the parsing to complete after the load has finished. See
-    // ScriptStreamer::notifyFinished for more information.
-    ScriptStreamingMode m_scriptStreamingMode;
-    Mutex m_mutex;
-    ThreadCondition m_parsingFinishedCondition;
 };
 
 } // namespace blink

third_party/WebKit/Source/bindings/core/v8/ScriptStreamerTest.cpp

@@ -8,7 +8,6 @@
 
 #include "bindings/core/v8/ScriptSourceCode.h"
 #include "bindings/core/v8/ScriptStreamerThread.h"
-#include "bindings/core/v8/ScriptStreamingMode.h"
 #include "bindings/core/v8/V8Binding.h"
 #include "bindings/core/v8/V8ScriptRunner.h"
 #include "core/dom/PendingScript.h"
@@ -59,9 +58,7 @@ private:
     PendingScript m_pendingScript;
 };
 
-// The bool param for ScriptStreamingTest controls whether to make the main
-// thread block and wait for parsing.
-class ScriptStreamingTest : public testing::TestWithParam<bool> {
+class ScriptStreamingTest : public testing::Test {
 public:
     ScriptStreamingTest()
         : m_scope(v8::Isolate::GetCurrent())
@@ -71,8 +68,6 @@ public:
         , m_pendingScript(PendingScriptWrapper::create(0, m_resource)) // Takes ownership of m_resource.
     {
         m_settings->setV8ScriptStreamingEnabled(true);
-        if (GetParam())
-            m_settings->setV8ScriptStreamingMode(ScriptStreamingModeAllPlusBlockParsingBlocking);
         m_resource->setLoading(true);
         ScriptStreamer::setSmallScriptThresholdForTesting(0);
     }
@@ -145,7 +140,7 @@ private:
     bool m_finished;
 };
 
-TEST_P(ScriptStreamingTest, CompilingStreamedScript)
+TEST_F(ScriptStreamingTest, CompilingStreamedScript)
 {
     // Test that we can successfully compile a streamed script.
     ScriptStreamer::startStreaming(pendingScript(), m_settings.get(), m_scope.scriptState(), PendingScript::ParsingBlocking);
@@ -174,7 +169,7 @@ TEST_P(ScriptStreamingTest, CompilingStreamedScript)
     EXPECT_FALSE(tryCatch.HasCaught());
 }
 
-TEST_P(ScriptStreamingTest, CompilingStreamedScriptWithParseError)
+TEST_F(ScriptStreamingTest, CompilingStreamedScriptWithParseError)
 {
     // Test that scripts with parse errors are handled properly. In those cases,
     // the V8 side typically finished before loading finishes: make sure we
@@ -207,7 +202,7 @@ TEST_P(ScriptStreamingTest, CompilingStreamedScriptWithParseError)
     EXPECT_TRUE(tryCatch.HasCaught());
 }
 
-TEST_P(ScriptStreamingTest, CancellingStreaming)
+TEST_F(ScriptStreamingTest, CancellingStreaming)
 {
     // Test that the upper layers (PendingScript and up) can be ramped down
     // while streaming is ongoing, and ScriptStreamer handles it gracefully.
@@ -234,7 +229,7 @@ TEST_P(ScriptStreamingTest, CancellingStreaming)
     EXPECT_FALSE(client.finished());
 }
 
-TEST_P(ScriptStreamingTest, SuppressingStreaming)
+TEST_F(ScriptStreamingTest, SuppressingStreaming)
 {
     // If we notice during streaming that there is a code cache, streaming
     // is suppressed (V8 doesn't parse while the script is loading), and the
@@ -262,7 +257,7 @@ TEST_P(ScriptStreamingTest, SuppressingStreaming)
     EXPECT_FALSE(sourceCode.streamer());
 }
 
-TEST_P(ScriptStreamingTest, EmptyScripts)
+TEST_F(ScriptStreamingTest, EmptyScripts)
 {
     // Empty scripts should also be streamed properly, that is, the upper layer
     // (ScriptResourceClient) should be notified when an empty script has been
@@ -283,7 +278,7 @@ TEST_P(ScriptStreamingTest, EmptyScripts)
     EXPECT_FALSE(sourceCode.streamer());
 }
 
-TEST_P(ScriptStreamingTest, SmallScripts)
+TEST_F(ScriptStreamingTest, SmallScripts)
 {
     // Small scripts shouldn't be streamed.
     ScriptStreamer::setSmallScriptThresholdForTesting(100);
@@ -306,7 +301,7 @@ TEST_P(ScriptStreamingTest, SmallScripts)
     EXPECT_FALSE(sourceCode.streamer());
 }
 
-TEST_P(ScriptStreamingTest, ScriptsWithSmallFirstChunk)
+TEST_F(ScriptStreamingTest, ScriptsWithSmallFirstChunk)
 {
     // If a script is long enough, if should be streamed, even if the first data
     // chunk is small.
@@ -336,8 +331,6 @@ TEST_P(ScriptStreamingTest, ScriptsWithSmallFirstChunk)
     EXPECT_FALSE(tryCatch.HasCaught());
 }
 
-INSTANTIATE_TEST_CASE_P(ScriptStreamingInstantiation, ScriptStreamingTest, ::testing::Values(false, true));
-
 } // namespace
 
 } // namespace blink

third_party/WebKit/Source/bindings/core/v8/ScriptStreamerThread.cpp

@@ -67,7 +67,9 @@ void ScriptStreamingTask::run()
     // Running the task can and will block: SourceStream::GetSomeData will get
     // called and it will block and wait for data from the network.
     m_v8Task->Run();
-    m_streamer->streamingCompleteOnBackgroundThread();
+    // Post a task to the main thread to signal that V8 has completed the
+    // streaming.
+    callOnMainThread(WTF::bind(&ScriptStreamer::streamingComplete, m_streamer));
     ScriptStreamerThread::shared()->taskDone();
 }
 

third_party/WebKit/Source/bindings/core/v8/ScriptStreamingMode.h

-// Copyright 2014 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef ScriptStreamingMode_h
-#define ScriptStreamingMode_h
-
-// ScriptStreamingModes are used for evaluating different heuristics for when we
-// should start streaming.
-
-namespace blink {
-
-enum ScriptStreamingMode {
-    // Stream all scripts
-    ScriptStreamingModeAll,
-    // Stream only async and deferred scripts
-    ScriptStreamingModeOnlyAsyncAndDefer,
-    // Stream all scripts, block the main thread after loading when streaming
-    // parser blocking scripts.
-    ScriptStreamingModeAllPlusBlockParsingBlocking
-};
-
-} // namespace blink
-
-#endif // ScriptStreamingMode_h

third_party/WebKit/Source/bindings/core/v8/v8.gypi

@@ -90,7 +90,6 @@
             'ScriptStreamer.h',
             'ScriptStreamerThread.cpp',
             'ScriptStreamerThread.h',
-            'ScriptStreamingMode.h',
             'ScriptString.cpp',
             'ScriptString.h',
             'ScriptValue.cpp',

third_party/WebKit/Source/core/frame/Settings.h

@@ -27,7 +27,6 @@
 #ifndef Settings_h
 #define Settings_h
 
-#include "bindings/core/v8/ScriptStreamingMode.h"
 #include "bindings/core/v8/V8CacheOptions.h"
 #include "core/SettingsMacros.h"
 #include "core/css/PointerProperties.h"

third_party/WebKit/Source/core/frame/Settings.in

@@ -281,7 +281,6 @@ fullscreenSupported initial=true
 v8CacheOptions type=V8CacheOptions, initial=V8CacheOptionsOff
 
 v8ScriptStreamingEnabled initial=false
-v8ScriptStreamingMode type=ScriptStreamingMode, initial=ScriptStreamingModeAll
 
 # These values are bit fields for the properties of available pointing devices
 # and may take on multiple values (e.g. laptop with touchpad and touchscreen