Fix parsing of feed network responses

* Encodes request payload as base64
* Skips the leading varint message size before trying to parse the response

Bug: 1044139
Change-Id: I4736625ffdec1ba6e9b6ad0d4b64265183bdedc9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2142957Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Dan H <harringtond@chromium.org>
Commit-Queue: Ian Wells <iwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#758280}

components/feed/DEPS

@@ -17,6 +17,7 @@ include_rules = [
   "+services/network/public/mojom",
   "+services/network/test",
   "+third_party/zlib/google",
+  "+third_party/protobuf/src/google/protobuf/io",
   "+ui/base/mojom",
   "+ui/gfx/geometry",
   "+ui/gfx/image",

components/feed/core/v2/feed_network_impl.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "components/feed/core/v2/feed_network_impl.h"
+#include "base/base64url.h"
 #include "base/bind.h"
 #include "base/containers/flat_set.h"
 #include "base/metrics/histogram_functions.h"
@@ -31,6 +32,7 @@
 #include "services/network/public/cpp/resource_request_body.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
+#include "third_party/protobuf/src/google/protobuf/io/coded_stream.h"
 #include "third_party/zlib/google/compression_utils.h"
 
 namespace feed {
@@ -40,14 +42,14 @@ constexpr char kAuthenticationScope[] =
 constexpr char kApplicationOctetStream[] = "application/octet-stream";
 constexpr base::TimeDelta kNetworkTimeout = base::TimeDelta::FromSeconds(30);
 
+// Add URLs for Bling when it is supported.
 constexpr char kFeedQueryUrl[] =
-    "https://www.google.com/httpservice/retry/InteractiveDiscoverAgaService/"
-    "FeedQuery";
+    "https://www.google.com/httpservice/retry/TrellisClankService/FeedQuery";
 constexpr char kNextPageQueryUrl[] =
-    "https://www.google.com/httpservice/retry/InteractiveDiscoverAgaService/"
+    "https://www.google.com/httpservice/retry/TrellisClankService/"
     "NextPageQuery";
 constexpr char kBackgroundQueryUrl[] =
-    "https://www.google.com/httpservice/noretry/BackgroundDiscoverAgaService/"
+    "https://www.google.com/httpservice/noretry/TrellisClankService/"
     "FeedQuery";
 
 using RawResponse = FeedNetworkImpl::RawResponse;
@@ -72,7 +74,17 @@ void ParseAndForwardResponse(base::OnceCallback<void(RESULT)> result_callback,
   if (result.status_code == 200) {
     auto response_message = std::make_unique<typename decltype(
         result.response_body)::element_type>();
-    if (response_message->ParseFromString(raw_response.response_bytes)) {
+
+    ::google::protobuf::io::CodedInputStream input_stream(
+        reinterpret_cast<const uint8_t*>(raw_response.response_bytes.data()),
+        raw_response.response_bytes.size());
+
+    // The first few bytes of the body are a varint containing the size of the
+    // message. We need to skip over them.
+    int message_size;
+    input_stream.ReadVarintSizeAsInt(&message_size);
+
+    if (response_message->ParseFromCodedStream(&input_stream)) {
       result.response_body = std::move(response_message);
     }
   }
@@ -380,6 +392,9 @@ void FeedNetworkImpl::SendQueryRequest(
     base::OnceCallback<void(QueryRequestResult)> callback) {
   std::string binary_proto;
   request.SerializeToString(&binary_proto);
+  std::string base64proto;
+  base::Base64UrlEncode(
+      binary_proto, base::Base64UrlEncodePolicy::INCLUDE_PADDING, &base64proto);
 
   // TODO(harringtond): Decide how we want to override these URLs for testing.
   // Should probably add a command-line flag.
@@ -400,7 +415,7 @@ void FeedNetworkImpl::SendQueryRequest(
       return;
   }
 
-  AddMothershipPayloadQueryParams(/*is_post=*/false, binary_proto,
+  AddMothershipPayloadQueryParams(/*is_post=*/false, base64proto,
                                   delegate_->GetLanguageTag(), &url);
   Send(url, "GET", /*request_body=*/std::string(),
        base::BindOnce(&ParseAndForwardResponse<QueryRequestResult,
@@ -419,7 +434,6 @@ void FeedNetworkImpl::SendActionRequest(
       "ClankActionUpload");
   AddMothershipPayloadQueryParams(/*is_post=*/true, /*payload=*/std::string(),
                                   delegate_->GetLanguageTag(), &url);
-
   Send(url, "POST", std::move(binary_proto),
        base::BindOnce(
            &ParseAndForwardResponse<ActionRequestResult,

components/feed/core/v2/feed_network_impl_unittest.cc

@@ -31,6 +31,8 @@
 #include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/protobuf/src/google/protobuf/io/coded_stream.h"
+#include "third_party/protobuf/src/google/protobuf/io/zero_copy_stream_impl_lite.h"
 #include "third_party/zlib/google/compression_utils.h"
 #include "url/gurl.h"
 
@@ -129,6 +131,16 @@ class FeedNetworkTest : public testing::Test {
     test_factory_.AddResponse(url, std::move(head), response_string, status);
   }
 
+  std::string PrependResponseLength(const std::string& response) {
+    std::string result;
+    ::google::protobuf::io::StringOutputStream string_output_stream(&result);
+    ::google::protobuf::io::CodedOutputStream stream(&string_output_stream);
+
+    stream.WriteVarint32(static_cast<uint32_t>(response.size()));
+    stream.WriteString(response);
+    return result;
+  }
+
   network::ResourceRequest RespondToQueryRequest(
       const std::string& response_string,
       net::HttpStatusCode code) {
@@ -137,7 +149,8 @@ class FeedNetworkTest : public testing::Test {
         test_factory()->GetPendingRequest(0);
     CHECK(pending_request);
     network::ResourceRequest resource_request = pending_request->request;
-    Respond(pending_request->request.url, response_string, code);
+    Respond(pending_request->request.url,
+            PrependResponseLength(response_string), code);
     task_environment_.FastForwardUntilNoTasksRemain();
     return resource_request;
   }
@@ -189,8 +202,8 @@ TEST_F(FeedNetworkTest, SendQueryRequestSendsValidRequest) {
       RespondToQueryRequest("", net::HTTP_OK);
 
   EXPECT_EQ(
-      "https://www.google.com/httpservice/retry/InteractiveDiscoverAgaService/"
-      "FeedQuery?reqpld=%08%01%C2%3E%04%12%02%08%01&fmt=bin&hl=en",
+      "https://www.google.com/httpservice/retry/TrellisClankService/"
+      "FeedQuery?reqpld=CAHCPgQSAggB&fmt=bin&hl=en",
       resource_request.url);
   EXPECT_EQ("GET", resource_request.method);
   EXPECT_FALSE(resource_request.headers.HasHeader("content-encoding"));

components/feed/core/v2/tasks/load_stream_task.cc

@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/time/clock.h"
 #include "base/time/time.h"
+#include "components/feed/core/proto/v2/wire/capability.pb.h"
 #include "components/feed/core/proto/v2/wire/client_info.pb.h"
 #include "components/feed/core/proto/v2/wire/feed_request.pb.h"
 #include "components/feed/core/proto/v2/wire/request.pb.h"
@@ -77,6 +78,8 @@ void LoadStreamTask::LoadFromStoreComplete(
 
   feed_request.mutable_feed_query()->set_reason(
       feedwire::FeedQuery::MANUAL_REFRESH);
+  request.mutable_feed_request()->add_client_capability(
+      feedwire::Capability::BASE_UI);
   if (!result.consistency_token.empty()) {
     feed_request.mutable_consistency_token()->set_token(
         result.consistency_token);

components/feed/core/v2/testdata/request.textproto

@@ -8,7 +8,8 @@ feed_request {
       build_type: DEV
       api_version: 29
     }
-    app_type: CHROME
+    # TODO(iwells): Change this to CLANK when possible.
+    app_type: TEST_APP
     app_version {
       major: 79
       minor: 0

components/feed/core/v2/tools/feed_response_to_textproto.sh

@@ -13,10 +13,12 @@ OUT_FILE=$2
 TMP_FILE=/tmp/trimmedfeedresponse.binarypb
 
 CHROMIUM_SRC=$(realpath $(dirname $(readlink -f $0))/../../../../..)
-FEEDPROTO="$CHROMIUM_SRC/components/feed/core/proto"
 
-# Responses start with a 4-byte length value that must be removed.
-tail -c +4 $IN_FILE > $TMP_FILE
+# Responses start with a varint length value that must be removed.
+cat $IN_FILE | python3 -c "import sys
+while sys.stdin.buffer.read(1)[0]>127:
+  pass
+sys.stdout.buffer.write(sys.stdin.buffer.read())" > $TMP_FILE
 
 python3 $CHROMIUM_SRC/components/feed/core/v2/tools/textpb_to_binarypb.py \
   --chromium_path=$CHROMIUM_SRC \

components/feed/core/v2/tools/textpb_to_binarypb.py

@@ -46,7 +46,7 @@ flags.DEFINE_string('message',
 flags.DEFINE_string('direction', 'forward',
                     'Set --direction=reverse to convert binary to text.')
 
-COMPONENT_FEED_PROTO_PATH = 'components/feed/core/proto'
+COMPONENT_FEED_PROTO_PATH = 'components/feed/core/proto/v2'
 
 def text_to_binary():
   with open(FLAGS.source_file, mode='r') as file: