Redirect HTML resource bytes directly to parser thread (Blink side CL)

Source/core/html/parser/BackgroundHTMLParser.cpp

Index: Source/core/html/parser/BackgroundHTMLParser.cpp
diff --git a/Source/core/html/parser/BackgroundHTMLParser.cpp b/Source/core/html/parser/BackgroundHTMLParser.cpp
index 77d11543259836602909d8da77ebeed03c3d073e..5c1ddfc0dbb4a9b7d01c1a437502fc38fcf8d711 100644
--- a/Source/core/html/parser/BackgroundHTMLParser.cpp
+++ b/Source/core/html/parser/BackgroundHTMLParser.cpp
@@ -27,7 +27,6 @@
 #include "core/html/parser/BackgroundHTMLParser.h"
 
 #include "core/html/parser/HTMLDocumentParser.h"
-#include "core/html/parser/HTMLParserThread.h"
 #include "core/html/parser/TextResourceDecoder.h"
 #include "core/html/parser/XSSAuditor.h"
 #include "wtf/MainThread.h"
@@ -94,11 +93,47 @@ BackgroundHTMLParser::BackgroundHTMLParser(PassRefPtr<WeakReference<BackgroundHT
     , m_xssAuditor(config->xssAuditor.release())
     , m_preloadScanner(config->preloadScanner.release())
     , m_decoder(config->decoder.release())
+    , m_resourceBridge(config->resourceBridge.release())
+    , m_parserThreadIsStandalone(false)
 {
+    // Tell the resource bridge that we're ready to start receiving
+    // chunks of data.
+    if (m_resourceBridge)
+        m_resourceBridge->setPeer(this);
 }
 
 BackgroundHTMLParser::~BackgroundHTMLParser()
 {
+    if (m_resourceBridge) {
+        // Tell the resource bridge that calling into this instance
+        // is no longer safe and any further data received should be
+        // ignored.
+        m_resourceBridge->setPeer(0);
+        // The resource bridge needs to be destructed on the main thread,
+        // as the filter it installs on the I/O thread can only be removed
+        // from the main thread...
+        // Note we can only call static functions on the HTMLDocumentParser at this point
+        // as the weakptr we have for it is likely invalidated now.
+        callOnMainThread(bind(&HTMLDocumentParser::destructResourceBridge, m_resourceBridge.release()));
+    }
+}
+
+void BackgroundHTMLParser::OnReceivedData(const char* data, size_t length)
+{
+    if (m_parserThreadIsStandalone) {
+        updateDocument(m_decoder->decode(data, length));
+        return;
+    }
+
+    // If the parser thread is not standalone yet, it means we may get
+    // further data packets from the main thread and will need to queue
+    // up any data coming directly from the parser thread until we're
+    // sure we're complete.
+
+    // Storing the raw pointer here is safe as we keep the shared
+    // memory alive for as long as this object is alive (via its
+    // attached WebParserResourceBridge)

[abarth-chromium, 2013/12/18 18:28:49]

That sounds very fragile.  How about we just make a copy of the data that this
object can own instead of having this complex lifetime coupling.

[oystein (OOO til 10th of July), 2014/01/13 23:19:50]

The lifetime coupling is still needed to be able to safely make the calls
between the Chrome and Blink parts of this, so making a copy here wouldn't
simplify any code elsewhere as far as I can tell.

+    m_queuedData.push_back(std::make_pair(data, length));
 }
 
 void BackgroundHTMLParser::append(const String& input)
@@ -110,6 +145,7 @@ void BackgroundHTMLParser::append(const String& input)
 
 void BackgroundHTMLParser::appendBytes(PassOwnPtr<Vector<char> > buffer)
 {
+    ASSERT(!m_parserThreadIsStandalone);
     updateDocument(m_decoder->decode(buffer->data(), buffer->size()));
 }
 
@@ -129,7 +165,6 @@ void BackgroundHTMLParser::updateDocument(const String& decodedData)
 
     if (encodingData != m_lastSeenEncodingData) {
         m_lastSeenEncodingData = encodingData;
-
         m_xssAuditor->setEncoding(encodingData.encoding());
         callOnMainThread(bind(&HTMLDocumentParser::didReceiveEncodingDataFromBackgroundParser, m_parser, encodingData));
     }
@@ -170,6 +205,22 @@ void BackgroundHTMLParser::stop()
     delete this;
 }
 
+void BackgroundHTMLParser::resourceFilterAdded()
+{
+    m_parserThreadIsStandalone = true;

[abarth-chromium, 2013/12/18 18:28:49]

I'm not sure what this state variable means.  It looks like a switch between
receiving data from the main thread and from receiving data directly from the
network stack.

[oystein (OOO til 10th of July), 2014/01/13 23:19:50]

Renamed to m_receivingDataOnlyFromResourceProvider. A bit verbose though, but
best I could come up with.

+
+    // At this point we know for sure that no further data will
+    // be coming from the main thread, so we can process anything
+    // we've received directly on the parser thread in the meantime.
+    for (DataQueue::iterator entry = m_queuedData.begin(); entry != m_queuedData.end(); ++entry)
+        updateDocument(m_decoder->decode(entry->first, entry->second));

[abarth-chromium, 2013/12/18 18:28:49]

Do we need to worry about one of these updateDocument calls stopping parsing?

[oystein (OOO til 10th of July), 2014/01/13 23:19:50]

Shouldn't matter. As soon as a parsed chunk is processed on the main thread and
signals for the parsing to stop, it'll invalidate the weakptrs that the
BackgroundHTMLParser holds, so any further updates will be ignored.

+
+    // After this point we'll never need m_queuedData again, so let's release its
+    // memory.
+    DataQueue emptyQueue;
+    std::swap(emptyQueue, m_queuedData);

[abarth-chromium, 2013/12/18 18:28:49]

These idioms will be different once you switch to using WTF collections.

+}
+
 void BackgroundHTMLParser::forcePlaintextForTextDocument()
 {
     // This is only used by the TextDocumentParser (a subclass of HTMLDocumentParser)