Support the remainder of chrome_pdf::UrlRequest

Adds support for the remaining properties of UrlRequest to
chrome_pdf::BlinkUrlLoader::Open(): the request body, the custom
referrer URL, and whether to ignore redirects.

BlinkUrlLoader::Client now uses a single IsValid() method to check for
missing resources, rather than returning errors from individual methods.
Returning errors from each method complicates the API for no benefit.

MockBlinkUrlLoaderClient has been replaced by FakeBlinkUrlLoaderClient.
The tests no longer assert interactions with BlinkUrlLoader::Client, but
the results of those interactions, so a mock is no longer appropriate.

Additional changes to Open() improve PepperURLLoaderHost compatibility:
The request URL is resolved against the document URL, site-for-cookies
are passed from the document, and service workers are skipped.

Bug: 1099022
Change-Id: If5cbb562ae74efcd3f3807a452e655a893a08eea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2424661Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809966}

pdf/pdf_view_web_plugin.cc

@@ -17,6 +17,7 @@
 #include "base/thread_annotations.h"
 #include "base/threading/thread_checker.h"
 #include "cc/paint/paint_canvas.h"
+#include "net/cookies/site_for_cookies.h"
 #include "pdf/pdf_engine.h"
 #include "pdf/pdf_init.h"
 #include "pdf/pdfium/pdfium_engine.h"
@@ -28,7 +29,9 @@
 #include "third_party/blink/public/platform/web_input_event_result.h"
 #include "third_party/blink/public/platform/web_rect.h"
 #include "third_party/blink/public/platform/web_string.h"
+#include "third_party/blink/public/platform/web_url.h"
 #include "third_party/blink/public/platform/web_url_error.h"
+#include "third_party/blink/public/platform/web_url_request.h"
 #include "third_party/blink/public/platform/web_url_response.h"
 #include "third_party/blink/public/web/web_associated_url_loader.h"
 #include "third_party/blink/public/web/web_associated_url_loader_options.h"
@@ -282,19 +285,37 @@ float PdfViewWebPlugin::GetToolbarHeightInScreenCoords() {
 
 void PdfViewWebPlugin::DocumentFocusChanged(bool document_has_focus) {}
 
+bool PdfViewWebPlugin::IsValid() const {
+  return container_ && container_->GetDocument().GetFrame();
+}
+
+blink::WebURL PdfViewWebPlugin::CompleteURL(
+    const blink::WebString& partial_url) const {
+  DCHECK(IsValid());
+  return container_->GetDocument().CompleteURL(partial_url);
+}
+
+net::SiteForCookies PdfViewWebPlugin::SiteForCookies() const {
+  DCHECK(IsValid());
+  return container_->GetDocument().SiteForCookies();
+}
+
+void PdfViewWebPlugin::SetReferrerForRequest(
+    blink::WebURLRequest& request,
+    const blink::WebURL& referrer_url) {
+  DCHECK(IsValid());
+  container_->GetDocument().GetFrame()->SetReferrerForRequest(request,
+                                                              referrer_url);
+}
+
 std::unique_ptr<blink::WebAssociatedURLLoader>
 PdfViewWebPlugin::CreateAssociatedURLLoader(
     const blink::WebAssociatedURLLoaderOptions& options) {
-  if (!container_)
-    return nullptr;
-
-  blink::WebLocalFrame* frame = container_->GetDocument().GetFrame();
-  if (!frame)
-    return nullptr;
-
   // TODO(crbug.com/1127146): blink::WebLocalFrame::CreateAssociatedURLLoader()
   // really should return a std::unique_ptr instead.
-  return base::WrapUnique(frame->CreateAssociatedURLLoader(options));
+  DCHECK(IsValid());
+  return base::WrapUnique(
+      container_->GetDocument().GetFrame()->CreateAssociatedURLLoader(options));
 }
 
 base::WeakPtr<PdfViewPluginBase> PdfViewWebPlugin::GetWeakPtr() {

pdf/pdf_view_web_plugin.h

@@ -102,6 +102,11 @@ class PdfViewWebPlugin final : public PdfViewPluginBase,
   void DocumentFocusChanged(bool document_has_focus) override;
 
   // BlinkUrlLoader::Client:
+  bool IsValid() const override;
+  blink::WebURL CompleteURL(const blink::WebString& partial_url) const override;
+  net::SiteForCookies SiteForCookies() const override;
+  void SetReferrerForRequest(blink::WebURLRequest& request,
+                             const blink::WebURL& referrer_url) override;
   std::unique_ptr<blink::WebAssociatedURLLoader> CreateAssociatedURLLoader(
       const blink::WebAssociatedURLLoaderOptions& options) override;
 

pdf/ppapi_migration/url_loader.cc

@@ -17,6 +17,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/notreached.h"
 #include "net/base/net_errors.h"
+#include "net/cookies/site_for_cookies.h"
 #include "net/http/http_util.h"
 #include "pdf/ppapi_migration/callback.h"
 #include "ppapi/c/pp_errors.h"
@@ -29,6 +30,8 @@
 #include "ppapi/cpp/url_response_info.h"
 #include "ppapi/cpp/var.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom-shared.h"
+#include "third_party/blink/public/platform/web_data.h"
+#include "third_party/blink/public/platform/web_http_body.h"
 #include "third_party/blink/public/platform/web_http_header_visitor.h"
 #include "third_party/blink/public/platform/web_string.h"
 #include "third_party/blink/public/platform/web_url.h"
@@ -99,16 +102,23 @@ void BlinkUrlLoader::Open(const UrlRequest& request, ResultCallback callback) {
   state_ = LoadingState::kOpening;
   open_callback_ = std::move(callback);
 
-  if (!client_) {
+  if (!client_ || !client_->IsValid()) {
     AbortLoad(PP_ERROR_FAILED);
     return;
   }
 
   // Modeled on `content::CreateWebURLRequest()`.
+  // TODO(crbug.com/1129291): The original code performs additional validations
+  // that we probably don't need in the new process model.
   blink::WebURLRequest blink_request;
-  blink_request.SetUrl(GURL(request.url));
+  blink_request.SetUrl(
+      client_->CompleteURL(blink::WebString::FromUTF8(request.url)));
   blink_request.SetHttpMethod(blink::WebString::FromASCII(request.method));
 
+  blink_request.SetSiteForCookies(client_->SiteForCookies());
+  blink_request.SetSkipServiceWorker(true);
+
+  // Note: The PDF plugin doesn't set the `X-Requested-With` header.
   if (!request.headers.empty()) {
     net::HttpUtil::HeadersIterator it(request.headers.begin(),
                                       request.headers.end(), "\n\r");
@@ -118,18 +128,27 @@ void BlinkUrlLoader::Open(const UrlRequest& request, ResultCallback callback) {
     }
   }
 
+  if (!request.body.empty()) {
+    blink::WebHTTPBody body;
+    body.Initialize();
+    body.AppendData(request.body);
+    blink_request.SetHttpBody(body);
+  }
+
+  if (!request.custom_referrer_url.empty()) {
+    client_->SetReferrerForRequest(blink_request,
+                                   GURL(request.custom_referrer_url));
+  }
+
   blink_request.SetRequestContext(blink::mojom::RequestContextType::PLUGIN);
   blink_request.SetRequestDestination(
       network::mojom::RequestDestination::kEmbed);
 
+  // TODO(crbug.com/822081): Revisit whether we need universal access.
   blink::WebAssociatedURLLoaderOptions options;
   options.grant_universal_access = grant_universal_access_;
+  ignore_redirects_ = request.ignore_redirects;
   blink_loader_ = client_->CreateAssociatedURLLoader(options);
-  if (!blink_loader_) {
-    AbortLoad(PP_ERROR_FAILED);
-    return;
-  }
-
   blink_loader_->LoadAsynchronously(blink_request, this);
 }
 
@@ -168,11 +187,18 @@ void BlinkUrlLoader::Close() {
     AbortLoad(PP_ERROR_ABORTED);
 }
 
+// Modeled on `content::PepperURLLoaderHost::WillFollowRedirect()`.
 bool BlinkUrlLoader::WillFollowRedirect(
     const blink::WebURL& new_url,
     const blink::WebURLResponse& redirect_response) {
-  NOTIMPLEMENTED();
-  return false;
+  DCHECK_EQ(state_, LoadingState::kOpening);
+
+  // TODO(crbug.com/1129291): The original code performs additional validations
+  // that we probably don't need in the new process model.
+
+  // Note that `pp::URLLoader::FollowRedirect()` is not supported, so the
+  // redirect can be canceled immediately by returning `false` here.
+  return !ignore_redirects_;
 }
 
 void BlinkUrlLoader::DidSendData(uint64_t bytes_sent,

pdf/ppapi_migration/url_loader.h

@@ -21,9 +21,16 @@
 
 namespace blink {
 class WebAssociatedURLLoader;
+class WebString;
+class WebURL;
+class WebURLRequest;
 struct WebAssociatedURLLoaderOptions;
 }  // namespace blink
 
+namespace net {
+class SiteForCookies;
+}  // namespace net
+
 namespace chrome_pdf {
 
 // Properties for making a URL request.
@@ -115,8 +122,24 @@ class BlinkUrlLoader final : public UrlLoader,
   // client.
   class Client {
    public:
-    // Returns a new `blink::WebAssociatedURLLoader` from the current local
-    // frame. May return `nullptr` if the local frame no longer exists.
+    // Returns `true` if the client is still usable. The client may require
+    // resources that can become unavailable, such as a local frame. Rather than
+    // handling missing resources separately for each method, callers can just
+    // verify validity once, before making any other calls.
+    virtual bool IsValid() const = 0;
+
+    // Completes `partial_url` using the current document.
+    virtual blink::WebURL CompleteURL(
+        const blink::WebString& partial_url) const = 0;
+
+    // Gets the site-for-cookies for the current document.
+    virtual net::SiteForCookies SiteForCookies() const = 0;
+
+    // Sets the referrer on `request` to `referrer_url` using the current frame.
+    virtual void SetReferrerForRequest(blink::WebURLRequest& request,
+                                       const blink::WebURL& referrer_url) = 0;
+
+    // Returns a new `blink::WebAssociatedURLLoader` from the current frame.
     virtual std::unique_ptr<blink::WebAssociatedURLLoader>
     CreateAssociatedURLLoader(
         const blink::WebAssociatedURLLoaderOptions& options) = 0;
@@ -185,6 +208,7 @@ class BlinkUrlLoader final : public UrlLoader,
 
   std::unique_ptr<blink::WebAssociatedURLLoader> blink_loader_;
 
+  bool ignore_redirects_ = false;
   ResultCallback open_callback_;
 
   base::circular_deque<char> buffer_;

pdf/ppapi_migration/url_loader_unittest.cc

@@ -20,11 +20,14 @@
 #include "base/strings/string_util.h"
 #include "base/test/mock_callback.h"
 #include "net/base/net_errors.h"
+#include "net/cookies/site_for_cookies.h"
 #include "pdf/ppapi_migration/callback.h"
 #include "ppapi/c/pp_errors.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom-shared.h"
+#include "third_party/blink/public/platform/web_data.h"
+#include "third_party/blink/public/platform/web_http_body.h"
 #include "third_party/blink/public/platform/web_http_header_visitor.h"
 #include "third_party/blink/public/platform/web_string.h"
 #include "third_party/blink/public/platform/web_url.h"
@@ -44,10 +47,13 @@ using ::testing::Each;
 using ::testing::ElementsAreArray;
 using ::testing::Invoke;
 using ::testing::NiceMock;
+using ::testing::Return;
 using ::testing::ReturnNull;
 using ::testing::SaveArg;
 using ::testing::UnorderedElementsAreArray;
 
+constexpr char kOriginUrl[] = "http://example.com/";
+constexpr char kDocumentUrl[] = "http://example.com/embedder/index.html";
 constexpr base::span<const char> kFakeData = "fake data";
 
 size_t GetRequestHeaderCount(const blink::WebURLRequest& request) {
@@ -84,41 +90,68 @@ class MockWebAssociatedURLLoader : public blink::WebAssociatedURLLoader {
               (override));
 };
 
-class MockBlinkUrlLoaderClient : public BlinkUrlLoader::Client {
+class FakeBlinkUrlLoaderClient : public BlinkUrlLoader::Client {
  public:
-  base::WeakPtr<MockBlinkUrlLoaderClient> GetWeakPtr() {
+  base::WeakPtr<FakeBlinkUrlLoaderClient> GetWeakPtr() {
     return weak_factory_.GetWeakPtr();
   }
 
   void InvalidateWeakPtrs() { weak_factory_.InvalidateWeakPtrs(); }
 
+  void Invalidate() { valid_ = false; }
+
+  MockWebAssociatedURLLoader& mock_url_loader() { return *mock_url_loader_; }
+
+  const blink::WebAssociatedURLLoaderOptions& saved_options() const {
+    return saved_options_;
+  }
+
   // BlinkUrlLoader::Client:
-  MOCK_METHOD(std::unique_ptr<blink::WebAssociatedURLLoader>,
-              CreateAssociatedURLLoader,
-              (const blink::WebAssociatedURLLoaderOptions&),
-              (override));
+  bool IsValid() const override { return valid_; }
+
+  blink::WebURL CompleteURL(
+      const blink::WebString& partial_url) const override {
+    EXPECT_TRUE(IsValid());
+    return GURL(kDocumentUrl).Resolve(partial_url.Utf8());
+  }
+
+  net::SiteForCookies SiteForCookies() const override {
+    EXPECT_TRUE(IsValid());
+    return net::SiteForCookies::FromUrl(GURL(kOriginUrl));
+  }
+
+  void SetReferrerForRequest(blink::WebURLRequest& request,
+                             const blink::WebURL& referrer_url) override {
+    EXPECT_FALSE(referrer_url.IsEmpty());
+    EXPECT_TRUE(IsValid());
+    request.SetReferrerString(referrer_url.GetString());
+  }
+
+  std::unique_ptr<blink::WebAssociatedURLLoader> CreateAssociatedURLLoader(
+      const blink::WebAssociatedURLLoaderOptions& options) override {
+    EXPECT_TRUE(IsValid());
+    EXPECT_TRUE(mock_url_loader_);
+    saved_options_ = options;
+    return std::move(mock_url_loader_);
+  }
 
  private:
-  base::WeakPtrFactory<MockBlinkUrlLoaderClient> weak_factory_{this};
+  bool valid_ = true;
+
+  std::unique_ptr<NiceMock<MockWebAssociatedURLLoader>> mock_url_loader_ =
+      std::make_unique<NiceMock<MockWebAssociatedURLLoader>>();
+  blink::WebAssociatedURLLoaderOptions saved_options_;
+
+  base::WeakPtrFactory<FakeBlinkUrlLoaderClient> weak_factory_{this};
 };
 
 class BlinkUrlLoaderTest : public testing::Test {
  protected:
   BlinkUrlLoaderTest() {
-    ON_CALL(mock_client_, CreateAssociatedURLLoader(_))
-        .WillByDefault(
-            Invoke(this, &BlinkUrlLoaderTest::FakeCreateAssociatedURLLoader));
-    ON_CALL(*mock_url_loader_, LoadAsynchronously(_, _))
+    ON_CALL(fake_client_.mock_url_loader(), LoadAsynchronously(_, _))
         .WillByDefault(
             Invoke(this, &BlinkUrlLoaderTest::FakeLoadAsynchronously));
-    loader_ = std::make_unique<BlinkUrlLoader>(mock_client_.GetWeakPtr());
-  }
-
-  std::unique_ptr<blink::WebAssociatedURLLoader> FakeCreateAssociatedURLLoader(
-      const blink::WebAssociatedURLLoaderOptions& options) {
-    EXPECT_TRUE(mock_url_loader_);
-    saved_options_ = options;
-    return std::move(mock_url_loader_);
+    loader_ = std::make_unique<BlinkUrlLoader>(fake_client_.GetWeakPtr());
   }
 
   void FakeLoadAsynchronously(const blink::WebURLRequest& request,
@@ -136,25 +169,21 @@ class BlinkUrlLoaderTest : public testing::Test {
     return result;
   }
 
-  NiceMock<MockBlinkUrlLoaderClient> mock_client_;
+  FakeBlinkUrlLoaderClient fake_client_;
   NiceMock<base::MockCallback<ResultCallback>> mock_callback_;
   std::unique_ptr<BlinkUrlLoader> loader_;
 
-  std::unique_ptr<NiceMock<MockWebAssociatedURLLoader>> mock_url_loader_ =
-      std::make_unique<NiceMock<MockWebAssociatedURLLoader>>();
-  blink::WebAssociatedURLLoaderOptions saved_options_;
   blink::WebURLRequest saved_request_;
 };
 
 TEST_F(BlinkUrlLoaderTest, GrantUniversalAccess) {
   loader_->GrantUniversalAccess();
   loader_->Open(UrlRequest(), mock_callback_.Get());
-  EXPECT_TRUE(saved_options_.grant_universal_access);
+  EXPECT_TRUE(fake_client_.saved_options().grant_universal_access);
 }
 
 TEST_F(BlinkUrlLoaderTest, Open) {
-  EXPECT_CALL(mock_client_, CreateAssociatedURLLoader(_));
-  EXPECT_CALL(*mock_url_loader_, LoadAsynchronously(_, _));
+  EXPECT_CALL(fake_client_.mock_url_loader(), LoadAsynchronously(_, _));
   EXPECT_CALL(mock_callback_, Run(_)).Times(0);
 
   UrlRequest request;
@@ -162,9 +191,12 @@ TEST_F(BlinkUrlLoaderTest, Open) {
   request.method = "FAKE";
   loader_->Open(request, mock_callback_.Get());
 
-  EXPECT_FALSE(saved_options_.grant_universal_access);
+  EXPECT_FALSE(fake_client_.saved_options().grant_universal_access);
   EXPECT_EQ(GURL("http://example.com/fake.pdf"), GURL(saved_request_.Url()));
   EXPECT_EQ("FAKE", saved_request_.HttpMethod().Ascii());
+  EXPECT_EQ(GURL(kOriginUrl),
+            saved_request_.SiteForCookies().RepresentativeUrl());
+  EXPECT_TRUE(saved_request_.GetSkipServiceWorker());
   EXPECT_EQ(0u, GetRequestHeaderCount(saved_request_));
   EXPECT_EQ(blink::mojom::RequestContextType::PLUGIN,
             saved_request_.GetRequestContext());
@@ -172,24 +204,33 @@ TEST_F(BlinkUrlLoaderTest, Open) {
             saved_request_.GetRequestDestination());
 }
 
-TEST_F(BlinkUrlLoaderTest, OpenWithInvalidatedClient) {
-  EXPECT_CALL(mock_client_, CreateAssociatedURLLoader(_)).Times(0);
-  EXPECT_CALL(*mock_url_loader_, LoadAsynchronously(_, _)).Times(0);
+TEST_F(BlinkUrlLoaderTest, OpenWithInvalidatedClientWeakPtr) {
+  EXPECT_CALL(fake_client_.mock_url_loader(), LoadAsynchronously(_, _))
+      .Times(0);
   EXPECT_CALL(mock_callback_, Run(PP_ERROR_FAILED));
 
-  mock_client_.InvalidateWeakPtrs();
+  fake_client_.InvalidateWeakPtrs();
   loader_->Open(UrlRequest(), mock_callback_.Get());
 }
 
-TEST_F(BlinkUrlLoaderTest, OpenWithFailingCreateAssociatedURLLoader) {
-  EXPECT_CALL(mock_client_, CreateAssociatedURLLoader(_))
-      .WillOnce(ReturnNull());
-  EXPECT_CALL(*mock_url_loader_, LoadAsynchronously(_, _)).Times(0);
+TEST_F(BlinkUrlLoaderTest, OpenWithInvalidatedClient) {
+  EXPECT_CALL(fake_client_.mock_url_loader(), LoadAsynchronously(_, _))
+      .Times(0);
   EXPECT_CALL(mock_callback_, Run(PP_ERROR_FAILED));
 
+  fake_client_.Invalidate();
   loader_->Open(UrlRequest(), mock_callback_.Get());
 }
 
+TEST_F(BlinkUrlLoaderTest, OpenWithRelativeUrl) {
+  UrlRequest request;
+  request.url = "relative.pdf";
+  loader_->Open(request, mock_callback_.Get());
+
+  EXPECT_EQ(GURL(kDocumentUrl).Resolve("relative.pdf"),
+            GURL(saved_request_.Url()));
+}
+
 TEST_F(BlinkUrlLoaderTest, OpenWithHeaders) {
   UrlRequest request;
   request.headers = base::JoinString(
@@ -208,6 +249,52 @@ TEST_F(BlinkUrlLoaderTest, OpenWithHeaders) {
   EXPECT_EQ("🙃", saved_request_.HttpHeaderField("Non-ASCII-Value").Utf8());
 }
 
+TEST_F(BlinkUrlLoaderTest, OpenWithBody) {
+  UrlRequest request;
+  request.body = "fake body";
+  loader_->Open(request, mock_callback_.Get());
+
+  blink::WebHTTPBody request_body = saved_request_.HttpBody();
+  EXPECT_EQ(1u, request_body.ElementCount());
+
+  blink::WebHTTPBody::Element element;
+  EXPECT_TRUE(request_body.ElementAt(0, element));
+  EXPECT_EQ(blink::WebHTTPBody::Element::kTypeData, element.type);
+
+  std::string data;
+  element.data.ForEachSegment(
+      [&](const char* segment, size_t length, size_t pos) {
+        data.append(segment, length);
+        return true;
+      });
+  EXPECT_EQ("fake body", data);
+}
+
+TEST_F(BlinkUrlLoaderTest, OpenWithCustomReferrerUrl) {
+  UrlRequest request;
+  request.custom_referrer_url = "http://example.com/referrer";
+  loader_->Open(request, mock_callback_.Get());
+
+  EXPECT_EQ("http://example.com/referrer",
+            saved_request_.ReferrerString().Utf8());
+}
+
+TEST_F(BlinkUrlLoaderTest, WillFollowRedirect) {
+  loader_->Open(UrlRequest(), mock_callback_.Get());
+
+  EXPECT_TRUE(loader_->WillFollowRedirect(GURL("http://example.com/login"),
+                                          blink::WebURLResponse()));
+}
+
+TEST_F(BlinkUrlLoaderTest, WillFollowRedirectWhileIgnoringRedirects) {
+  UrlRequest request;
+  request.ignore_redirects = true;
+  loader_->Open(request, mock_callback_.Get());
+
+  EXPECT_FALSE(loader_->WillFollowRedirect(GURL("http://example.com/login"),
+                                           blink::WebURLResponse()));
+}
+
 TEST_F(BlinkUrlLoaderTest, DidReceiveResponse) {
   loader_->Open(UrlRequest(), mock_callback_.Get());
   EXPECT_CALL(mock_callback_, Run(PP_OK));