Adapt ScriptStreamer to recent changes in v8 streaming.

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

Index: third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp b/third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp
index 2c5e18eb4b5b6060f1ca10e33ae50f05bd9914af..cc1bb81de423305bc94532a5b828c863c6fa8f26 100644
--- a/third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/ScriptStreamer.cpp
@@ -46,7 +46,7 @@ void recordStartedStreamingHistogram(ScriptStreamer::Type scriptType, int reason
         break;
     }
     default:
-        ASSERT_NOT_REACHED();
+        NOTREACHED();
         break;
     }
 }
@@ -85,7 +85,7 @@ void recordNotStreamingReasonHistogram(ScriptStreamer::Type scriptType, NotStrea
         break;
     }
     default:
-        ASSERT_NOT_REACHED();
+        NOTREACHED();
         break;
     }
 }
@@ -116,7 +116,7 @@ public:
     void produce(const uint8_t* data, size_t length)
     {
         MutexLocker locker(m_mutex);
-        ASSERT(!m_finished);
+        DCHECK(!m_finished);
         m_data.append(std::make_pair(data, length));
         m_haveData.signal();
     }
@@ -180,8 +180,6 @@ public:
         , m_finished(false)
         , m_queueLeadPosition(0)
         , m_queueTailPosition(0)
-        , m_bookmarkPosition(0)
-        , m_lengthOfBOM(0)
         , m_loadingTaskRunner(loadingTaskRunner->clone())
     {
     }
@@ -192,7 +190,7 @@ public:
     // some data.
     size_t GetMoreData(const uint8_t** src) override
     {
-        ASSERT(!isMainThread());
+        DCHECK(!isMainThread());
         {
             MutexLocker locker(m_mutex);
             if (m_cancelled)
@@ -210,53 +208,22 @@ public:
         return length;
     }
 
-    // Called by V8 on background thread.
-    bool SetBookmark() override
-    {
-        ASSERT(!isMainThread());
-        m_bookmarkPosition = m_queueLeadPosition;
-        return true;
-    }
-
-    // Called by V8 on background thread.
-    void ResetToBookmark() override
-    {
-        ASSERT(!isMainThread());
-        {
-            MutexLocker locker(m_mutex);
-            m_queueLeadPosition = m_bookmarkPosition;
-            // See comments at m_lengthOfBOM declaration below for why
-            // we need this here.
-            m_queueTailPosition = m_bookmarkPosition + m_lengthOfBOM;
-            m_dataQueue.clear();
-        }
-
-        // Inform main thread to re-queue the data.
-        m_loadingTaskRunner->postTask(
-            BLINK_FROM_HERE, crossThreadBind(&SourceStream::fetchDataFromResourceBuffer, crossThreadUnretained(this), 0));
-    }
-
     void didFinishLoading()
     {
-        ASSERT(isMainThread());
-
-        // ResetToBookmark may reset the data queue's 'finished' status,
-        // so we may need to re-finish after a ResetToBookmark happened.
-        // We do this by remembering m_finished, and always checking for it
-        // at the end ot fetchDataFromResourceBuffer.
+        DCHECK(isMainThread());
         m_finished = true;
-        fetchDataFromResourceBuffer(0);
+        m_dataQueue.finish();
     }
 
-    void didReceiveData(ScriptStreamer* streamer, size_t lengthOfBOM)
+    void didReceiveData(ScriptStreamer* streamer)
     {
-        ASSERT(isMainThread());
-        prepareDataOnMainThread(streamer, lengthOfBOM);
+        DCHECK(isMainThread());
+        prepareDataOnMainThread(streamer);
     }
 
     void cancel()
     {
-        ASSERT(isMainThread());
+        DCHECK(isMainThread());
         // The script is no longer needed by the upper layers. Stop streaming
         // it. The next time GetMoreData is called (or woken up), it will return
         // 0, which will be interpreted as EOS by V8 and the parsing will
@@ -270,17 +237,14 @@ public:
     }
 
 private:
-    void prepareDataOnMainThread(ScriptStreamer* streamer, size_t lengthOfBOM)
+    void prepareDataOnMainThread(ScriptStreamer* streamer)
     {
-        ASSERT(isMainThread());
+        DCHECK(isMainThread());
 
         // The Resource must still be alive; otherwise we should've cancelled
         // the streaming (if we have cancelled, the background thread is not
         // waiting).
-        ASSERT(streamer->resource());
-
-        // BOM can only occur at the beginning of the data.
-        ASSERT(lengthOfBOM == 0 || m_queueTailPosition == 0);
+        DCHECK(streamer->resource());
 
         if (!streamer->resource()->response().cacheStorageCacheName().isNull()) {
             streamer->suppressStreaming();
@@ -304,19 +268,15 @@ private:
             m_resourceBuffer = streamer->resource()->resourceBuffer();
         }
 
-        fetchDataFromResourceBuffer(lengthOfBOM);
+        fetchDataFromResourceBuffer();
     }
 
-    void fetchDataFromResourceBuffer(size_t lengthOfBOM)
+    void fetchDataFromResourceBuffer()
     {
-        ASSERT(isMainThread());
-        MutexLocker locker(m_mutex); // For m_cancelled + m_queueTailPosition.
-
-        if (lengthOfBOM > 0) {
-            ASSERT(!m_lengthOfBOM); // There should be only one BOM.
-            m_lengthOfBOM = lengthOfBOM;
-        }
+        DCHECK(isMainThread());
+        MutexLocker locker(m_mutex);
 
+        DCHECK(!m_finished);
         if (m_cancelled) {
             m_dataQueue.finish();
             return;
@@ -324,43 +284,15 @@ private:
 
         // Get as much data from the ResourceBuffer as we can.
         const char* data = nullptr;
-        Vector<const char*> chunks;
-        Vector<size_t> chunkLengths;
-        size_t bufferLength = 0;
         while (size_t length = m_resourceBuffer->getSomeData(data, m_queueTailPosition)) {
-            // FIXME: Here we can limit based on the total length, if it turns
-            // out that we don't want to give all the data we have (memory
-            // vs. speed).
-            chunks.append(data);
-            chunkLengths.append(length);
-            bufferLength += length;
-            m_queueTailPosition += length;
-        }
+            // Copy the data chunks into a new buffer, since we're going to
+            // give the data to a background thread.
+            uint8_t* copiedData = new uint8_t[length];
+            memcpy(copiedData, data, length);
+            m_dataQueue.produce(copiedData, length);
 
-        // Copy the data chunks into a new buffer, since we're going to give the
-        // data to a background thread.
-        if (bufferLength > lengthOfBOM) {
-            size_t totalLength = bufferLength - lengthOfBOM;
-            uint8_t* copiedData = new uint8_t[totalLength];
-            size_t offset = 0;
-            size_t offsetInChunk = lengthOfBOM;
-            for (size_t i = 0; i < chunks.size(); ++i) {
-                if (offsetInChunk >= chunkLengths[i]) {
-                    offsetInChunk -= chunkLengths[i];
-                    continue;
-                }
-
-                size_t dataLength = chunkLengths[i] - offsetInChunk;
-                memcpy(copiedData + offset, chunks[i] + offsetInChunk, dataLength);
-                offset += dataLength;
-                // BOM is in the beginning of the buffer.
-                offsetInChunk = 0;
-            }
-            m_dataQueue.produce(copiedData, totalLength);
+            m_queueTailPosition += length;
         }
-
-        if (m_finished)
-            m_dataQueue.finish();
     }
 
     // For coordinating between the main thread and background thread tasks.
@@ -381,23 +313,6 @@ private:
     SourceStreamDataQueue m_dataQueue; // Thread safe.
     size_t m_queueLeadPosition; // Only used by v8 thread.
     size_t m_queueTailPosition; // Used by both threads; guarded by m_mutex.
-    size_t m_bookmarkPosition; // Only used by v8 thread.
-
-    // BOM (Unicode Byte Order Mark) handling:
-    // This class is responsible for stripping out the BOM, since Chrome
-    // delivers the input stream potentially with BOM, but V8 doesn't want
-    // to see the BOM. This is mostly easy to do, except for a funky edge
-    // condition with bookmarking:
-    // - m_queueLeadPosition counts the bytes that V8 has received
-    //   (i.e., without BOM)
-    // - m_queueTailPosition counts the bytes that Chrome has sent
-    //   (i.e., with BOM)
-    // So when resetting the bookmark, we have to adjust the lead position
-    // to account for the BOM (which happens implicitly in the regular
-    // streaming case).
-    // We store this separately, to avoid having to guard all
-    // m_queueLeadPosition references with a mutex.
-    size_t m_lengthOfBOM; // Used by both threads; guarded by m_mutex.
 
     std::unique_ptr<WebTaskRunner> m_loadingTaskRunner;
 };
@@ -443,7 +358,7 @@ bool ScriptStreamer::isFinished() const
 
 void ScriptStreamer::streamingCompleteOnBackgroundThread()
 {
-    ASSERT(!isMainThread());
+    DCHECK(!isMainThread());
     {
         MutexLocker locker(m_mutex);
         m_parsingFinished = true;
@@ -462,7 +377,7 @@ void ScriptStreamer::streamingCompleteOnBackgroundThread()
 
 void ScriptStreamer::cancel()
 {
-    ASSERT(isMainThread());
+    DCHECK(isMainThread());
     // The upper layer doesn't need the script any more, but streaming might
     // still be ongoing. Tell SourceStream to try to cancel it whenever it gets
     // the control the next time. It can also be that V8 has already completed
@@ -476,7 +391,7 @@ void ScriptStreamer::cancel()
 void ScriptStreamer::suppressStreaming()
 {
     MutexLocker locker(m_mutex);
-    ASSERT(!m_loadingFinished);
+    DCHECK(!m_loadingFinished);
     // It can be that the parsing task has already finished (e.g., if there was
     // a parse error).
     m_streamingSuppressed = true;
@@ -484,49 +399,50 @@ void ScriptStreamer::suppressStreaming()
 
 void ScriptStreamer::notifyAppendData(ScriptResource* resource)
 {
-    ASSERT(isMainThread());
-    ASSERT(m_resource == resource);
+    DCHECK(isMainThread());
+    DCHECK_EQ(m_resource, resource);
     {
         MutexLocker locker(m_mutex);
         if (m_streamingSuppressed)
             return;
     }
-    size_t lengthOfBOM = 0;
     if (!m_haveEnoughDataForStreaming) {
         // Even if the first data chunk is small, the script can still be big
         // enough - wait until the next data chunk comes before deciding whether
         // to start the streaming.
-        ASSERT(resource->resourceBuffer());
+        DCHECK(resource->resourceBuffer());
         if (resource->resourceBuffer()->size() < s_smallScriptThreshold)
             return;
         m_haveEnoughDataForStreaming = true;
 
-        // Encoding should be detected only when we have some data. It's
-        // possible that resource->encoding() returns a different encoding
-        // before the loading has started and after we got some data.  In
-        // addition, check for byte order marks. Note that checking the byte
-        // order mark might change the encoding. We cannot decode the full text
-        // here, because it might contain incomplete UTF-8 characters. Also note
-        // that have at least s_smallScriptThreshold worth of data, which is more
-        // than enough for detecting a BOM.
-        constexpr size_t maximumLengthOfBOM = 4;
-        char maybeBOM[maximumLengthOfBOM] = {};
-        if (!resource->resourceBuffer()->getPartAsBytes(maybeBOM, static_cast<size_t>(0), maximumLengthOfBOM)) {
-            NOTREACHED();
-            return;
-        }
-
-        std::unique_ptr<TextResourceDecoder> decoder(TextResourceDecoder::create("application/javascript", resource->encoding()));
-        lengthOfBOM = decoder->checkForBOM(maybeBOM, maximumLengthOfBOM);
+        {
+            // Check for BOM (byte order marks), because that might change our
+            // understanding of the data encoding.
+            constexpr size_t maximumLengthOfBOM = 4;
+            char maybeBOM[maximumLengthOfBOM] = {};
+            if (!resource->resourceBuffer()->getPartAsBytes(maybeBOM, static_cast<size_t>(0), maximumLengthOfBOM)) {
+                NOTREACHED();
+                return;
+            }
 
-        // Maybe the encoding changed because we saw the BOM; get the encoding
-        // from the decoder.
-        if (!convertEncoding(decoder->encoding().name(), &m_encoding)) {
-            suppressStreaming();
-            recordNotStreamingReasonHistogram(m_scriptType, EncodingNotSupported);
-            recordStartedStreamingHistogram(m_scriptType, 0);
-            return;
+            std::unique_ptr<TextResourceDecoder> decoder(TextResourceDecoder::create("application/javascript", resource->encoding()));
+            decoder->checkForBOM(maybeBOM, maximumLengthOfBOM);
+
+            // The encoding may change when we see the BOM. Check for BOM now
+            // and update the encoding from the decoder when necessary. Abort
+            // streaming if the encoding is different from the one we started

[marja, 2016/09/20 19:03:55]

This comment is not correct: we don't abort streaming if the encoding is
different. convertEncoding returns false when the encoding not supported, and in
that case we suppress the streaming. In other cases we just update m_encoding
and carry on.

A side though from checking convertEncoding: as we don't stream two byte
scripts, is the V8 side of TwoByteExternalStreamingStream dead code?

[vogelheim, 2016/09/22 08:15:14]

Within Chromium: Yes, I think so.

This is a bit suprising... The V8 API supports it. Options are:

1, We keep TwoByteExternalStreamingStream in (despite it not being used by
Chromium, I believe not by node.js or other clients either).
2, We UNREACHABLE() it in the API.
3, Or we change the API to remove it.

Not sure I think about this, yet.

At least it has unit tests, now. :-]

[vogelheim, 2016/09/22 08:15:14]

Done.

+            // with.
+            //
+            // Also note that have at least s_smallScriptThreshold worth of
+            // data, which is more than enough for detecting a BOM.
+            if (!convertEncoding(decoder->encoding().name(), &m_encoding)) {
+                suppressStreaming();
+                recordNotStreamingReasonHistogram(m_scriptType, EncodingNotSupported);
+                recordStartedStreamingHistogram(m_scriptType, 0);
+                return;
+            }
         }
+
         if (ScriptStreamerThread::shared()->isRunningTask()) {
             // At the moment we only have one thread for running the tasks. A
             // new task shouldn't be queued before the running task completes,
@@ -545,8 +461,8 @@ void ScriptStreamer::notifyAppendData(ScriptResource* resource)
             return;
         }
 
-        ASSERT(!m_stream);
-        ASSERT(!m_source);
+        DCHECK(!m_stream);
+        DCHECK(!m_source);
         m_stream = new SourceStream(m_loadingTaskRunner.get());
         // m_source takes ownership of m_stream.
         m_source = wrapUnique(new v8::ScriptCompiler::StreamedSource(m_stream, m_encoding));
@@ -567,13 +483,13 @@ void ScriptStreamer::notifyAppendData(ScriptResource* resource)
         recordStartedStreamingHistogram(m_scriptType, 1);
     }
     if (m_stream)
-        m_stream->didReceiveData(this, lengthOfBOM);
+        m_stream->didReceiveData(this);
 }
 
 void ScriptStreamer::notifyFinished(Resource* resource)
 {
-    ASSERT(isMainThread());
-    ASSERT(m_resource == resource);
+    DCHECK(isMainThread());
+    DCHECK_EQ(m_resource, resource);
     // A special case: empty and small scripts. We didn't receive enough data to
     // start the streaming before this notification. In that case, there won't
     // be a "parsing complete" notification either, and we should not wait for
@@ -623,7 +539,7 @@ void ScriptStreamer::streamingComplete()
 {
     // The background task is completed; do the necessary ramp-down in the main
     // thread.
-    ASSERT(isMainThread());
+    DCHECK(isMainThread());
 
     // It's possible that the corresponding Resource was deleted before V8
     // finished streaming. In that case, the data or the notification is not
@@ -641,7 +557,7 @@ void ScriptStreamer::streamingComplete()
 
 void ScriptStreamer::notifyFinishedToClient()
 {
-    ASSERT(isMainThread());
+    DCHECK(isMainThread());
     // Usually, the loading will be finished first, and V8 will still need some
     // time to catch up. But the other way is possible too: if V8 detects a
     // parse error, the V8 side can complete before loading has finished. Send
@@ -658,8 +574,8 @@ void ScriptStreamer::notifyFinishedToClient()
 
 bool ScriptStreamer::startStreamingInternal(PendingScript* script, Type scriptType, Settings* settings, ScriptState* scriptState, WebTaskRunner* loadingTaskRunner)
 {
-    ASSERT(isMainThread());
-    ASSERT(scriptState->contextIsValid());
+    DCHECK(isMainThread());
+    DCHECK(scriptState->contextIsValid());
     ScriptResource* resource = script->resource();
     if (resource->isLoaded()) {
         recordNotStreamingReasonHistogram(scriptType, AlreadyLoaded);