Fix parsing of segmented WebRequest formData

After https://chromium-review.googlesource.com/c/chromium/src/+/581748
we no longer flatten SharedBuffers in GetRequestBodyForWebHTTPBody()
but instead pass segmented data downstream (multiple DataElements).

This minimizes unnecessary reallocations/copies in general, but relies
on downstream clients being able to handle segmented data (or flatten it
on the fly if really needed).

Turns out WebRequest's FormDataParsers assume the input buffers are
flattened (or segmented along expected chunk boundaries for multipart).

This CL adds a buffering mechanism to ParsedDataPresenter which flattens
consecutive byte segments before passing down to the parser.

BUG=831169

Change-Id: Ibea9193ff783cad0c80bb9722f719ff063b65720
Reviewed-on: https://chromium-review.googlesource.com/1011322
Commit-Queue: Florin Malita <fmalita@chromium.org>
Reviewed-by: Ken Rockot <rockot@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551344}

extensions/browser/api/web_request/upload_data_presenter.cc

@@ -8,6 +8,7 @@
 
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
+#include "base/numerics/checked_math.h"
 #include "base/strings/string_util.h"
 #include "base/values.h"
 #include "extensions/browser/api/web_request/form_data_parser.h"
@@ -106,7 +107,39 @@ void ParsedDataPresenter::FeedBytes(base::StringPiece bytes) {
   if (!success_)
     return;
 
-  if (!parser_->SetSource(bytes)) {
+  if (pending_bytes_.empty()) {
+    // First data chunk: track externally.
+    DCHECK(buffer_.empty());
+
+    pending_bytes_ = bytes;
+    return;
+  }
+
+  // Pending data is either tracked externally or is located in our buffer.
+  DCHECK(buffer_.empty() || pending_bytes_.data() == buffer_.data());
+  DCHECK(buffer_.empty() || pending_bytes_.size() == buffer_.size());
+
+  const auto safe_size = base::CheckAdd(pending_bytes_.size(), bytes.size());
+  if (!safe_size.IsValid()) {
+    Abort();
+    return;
+  }
+
+  buffer_.reserve(safe_size.ValueOrDie());
+  if (buffer_.empty()) {
+    // Second data chunk: copy pending external data to our internal buffer.
+    buffer_.append(pending_bytes_.data(), pending_bytes_.size());
+  }
+
+  buffer_.append(bytes.data(), bytes.size());
+  pending_bytes_.set(buffer_.data(), buffer_.size());
+}
+
+void ParsedDataPresenter::CommitPendingBytes() {
+  if (!success_ || pending_bytes_.empty())
+    return;
+
+  if (!parser_->SetSource(pending_bytes_)) {
     Abort();
     return;
   }
@@ -116,11 +149,17 @@ void ParsedDataPresenter::FeedBytes(base::StringPiece bytes) {
     base::Value* list = GetOrCreateList(dictionary_.get(), result.name());
     list->GetList().emplace_back(result.take_value());
   }
+
+  buffer_.clear();
+  pending_bytes_.clear();
 }
 
-void ParsedDataPresenter::FeedFile(const base::FilePath& path) {}
+void ParsedDataPresenter::FeedFile(const base::FilePath&) {
+  CommitPendingBytes();
+}
 
 bool ParsedDataPresenter::Succeeded() {
+  CommitPendingBytes();
   if (success_ && !parser_->AllDataReadOK())
     Abort();
   return success_;

extensions/browser/api/web_request/upload_data_presenter.h

@@ -124,10 +124,17 @@ class ParsedDataPresenter : public UploadDataPresenter {
   // Clears resources and the success flag.
   void Abort();
 
+  // Flushes any pending data to the parser.
+  void CommitPendingBytes();
+
   std::unique_ptr<FormDataParser> parser_;
   bool success_;
   std::unique_ptr<base::DictionaryValue> dictionary_;
 
+  // Buffered data (not yet commited to the parser).
+  base::StringPiece pending_bytes_;
+  std::string buffer_;
+
   DISALLOW_COPY_AND_ASSIGN(ParsedDataPresenter);
 };
 

extensions/browser/api/web_request/upload_data_presenter_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/files/file_path.h"
 #include "base/values.h"
 #include "extensions/browser/api/web_request/upload_data_presenter.h"
 #include "extensions/browser/api/web_request/web_request_api_constants.h"
@@ -81,4 +82,46 @@ TEST(WebRequestUploadDataPresenterTest, RawData) {
   EXPECT_TRUE(result->Equals(&expected_list));
 }
 
+TEST(WebRequestUploadDataPresenterTest, ParsedDataSegmented) {
+  // Input.
+  static constexpr char block1[] = "v1=FOO";
+  static constexpr char block2[] = "BAR&v2=BAZ";
+  static constexpr size_t block1_size = sizeof(block1) - 1;
+  static constexpr size_t block2_size = sizeof(block2) - 1;
+
+  // Expected output.
+  auto v1 = std::make_unique<base::ListValue>();
+  v1->AppendString("FOOBAR");
+  auto v2 = std::make_unique<base::ListValue>();
+  v2->AppendString("BAZ");
+
+  base::DictionaryValue expected_form;
+  expected_form.SetWithoutPathExpansion("v1", std::move(v1));
+  expected_form.SetWithoutPathExpansion("v2", std::move(v2));
+
+  {
+    // Consecutive data segments should be consolidated and parsed successfuly.
+    auto parsed_data_presenter = ParsedDataPresenter::CreateForTests();
+    ASSERT_TRUE(parsed_data_presenter.get());
+    parsed_data_presenter->FeedBytes(base::StringPiece(block1, block1_size));
+    parsed_data_presenter->FeedBytes(base::StringPiece(block2, block2_size));
+
+    EXPECT_TRUE(parsed_data_presenter->Succeeded());
+    auto result = parsed_data_presenter->Result();
+    ASSERT_TRUE(result);
+    EXPECT_EQ(*result, expected_form);
+  }
+
+  {
+    // Data segments separate by file inputs should not be consolidated.
+    auto parsed_data_presenter = ParsedDataPresenter::CreateForTests();
+    ASSERT_TRUE(parsed_data_presenter.get());
+    parsed_data_presenter->FeedBytes(base::StringPiece(block1, block1_size));
+    parsed_data_presenter->FeedFile(base::FilePath());
+    parsed_data_presenter->FeedBytes(base::StringPiece(block2, block2_size));
+
+    EXPECT_FALSE(parsed_data_presenter->Succeeded());
+  }
+}
+
 }  // namespace extensions