Move multipart resource handling to core/fetch (1/2)

third_party/WebKit/Source/core/fetch/MultipartImageResourceParser.cpp

+// Copyright 2016 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.
+
+#include "core/fetch/MultipartImageResourceParser.h"
+
+#include "public/platform/Platform.h"
+#include "public/platform/WebURLResponse.h"
+#include "wtf/NotFound.h"
+#include "wtf/text/WTFString.h"
+
+#include <algorithm>
+
+namespace blink {
+
+MultipartImageResourceParser::MultipartImageResourceParser(const ResourceResponse& response, const Vector<char>& boundary, Client* client)
+    : m_originalResponse(response)
+    , m_boundary(boundary)
+    , m_client(client)
+{
+    // Some servers report a boundary prefixed with "--".  See https://crbug.com/5786.
+    if (m_boundary.size() < 2 || m_boundary[0] != '-' || m_boundary[1] != '-')
+        m_boundary.prepend("--", 2);
+}
+
+void MultipartImageResourceParser::appendData(const char* bytes, size_t size)
+{
+    // m_sawLastBoundary means that we've already received the final boundary
+    // token. The server should stop sending us data at this point, but if it
+    // does, we just throw it away.
+    if (m_sawLastBoundary)
+        return;
+    m_data.append(bytes, size);
+
+    if (m_isParsingTop) {
+        // Eat leading \r\n
+        int pos = pushOverLine(m_data, 0);

[Tom Sepez, 2016/03/04 19:07:31]

pos must be a size_t, too.

[yhirano, 2016/03/04 20:07:51]

Done.

+        if (pos)
+            m_data.remove(0, pos);
+
+        if (m_data.size() < m_boundary.size() + 2) {
+            // We don't have enough data yet to make a boundary token.  Just
+            // wait until the next chunk of data arrives.
+            return;
+        }
+
+        // Some servers don't send a boundary token before the first chunk of
+        // data.  We handle this case anyway (Gecko does too).
+        if (0 != memcmp(m_data.data(), m_boundary.data(), m_boundary.size())) {
+            m_data.prepend("\n", 1);
+            const auto& boundary = m_boundary;

[Tom Sepez, 2016/03/04 19:07:32]

nit: why this local?

[hiroshige, 2016/03/04 19:57:11]

Probably due to template instantiation problem in WTF::Vector. See comments in
Line 78 in
https://codereview.chromium.org/1693183002/diff/180001/third_party/WebKit/Sou...

yhirano@, can we create a crbug entry and add a comment here with link to crbug?

[yhirano, 2016/03/04 20:07:51]

Done.

+            m_data.prepend(boundary);
+        }
+        m_isParsingTop = false;
+    }
+    ASSERT(!m_isParsingTop);

[Tom Sepez, 2016/03/04 19:07:32]

Nit:  Seems pointless to assert given the assignment at 54.

[yhirano, 2016/03/04 20:07:51]

Done.

+
+    // Headers
+    if (m_isParsingHeaders) {
+        // Eat leading \r\n
+        int pos = pushOverLine(m_data, 0);
+        if (pos)
+            m_data.remove(0, pos);
+
+        if (parseHeaders()) {

[Tom Sepez, 2016/03/04 19:07:31]

nit: lets invert this so you write 
  if (!parseHeaders) 
    return;

Then you can avoid an else entirely.

[yhirano, 2016/03/04 20:07:51]

Done.

+            // Successfully parsed headers.
+            m_isParsingHeaders = false;
+        } else {
+            // Get more data before trying again.
+            return;
+        }
+        if (isCancelled())
+            return;
+    }
+    ASSERT(!m_isParsingHeaders);

[Tom Sepez, 2016/03/04 19:07:31]

ditto

[yhirano, 2016/03/04 20:07:51]

Done.

+
+    size_t boundaryPosition;
+    while ((boundaryPosition = findBoundary(m_data, &m_boundary)) != kNotFound) {
+        // Strip out trailing \r\n characters in the buffer preceding the
+        // boundary on the same lines as Firefox.

[Tom Sepez, 2016/03/04 19:07:31]

nit: as does Firefox. 
Otherwise it sounds like the string "Firefox" might be in the buffer.

[yhirano, 2016/03/04 20:07:51]

Done.

+        size_t dataSize = boundaryPosition;
+        if (boundaryPosition > 0 && m_data[boundaryPosition - 1] == '\n') {
+            dataSize--;
+            if (boundaryPosition > 1 && m_data[boundaryPosition - 2] == '\r') {
+                dataSize--;
+            }
+        }
+        if (dataSize > 0) {

[Tom Sepez, 2016/03/04 19:07:32]

nit: size_t's are unsigned, so maybe just 
  if (dataSize)

[yhirano, 2016/03/04 20:07:51]

Done.

+            m_client->multipartDataReceived(m_data.data(), dataSize);
+            if (isCancelled())
+                return;
+        }
+        size_t boundaryEndPosition = boundaryPosition + m_boundary.size();
+        if (boundaryEndPosition < m_data.size() && '-' == m_data[boundaryEndPosition]) {
+            // This was the last boundary so we can stop processing.
+            m_sawLastBoundary = true;
+            m_data.clear();
+            return;
+        }
+
+        // We can now throw out data up through the boundary
+        int offset = pushOverLine(m_data, boundaryEndPosition);

[Tom Sepez, 2016/03/04 19:07:32]

again, size_t.

[yhirano, 2016/03/04 20:07:51]

Done.

+        m_data.remove(0, boundaryEndPosition + offset);
+
+        // Ok, back to parsing headers
+        if (!parseHeaders()) {
+            m_isParsingHeaders = true;
+            break;
+        }
+    }
+
+    // At this point, we should send over any data we have, but keep enough data
+    // buffered to handle a boundary that may have been truncated.
+    if (!m_isParsingHeaders && m_data.size() > m_boundary.size()) {
+        // If the last character is a new line character, go ahead and just send
+        // everything we have buffered.  This matches an optimization in Gecko.
+        size_t sendLength = m_data.size() - m_boundary.size();
+        if (m_data.last() == '\n')
+            sendLength = m_data.size();
+        m_client->multipartDataReceived(m_data.data(), sendLength);
+        m_data.remove(0, sendLength);
+    }
+}
+
+void MultipartImageResourceParser::finish()
+{
+    ASSERT(!isCancelled());
+    // If we have any pending data and we're not in a header, go ahead and send
+    // it to the client.
+    if (!m_isParsingHeaders && !m_data.isEmpty() && !m_sawLastBoundary)
+        m_client->multipartDataReceived(m_data.data(), m_data.size());
+    m_data.clear();
+    m_sawLastBoundary = true;
+}
+
+int MultipartImageResourceParser::pushOverLine(const Vector<char>& data, size_t pos)
+{
+    int offset = 0;

[Tom Sepez, 2016/03/04 19:07:31]

make this a size_t, and return a size_t.

[yhirano, 2016/03/04 20:07:51]

Done.

+    if (pos < data.size() && (data[pos] == '\r' || data[pos] == '\n')) {
+        ++offset;

[Tom Sepez, 2016/03/04 19:07:31]

Wouldn't this strip \n\n in addition to \r\n and \n.  Is that what  you wanted?

[yhirano, 2016/03/04 20:07:51]

The logic has some problems but I would like to fix them later: the problems
exist in the current multipart_response_delegate.cc.

+        if (pos + 1 < data.size() && data[pos + 1] == '\n')
+            ++offset;
+    }
+    return offset;
+}
+
+bool MultipartImageResourceParser::parseHeaders()
+{
+    // Create a WebURLResponse based on the original set of headers + the
+    // replacement headers. We only replace the same few headers that gecko
+    // does. See netwerk/streamconv/converters/nsMultiMixedConv.cpp.
+    WebURLResponse response(m_originalResponse.url());
+    for (const auto& header : m_originalResponse.httpHeaderFields())
+        response.addHTTPHeaderField(header.key, header.value);
+
+    size_t end = 0;
+    if (!Platform::current()->parseMultipartHeadersFromBody(m_data.data(), m_data.size(), &response, &end))
+        return false;
+    m_data.remove(0, end);
+
+    // To avoid recording every multipart load as a separate visit in
+    // the history database, we want to keep track of whether the response
+    // is part of a multipart payload.  We do want to record the first visit,
+    // so we only set isMultipartPayload to true after the first visit.
+    response.setIsMultipartPayload(!m_isFirstPart);
+    m_isFirstPart = false;
+    // Send the response!
+    m_client->onePartInMultipartReceived(response.toResourceResponse());
+
+    return true;
+}
+
+// Boundaries are supposed to be preceeded with --, but it looks like gecko
+// doesn't require the dashes to exist.  See nsMultiMixedConv::FindToken.
+size_t MultipartImageResourceParser::findBoundary(const Vector<char>& data, Vector<char>* boundary)
+{
+    auto it = std::search(data.data(), data.data() + data.size(), boundary->data(), boundary->data() + boundary->size());
+    if (it == data.data() + data.size())
+        return kNotFound;
+
+    size_t boundaryPosition = it - data.data();
+    // Back up over -- for backwards compat
+    // TODO(tc): Don't we only want to do this once?  Gecko code doesn't
+    // seem to care.
+    if (boundaryPosition >= 2) {

[Tom Sepez, 2016/03/04 19:07:31]

nit: Our style is generally to turn these around, as in

data[bondaryPosition -1] == '-'

since gcc gives a warning if you mistakenly use assignment nowdays.

[yhirano, 2016/03/04 20:07:51]

Done.

+        if ('-' == data[boundaryPosition - 1] && '-' == data[boundaryPosition - 2]) {
+            boundaryPosition -= 2;
+            Vector<char> v(2, '-');
+            v.appendVector(*boundary);
+            *boundary = v;
+        }
+    }
+    return boundaryPosition;
+}
+
+DEFINE_TRACE(MultipartImageResourceParser)
+{
+    visitor->trace(m_client);
+}
+
+} // namespace blink