Browser process changes for Resource Timing sizes.

content/browser/loader/async_resource_handler_unittest.cc

+// 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 "content/browser/loader/async_resource_handler.h"
+
+#include <stddef.h>
+#include <stdint.h>
+#include <memory>
+#include <string>
+#include <tuple>
+#include <utility>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/feature_list.h"
+#include "base/logging.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/shared_memory_handle.h"
+#include "base/memory/weak_ptr.h"
+#include "base/process/process.h"
+#include "base/process/process_handle.h"
+#include "base/run_loop.h"
+#include "base/strings/string_number_conversions.h"
+#include "content/browser/loader/resource_dispatcher_host_impl.h"
+#include "content/browser/loader/resource_loader.h"
+#include "content/browser/loader/resource_loader_delegate.h"
+#include "content/browser/loader/resource_message_filter.h"
+#include "content/browser/loader/resource_request_info_impl.h"
+#include "content/common/resource_messages.h"
+#include "content/common/resource_request.h"
+#include "content/public/browser/resource_context.h"
+#include "content/public/browser/resource_request_info.h"
+#include "content/public/common/content_features.h"
+#include "content/public/common/process_type.h"
+#include "content/public/common/resource_type.h"
+#include "content/public/test/mock_resource_context.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "ipc/ipc_message.h"
+#include "ipc/ipc_message_macros.h"
+#include "net/http/http_response_headers.h"
+#include "net/http/http_util.h"
+#include "net/url_request/url_request.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_job_factory_impl.h"
+#include "net/url_request/url_request_test_job.h"
+#include "net/url_request/url_request_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/base/page_transition_types.h"
+#include "url/gurl.h"
+
+namespace content {
+
+namespace {
+
+// This string is repeated as necessary to create a response body of the
+// required size. It is 8 bytes so that repeated doubling can achieve any power
+// of 2 size exactly.
+const char kBaseData[] = "Hello.  ";
+
+// Response short enough to be inlined into a single IPC.
+const GURL kTestURLShort("test:short");
+
+// Response can be sent in a single chunk (not inlined).
+const GURL kTestURLMedium("test:medium");
+
+// Size of response to kTestURLMedium. It should be a power of 2 and larger than
+// kInlinedLeadingChunkSize.
+const size_t kMediumLength = 4096;
+
+// Response requires two chunks.
+const GURL kTestURLLarge("test:large");
+
+// Size of response to kTestURLLarge. It should be a power of 2 and larger than
+// kMaxAllocationSize.
+const size_t kLargeLength = 64 * 1024;
+
+std::string RepeatData(size_t target_size) {
+  std::string data = kBaseData;
+  data.reserve(target_size);
+  while (data.size() < target_size)
+    data += data;
+  return data;
+}
+
+// A Content-Length response header is needed for a response to be eligible for
+// inlining in an IPC. GetTotalReceivedBytes() must be implemented to test that
+// it is sent correctly by IPC. net::URLRequestTestJob doesn't supply a
+// Content-Length header or implement GetTotalReceivedBytes(). TestJob is a
+// subclass that adds these features, and also serves sufficiently large
+// responses for our tests.

[kinuko, 2016/07/08 09:39:28]

While this description is very useful for reviewing code it's probably too
lengthy / verbose to have in the code (and it adds some maintenance cost too
when others want to add more code to this class).

Maybe just: "This test job is to add Content-Length header and implement
GetTotalReceivedBytes()" is enough.

btw as for GetTotalReceivedBytes() we can just implement that in
URLRequestTestJob, right?

[Adam Rice, 2016/07/08 11:21:10]

Done.
I think this is worthwhile but I'd rather do it in a separate CL.

+class TestJob : public net::URLRequestTestJob {
+ public:
+  TestJob(net::URLRequest* request, net::NetworkDelegate* network_delegate)
+      : net::URLRequestTestJob(request, network_delegate, true) {}
+
+  // URLRequestJob implementation:
+  int64_t GetTotalReceivedBytes() const override {
+    std::string http_headers = net::HttpUtil::ConvertHeadersBackToHTTPResponse(
+        response_headers_->raw_headers());
+    return http_headers.size() + offset_;
+  }
+
+ protected:
+  // Override URLRequestTestJob behaviour.
+  void StartAsync() override {
+    std::string data;
+    if (request_->url() == kTestURLShort) {
+      data = kBaseData;
+    } else if (request_->url() == kTestURLMedium) {
+      data = RepeatData(kMediumLength);
+    } else if (request_->url() == kTestURLLarge) {
+      data = RepeatData(kLargeLength);
+    }
+    if (data.empty()) {
+      URLRequestTestJob::StartAsync();
+      return;
+    }
+
+    std::string headers = test_headers();
+    response_headers_ = new net::HttpResponseHeaders(
+        net::HttpUtil::AssembleRawHeaders(headers.data(), headers.size()));
+
+    response_data_ = data;
+    response_headers_->AddHeader("Content-Length: " +
+                                 base::SizeTToString(data.size()));
+    URLRequestTestJob::StartAsync();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TestJob);
+};
+
+// A ProtocolHandler is necessary to set TestJob as the URLRequestJob.

[kinuko, 2016/07/08 09:39:28]

I understand you added this as a follow-up for my review, but I don't think this
comment's necessary, please remove (it's obvious we need this as far as we need
custom job)

[Adam Rice, 2016/07/08 11:21:10]

Done.

+class TestJobProtocolHandler
+    : public net::URLRequestJobFactory::ProtocolHandler {
+ public:
+  // URLRequestJobFactory::ProtocolHandler implementation:

[kinuko, 2016/07/08 09:39:28]

nit: in this case it's obvious too, I don't think this comment's necessary

[Adam Rice, 2016/07/08 11:21:10]

Done.

+  net::URLRequestJob* MaybeCreateJob(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new TestJob(request, network_delegate);
+  }
+};
+
+// A subclass of ResourceMessageFilter that records IPC messages that are sent.
+class RecordingResourceMessageFilter : public ResourceMessageFilter {
+ public:
+  RecordingResourceMessageFilter(ResourceContext* resource_context,
+                                 net::URLRequestContext* request_context)
+      : ResourceMessageFilter(
+            0,
+            PROCESS_TYPE_RENDERER,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            nullptr,
+            base::Bind(&RecordingResourceMessageFilter::GetContexts,
+                       base::Unretained(this))),
+        resource_context_(resource_context),
+        request_context_(request_context) {
+    set_peer_process_for_testing(base::Process::Current());
+  }
+
+  const std::vector<std::unique_ptr<IPC::Message>>& messages() const {
+    return messages_;
+  }
+
+  // IPC::Sender implementation
+  bool Send(IPC::Message* message) override {
+    if (message->type() == ResourceMsg_SetDataBuffer::ID)
+      ConsumeHandle(*message);
+    messages_.push_back(base::WrapUnique(message));
+    return true;
+  }
+
+ private:
+  ~RecordingResourceMessageFilter() override {}
+
+  void GetContexts(ResourceType resource_type,
+                   int origin_pid,
+                   ResourceContext** resource_context,
+                   net::URLRequestContext** request_context) {
+    *resource_context = resource_context_;
+    *request_context = request_context_;
+  }
+
+  // Unpickle the base::SharedMemoryHandle to avoid warnings about
+  // "MessageAttachmentSet destroyed with unconsumed descriptors".
+  void ConsumeHandle(const IPC::Message& message) {
+    IPC_BEGIN_MESSAGE_MAP(RecordingResourceMessageFilter, message)
+      IPC_MESSAGE_HANDLER(ResourceMsg_SetDataBuffer, OnSetDataBuffer)
+      IPC_MESSAGE_UNHANDLED(NOTREACHED())
+    IPC_END_MESSAGE_MAP()
+  }
+
+  void OnSetDataBuffer(int request_id,
+                       base::SharedMemoryHandle shm_handle,
+                       int shm_size,
+                       base::ProcessId renderer_pid) {}
+
+  ResourceContext* const resource_context_;
+  net::URLRequestContext* const request_context_;
+  std::vector<std::unique_ptr<IPC::Message>> messages_;
+};
+
+class AsyncResourceHandlerTest : public ::testing::Test {
+ protected:
+  AsyncResourceHandlerTest()
+      : thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP), context_(true) {}
+
+  void CreateRequest(const GURL& url) {
+    test_job_factory_.SetProtocolHandler(
+        "test", base::MakeUnique<TestJobProtocolHandler>());
+    context_.set_job_factory(&test_job_factory_);
+    context_.Init();
+    std::unique_ptr<net::URLRequest> request =
+        context_.CreateRequest(GURL(url), net::DEFAULT_PRIORITY, nullptr);
+    resource_context_ = base::MakeUnique<MockResourceContext>(&context_);
+    filter_ = make_scoped_refptr(
+        new RecordingResourceMessageFilter(resource_context_.get(), &context_));
+    ResourceRequestInfo::AllocateForTesting(request.get(), RESOURCE_TYPE_IMAGE,
+                                            resource_context_.get(),
+                                            0,       // render_process_id
+                                            0,       // render_view_id
+                                            0,       // render_frame_id
+                                            false,   // is_main_frame
+                                            false,   // parent_is_main_frame
+                                            false,   // allow_download
+                                            true,    // is_async
+                                            false);  // is_using_lofi
+    std::unique_ptr<AsyncResourceHandler> handler =
+        base::MakeUnique<AsyncResourceHandler>(request.get(), &rdh_);
+    loader_delegate_.reset(new TrivialResourceLoaderDelegate(this));
+    loader_ =
+        base::MakeUnique<ResourceLoader>(std::move(request), std::move(handler),
+                                         nullptr, loader_delegate_.get());
+  }
+
+  void StartRequest() { loader_->StartRequest(); }
+
+  void WaitForFinish() { finish_waiter_.Run(); }
+
+  void CreateStartAndWait(const GURL& url) {
+    CreateRequest(url);
+    StartRequest();
+    WaitForFinish();
+  }
+
+  TestBrowserThreadBundle thread_bundle_;
+  ResourceDispatcherHostImpl rdh_;
+  net::TestURLRequestContext context_;
+  net::URLRequestJobFactoryImpl test_job_factory_;
+  std::unique_ptr<MockResourceContext> resource_context_;
+  scoped_refptr<RecordingResourceMessageFilter> filter_;
+  std::unique_ptr<ResourceLoader> loader_;
+  base::RunLoop finish_waiter_;
+
+ private:
+  class TrivialResourceLoaderDelegate : public ResourceLoaderDelegate {
+   public:
+    explicit TrivialResourceLoaderDelegate(AsyncResourceHandlerTest* fixture)
+        : fixture_(fixture) {}
+    ResourceDispatcherHostLoginDelegate* CreateLoginDelegate(
+        ResourceLoader* loader,
+        net::AuthChallengeInfo* auth_info) override {
+      return nullptr;
+    }
+
+    bool HandleExternalProtocol(ResourceLoader* loader,
+                                const GURL& url) override {
+      return false;
+    }
+    void DidStartRequest(ResourceLoader* loader) override {}
+    void DidReceiveRedirect(ResourceLoader* loader,
+                            const GURL& new_url) override {}
+    void DidReceiveResponse(ResourceLoader* loader) override {}
+    void DidFinishLoading(ResourceLoader* loader) override {
+      fixture_->DidFinishLoading();
+    }
+
+   private:
+    AsyncResourceHandlerTest* fixture_;
+  };
+
+  void DidFinishLoading() {
+    loader_.reset();
+    finish_waiter_.Quit();
+  }
+
+  std::unique_ptr<TrivialResourceLoaderDelegate> loader_delegate_;
+};
+
+std::unique_ptr<ResourceMsg_DataReceived::Param> UnpackDataReceivedIPC(
+    const IPC::Message* msg) {
+  if (ResourceMsg_DataReceived::ID != msg->type())
+    return nullptr;
+  static_assert(std::tuple_size<ResourceMsg_DataReceived::Param>::value == 5u,
+                "ResourceMsg_DataReceived argument count has changed. Tests "
+                "must be updated.");

[kinuko, 2016/07/08 09:39:28]

I feel these static_asserts are a bit too much. When test breaks we can look
into... that's how most tests are written, aren't they?

[Adam Rice, 2016/07/08 11:21:10]

I wanted to save debugging time. But since new arguments will usually be added
to the end, it may waste more time than it saves. Removed.

+  std::unique_ptr<ResourceMsg_DataReceived::Param> params =
+      base::MakeUnique<ResourceMsg_DataReceived::Param>();
+  if (!ResourceMsg_DataReceived::Read(msg, params.get()))

[kinuko, 2016/07/08 09:39:28]

Is checking the return value that useful here?

[Adam Rice, 2016/07/08 11:21:10]

As far I can tell it will only catch bugs in the IPC implementation, which is
not the purpose of this test. Removed.

+    return nullptr;
+  return params;
+}
+
+std::unique_ptr<ResourceMsg_InlinedDataChunkReceived::Param>
+UnpackInlinedDataChunkReceivedIPC(const IPC::Message* msg) {
+  if (ResourceMsg_InlinedDataChunkReceived::ID != msg->type())
+    return nullptr;
+  static_assert(
+      std::tuple_size<ResourceMsg_InlinedDataChunkReceived::Param>::value == 4u,
+      "ResourceMsg_InlinedDataChunkReceived argument count has changed. Tests "
+      "must be updated.");
+  std::unique_ptr<ResourceMsg_InlinedDataChunkReceived::Param> params =
+      base::MakeUnique<ResourceMsg_InlinedDataChunkReceived::Param>();
+  if (!ResourceMsg_InlinedDataChunkReceived::Read(msg, params.get()))
+    return nullptr;
+  return params;
+}
+
+TEST_F(AsyncResourceHandlerTest, Construct) {
+  CreateRequest(net::URLRequestTestJob::test_url_1());
+}
+
+TEST_F(AsyncResourceHandlerTest, OneChunkLengths) {
+  CreateStartAndWait(kTestURLMedium);
+  const auto& messages = filter_->messages();
+  ASSERT_EQ(4u, messages.size());
+  auto params = UnpackDataReceivedIPC(messages[2].get());
+  ASSERT_TRUE(params);
+
+  int encoded_data_length = std::get<3>(*params);
+  EXPECT_EQ(4163, encoded_data_length);
+  int encoded_body_length = std::get<4>(*params);
+  EXPECT_EQ(4096, encoded_body_length);
+}
+
+TEST_F(AsyncResourceHandlerTest, InlinedChunkLengths) {
+  // TODO(ricea): Remove this Feature-enabling code once the feature is on by
+  // default.
+  auto feature_list = base::MakeUnique<base::FeatureList>();
+  feature_list->InitializeFromCommandLine(
+      features::kOptimizeLoadingIPCForSmallResources.name, "");
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(std::move(feature_list));
+
+  CreateStartAndWait(kTestURLShort);
+  const auto& messages = filter_->messages();
+  ASSERT_EQ(3u, messages.size());
+  auto params = UnpackInlinedDataChunkReceivedIPC(messages[1].get());
+  ASSERT_TRUE(params);
+
+  int encoded_data_length = std::get<2>(*params);
+  EXPECT_EQ(72, encoded_data_length);
+  int encoded_body_length = std::get<3>(*params);
+  EXPECT_EQ(8, encoded_body_length);
+
+  base::FeatureList::ClearInstanceForTesting();
+  base::FeatureList::SetInstance(base::MakeUnique<base::FeatureList>());
+}
+
+TEST_F(AsyncResourceHandlerTest, TwoChunksLengths) {
+  CreateStartAndWait(kTestURLLarge);
+  const auto& messages = filter_->messages();
+  ASSERT_EQ(5u, messages.size());
+  auto params = UnpackDataReceivedIPC(messages[2].get());
+  ASSERT_TRUE(params);
+
+  int encoded_data_length = std::get<3>(*params);
+  EXPECT_EQ(32836, encoded_data_length);
+  int encoded_body_length = std::get<4>(*params);
+  EXPECT_EQ(32768, encoded_body_length);
+
+  params = UnpackDataReceivedIPC(messages[3].get());
+  ASSERT_TRUE(params);
+  encoded_data_length = std::get<3>(*params);
+  EXPECT_EQ(32768, encoded_data_length);
+  encoded_body_length = std::get<4>(*params);
+  EXPECT_EQ(32768, encoded_body_length);
+}
+
+}  // namespace
+
+}  // namespace content