Drop RefCounted from chrome_pdf::UrlLoader

Replaces all instances of scoped_refptr<UrlLoader> with
std::unique_ptr<UrlLoader>. It turns out there's no need for multiple
ownership of UrlLoader.

Drops OutOfProcessInstance's embed_loader_ and embed_preview_loader_
fields in favor of binding to the result callback. This may cause
behavioral changes in two cases:

1. The field is overwritten before the callback runs.
2. The field is not cleared after the callback runs.

(1) would cause other problems, as the current code assumes only one
load occurs at a time (as tracked by document_load_state_ and
preview_document_load_state_). The extra fields are needed only to
simulate bound parameters in the old pp::CompletionCallback
implementation.

(2) is the case currently, but this change simply ensures that the
loader is cleaned up as soon as it's no longer needed, rather than
"leaking" a lingering reference.

Retains the form_loader_ field, rather than also binding to the result
callback, as this loader is not handed off to PDFiumEngine. In general,
we don't handle form submission correctly, but fixing this is more a
matter for crbug.com/719344.

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

pdf/out_of_process_instance.cc

@@ -18,7 +18,6 @@
 #include "base/callback.h"
 #include "base/feature_list.h"
 #include "base/logging.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/notreached.h"
@@ -1078,9 +1077,10 @@ void OutOfProcessInstance::OnPaint(const std::vector<gfx::Rect>& paint_rects,
   }
 }
 
-void OutOfProcessInstance::DidOpen(int32_t result) {
+void OutOfProcessInstance::DidOpen(std::unique_ptr<UrlLoader> loader,
+                                   int32_t result) {
   if (result == PP_OK) {
-    if (!engine()->HandleDocumentLoad(embed_loader_)) {
+    if (!engine()->HandleDocumentLoad(std::move(loader))) {
       document_load_state_ = LOAD_STATE_LOADING;
       DocumentLoadFailed();
     }
@@ -1089,13 +1089,14 @@ void OutOfProcessInstance::DidOpen(int32_t result) {
   }
 }
 
-void OutOfProcessInstance::DidOpenPreview(int32_t result) {
+void OutOfProcessInstance::DidOpenPreview(std::unique_ptr<UrlLoader> loader,
+                                          int32_t result) {
   if (result == PP_OK) {
     preview_client_ = std::make_unique<PreviewModeClient>(this);
     preview_engine_ =
         std::make_unique<PDFiumEngine>(preview_client_.get(),
                                        /*enable_javascript=*/false);
-    preview_engine_->HandleDocumentLoad(embed_preview_loader_);
+    preview_engine_->HandleDocumentLoad(std::move(loader));
   } else {
     NOTREACHED();
   }
@@ -1472,13 +1473,11 @@ void OutOfProcessInstance::SubmitForm(const std::string& url,
 }
 
 void OutOfProcessInstance::FormDidOpen(int32_t result) {
-  // TODO: inform the user of success/failure.
-  if (result != PP_OK) {
-    LOG(ERROR) << "FormDidOpen failed: " << result;
-  }
+  // TODO(crbug.com/719344): Process response.
+  LOG_IF(ERROR, result != PP_OK) << "FormDidOpen failed: " << result;
 }
 
-scoped_refptr<UrlLoader> OutOfProcessInstance::CreateUrlLoader() {
+std::unique_ptr<UrlLoader> OutOfProcessInstance::CreateUrlLoader() {
   if (full_) {
     if (!did_call_start_loading_) {
       did_call_start_loading_ = true;
@@ -2092,18 +2091,17 @@ void OutOfProcessInstance::LoadUrl(const std::string& url,
   request.method = "GET";
   request.ignore_redirects = true;
 
-  scoped_refptr<UrlLoader>& loader =
-      is_print_preview ? embed_preview_loader_ : embed_loader_;
-  loader = CreateUrlLoaderInternal();
-  loader->Open(
+  std::unique_ptr<UrlLoader> loader = CreateUrlLoaderInternal();
+  UrlLoader* raw_loader = loader.get();
+  raw_loader->Open(
       request,
       base::BindOnce(is_print_preview ? &OutOfProcessInstance::DidOpenPreview
                                       : &OutOfProcessInstance::DidOpen,
-                     weak_factory_.GetWeakPtr()));
+                     weak_factory_.GetWeakPtr(), std::move(loader)));
 }
 
-scoped_refptr<UrlLoader> OutOfProcessInstance::CreateUrlLoaderInternal() {
-  auto loader = base::MakeRefCounted<PepperUrlLoader>(this);
+std::unique_ptr<UrlLoader> OutOfProcessInstance::CreateUrlLoaderInternal() {
+  auto loader = std::make_unique<PepperUrlLoader>(this);
   loader->GrantUniversalAccess();
   return loader;
 }

pdf/out_of_process_instance.h

@@ -16,7 +16,6 @@
 
 #include "base/callback.h"
 #include "base/containers/queue.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "pdf/paint_manager.h"
 #include "pdf/pdf_view_plugin_base.h"
@@ -106,8 +105,8 @@ class OutOfProcessInstance : public PdfViewPluginBase,
                         const PP_PdfPrintSettings_Dev* pdf_print_settings);
 
   void FlushCallback(int32_t result);
-  void DidOpen(int32_t result);
-  void DidOpenPreview(int32_t result);
+  void DidOpen(std::unique_ptr<UrlLoader> loader, int32_t result);
+  void DidOpenPreview(std::unique_ptr<UrlLoader> loader, int32_t result);
 
   // PdfViewPluginBase implementation.
   void ProposeDocumentLayout(const DocumentLayout& layout) override;
@@ -145,7 +144,7 @@ class OutOfProcessInstance : public PdfViewPluginBase,
   void SubmitForm(const std::string& url,
                   const void* data,
                   int length) override;
-  scoped_refptr<UrlLoader> CreateUrlLoader() override;
+  std::unique_ptr<UrlLoader> CreateUrlLoader() override;
   std::vector<SearchStringResult> SearchString(const base::char16* string,
                                                const base::char16* term,
                                                bool case_sensitive) override;
@@ -224,7 +223,7 @@ class OutOfProcessInstance : public PdfViewPluginBase,
 
   // Creates a URL loader and allows it to access all urls, i.e. not just the
   // frame's origin.
-  scoped_refptr<UrlLoader> CreateUrlLoaderInternal();
+  std::unique_ptr<UrlLoader> CreateUrlLoaderInternal();
 
   bool CanSaveEdits() const;
   void SaveToFile(const std::string& token);
@@ -338,11 +337,6 @@ class OutOfProcessInstance : public PdfViewPluginBase,
   pp::ImageData image_data_;
   SkBitmap skia_image_data_;  // Must be kept in sync with |image_data_|.
 
-  // Used when the plugin is embedded in a page and we have to create the loader
-  // ourself.
-  scoped_refptr<UrlLoader> embed_loader_;
-  scoped_refptr<UrlLoader> embed_preview_loader_;
-
   // The current cursor.
   PP_CursorType_Dev cursor_ = PP_CURSORTYPE_POINTER;
 
@@ -435,7 +429,7 @@ class OutOfProcessInstance : public PdfViewPluginBase,
   std::string url_;
 
   // Used for submitting forms.
-  scoped_refptr<UrlLoader> form_loader_;
+  std::unique_ptr<UrlLoader> form_loader_;
 
   // The callback for receiving the password from the page.
   base::OnceCallback<void(const std::string&)> password_callback_;

pdf/pdf_engine.h

@@ -7,12 +7,12 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/containers/span.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/time/time.h"
@@ -221,7 +221,7 @@ class PDFEngine {
                             int length) {}
 
     // Creates and returns new URL loader for partial document requests.
-    virtual scoped_refptr<UrlLoader> CreateUrlLoader() = 0;
+    virtual std::unique_ptr<UrlLoader> CreateUrlLoader() = 0;
 
     // Searches the given string for "term" and returns the results.  Unicode-
     // aware.
@@ -337,7 +337,7 @@ class PDFEngine {
                      std::vector<gfx::Rect>& ready,
                      std::vector<gfx::Rect>& pending) = 0;
   virtual void PostPaint() = 0;
-  virtual bool HandleDocumentLoad(scoped_refptr<UrlLoader> loader) = 0;
+  virtual bool HandleDocumentLoad(std::unique_ptr<UrlLoader> loader) = 0;
   virtual bool HandleEvent(const InputEvent& event) = 0;
   virtual uint32_t QuerySupportedPrintOutputFormats() = 0;
   virtual void PrintBegin() = 0;

pdf/pdf_view_web_plugin.cc

@@ -10,7 +10,6 @@
 #include <vector>
 
 #include "base/check_op.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/thread_annotations.h"
 #include "base/threading/thread_checker.h"
 #include "cc/paint/paint_canvas.h"
@@ -207,7 +206,7 @@ void PdfViewWebPlugin::SubmitForm(const std::string& url,
                                   const void* data,
                                   int length) {}
 
-scoped_refptr<UrlLoader> PdfViewWebPlugin::CreateUrlLoader() {
+std::unique_ptr<UrlLoader> PdfViewWebPlugin::CreateUrlLoader() {
   return nullptr;
 }
 

pdf/pdf_view_web_plugin.h

@@ -79,7 +79,7 @@ class PdfViewWebPlugin final : public PdfViewPluginBase,
   void SubmitForm(const std::string& url,
                   const void* data,
                   int length) override;
-  scoped_refptr<UrlLoader> CreateUrlLoader() override;
+  std::unique_ptr<UrlLoader> CreateUrlLoader() override;
   std::vector<SearchStringResult> SearchString(const base::char16* string,
                                                const base::char16* term,
                                                bool case_sensitive) override;

pdf/pdfium/pdfium_engine.cc

@@ -20,7 +20,6 @@
 #include "base/check_op.h"
 #include "base/debug/alias.h"
 #include "base/feature_list.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/notreached.h"
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
@@ -632,7 +631,7 @@ void PDFiumEngine::PostPaint() {
   }
 }
 
-bool PDFiumEngine::HandleDocumentLoad(scoped_refptr<UrlLoader> loader) {
+bool PDFiumEngine::HandleDocumentLoad(std::unique_ptr<UrlLoader> loader) {
   password_tries_remaining_ = kMaxPasswordTries;
   process_when_pending_request_complete_ =
       base::FeatureList::IsEnabled(features::kPdfIncrementalLoading);

pdf/pdfium/pdfium_engine.h

@@ -93,7 +93,7 @@ class PDFiumEngine : public PDFEngine,
              std::vector<gfx::Rect>& ready,
              std::vector<gfx::Rect>& pending) override;
   void PostPaint() override;
-  bool HandleDocumentLoad(scoped_refptr<UrlLoader> loader) override;
+  bool HandleDocumentLoad(std::unique_ptr<UrlLoader> loader) override;
   bool HandleEvent(const InputEvent& event) override;
   uint32_t QuerySupportedPrintOutputFormats() override;
   void PrintBegin() override;

pdf/ppapi_migration/url_loader.h

@@ -11,7 +11,6 @@
 #include <string>
 
 #include "base/containers/span.h"
-#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/optional.h"
 #include "pdf/ppapi_migration/callback.h"
@@ -72,10 +71,11 @@ struct UrlResponse final {
 };
 
 // Abstraction for a Blink or Pepper URL loader.
-class UrlLoader : public base::RefCounted<UrlLoader> {
+class UrlLoader {
  public:
   UrlLoader(const UrlLoader&) = delete;
   UrlLoader& operator=(const UrlLoader&) = delete;
+  virtual ~UrlLoader();
 
   // Tries to grant the loader the capability to make unrestricted cross-origin
   // requests ("universal access," in `blink::SecurityOrigin` terms). Must be
@@ -97,13 +97,10 @@ class UrlLoader : public base::RefCounted<UrlLoader> {
 
  protected:
   UrlLoader();
-  virtual ~UrlLoader();
 
   UrlResponse& mutable_response() { return response_; }
 
  private:
-  friend class base::RefCounted<UrlLoader>;
-
   UrlResponse response_;
 };
 
@@ -130,6 +127,7 @@ class BlinkUrlLoader final : public UrlLoader,
   explicit BlinkUrlLoader(base::WeakPtr<Client> client);
   BlinkUrlLoader(const BlinkUrlLoader&) = delete;
   BlinkUrlLoader& operator=(const BlinkUrlLoader&) = delete;
+  ~BlinkUrlLoader() override;
 
   // UrlLoader:
   void GrantUniversalAccess() override;
@@ -154,9 +152,6 @@ class BlinkUrlLoader final : public UrlLoader,
   void DidFail(const blink::WebURLError& error) override;
 
  private:
-  // Private because the class is RefCounted.
-  ~BlinkUrlLoader() override;
-
   base::WeakPtr<Client> client_;
   bool grant_universal_access_ = false;
 
@@ -169,6 +164,7 @@ class PepperUrlLoader final : public UrlLoader {
   explicit PepperUrlLoader(pp::InstanceHandle plugin_instance);
   PepperUrlLoader(const PepperUrlLoader&) = delete;
   PepperUrlLoader& operator=(const PepperUrlLoader&) = delete;
+  ~PepperUrlLoader() override;
 
   // UrlLoader:
   void GrantUniversalAccess() override;
@@ -180,9 +176,6 @@ class PepperUrlLoader final : public UrlLoader {
   void Close() override;
 
  private:
-  // Private because the class is RefCounted.
-  ~PepperUrlLoader() override;
-
   void DidOpen(ResultCallback callback, int32_t result);
 
   pp::InstanceHandle plugin_instance_;

pdf/ppapi_migration/url_loader_unittest.cc

@@ -9,7 +9,6 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/single_thread_task_runner.h"
 #include "base/test/mock_callback.h"
@@ -70,7 +69,7 @@ class BlinkUrlLoaderTest : public testing::Test {
     ON_CALL(mock_client_, CreateAssociatedURLLoader(_))
         .WillByDefault(
             Invoke(this, &BlinkUrlLoaderTest::FakeCreateAssociatedURLLoader));
-    loader_ = base::MakeRefCounted<BlinkUrlLoader>(mock_client_.GetWeakPtr());
+    loader_ = std::make_unique<BlinkUrlLoader>(mock_client_.GetWeakPtr());
   }
 
   std::unique_ptr<blink::WebAssociatedURLLoader> FakeCreateAssociatedURLLoader(
@@ -82,7 +81,7 @@ class BlinkUrlLoaderTest : public testing::Test {
 
   NiceMock<MockBlinkUrlLoaderClient> mock_client_;
   base::MockCallback<ResultCallback> mock_callback_;
-  scoped_refptr<BlinkUrlLoader> loader_;
+  std::unique_ptr<BlinkUrlLoader> loader_;
 
   std::unique_ptr<MockWebAssociatedURLLoader> mock_url_loader_ =
       std::make_unique<MockWebAssociatedURLLoader>();

pdf/preview_mode_client.cc

@@ -6,11 +6,11 @@
 
 #include <stdint.h>
 
+#include <memory>
 #include <string>
 #include <utility>
 
 #include "base/callback.h"
-#include "base/memory/scoped_refptr.h"
 #include "base/notreached.h"
 #include "pdf/document_layout.h"
 #include "pdf/ppapi_migration/url_loader.h"
@@ -116,7 +116,7 @@ void PreviewModeClient::SubmitForm(const std::string& url,
   NOTREACHED();
 }
 
-scoped_refptr<UrlLoader> PreviewModeClient::CreateUrlLoader() {
+std::unique_ptr<UrlLoader> PreviewModeClient::CreateUrlLoader() {
   NOTREACHED();
   return nullptr;
 }

pdf/preview_mode_client.h

@@ -62,7 +62,7 @@ class PreviewModeClient : public PDFEngine::Client {
   void SubmitForm(const std::string& url,
                   const void* data,
                   int length) override;
-  scoped_refptr<UrlLoader> CreateUrlLoader() override;
+  std::unique_ptr<UrlLoader> CreateUrlLoader() override;
   std::vector<SearchStringResult> SearchString(const base::char16* string,
                                                const base::char16* term,
                                                bool case_sensitive) override;

pdf/test/test_client.cc

@@ -4,7 +4,8 @@
 
 #include "pdf/test/test_client.h"
 
-#include "base/memory/scoped_refptr.h"
+#include <memory>
+
 #include "pdf/document_layout.h"
 #include "pdf/ppapi_migration/url_loader.h"
 
@@ -35,7 +36,7 @@ std::string TestClient::GetURL() {
   return std::string();
 }
 
-scoped_refptr<UrlLoader> TestClient::CreateUrlLoader() {
+std::unique_ptr<UrlLoader> TestClient::CreateUrlLoader() {
   return nullptr;
 }
 

pdf/test/test_client.h

@@ -30,7 +30,7 @@ class TestClient : public PDFEngine::Client {
   std::string Prompt(const std::string& question,
                      const std::string& default_answer) override;
   std::string GetURL() override;
-  scoped_refptr<UrlLoader> CreateUrlLoader() override;
+  std::unique_ptr<UrlLoader> CreateUrlLoader() override;
   std::vector<SearchStringResult> SearchString(const base::char16* string,
                                                const base::char16* term,
                                                bool case_sensitive) override;

pdf/url_loader_wrapper_impl.cc

@@ -9,6 +9,7 @@
 #include <stdlib.h>
 #include <string.h>
 
+#include <memory>
 #include <string>
 #include <utility>
 
@@ -100,7 +101,8 @@ bool IsDoubleEndLineAtEnd(const char* buffer, int size) {
 
 }  // namespace
 
-URLLoaderWrapperImpl::URLLoaderWrapperImpl(scoped_refptr<UrlLoader> url_loader)
+URLLoaderWrapperImpl::URLLoaderWrapperImpl(
+    std::unique_ptr<UrlLoader> url_loader)
     : url_loader_(std::move(url_loader)) {
   SetHeadersFromLoader();
 }

pdf/url_loader_wrapper_impl.h

@@ -10,7 +10,6 @@
 #include <memory>
 #include <string>
 
-#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
 #include "pdf/ppapi_migration/callback.h"
@@ -23,7 +22,7 @@ class UrlLoader;
 
 class URLLoaderWrapperImpl : public URLLoaderWrapper {
  public:
-  explicit URLLoaderWrapperImpl(scoped_refptr<UrlLoader> url_loader);
+  explicit URLLoaderWrapperImpl(std::unique_ptr<UrlLoader> url_loader);
   URLLoaderWrapperImpl(const URLLoaderWrapperImpl&) = delete;
   URLLoaderWrapperImpl& operator=(const URLLoaderWrapperImpl&) = delete;
   ~URLLoaderWrapperImpl() override;
@@ -59,7 +58,7 @@ class URLLoaderWrapperImpl : public URLLoaderWrapper {
 
   void ReadResponseBodyImpl(ResultCallback callback);
 
-  scoped_refptr<UrlLoader> url_loader_;
+  std::unique_ptr<UrlLoader> url_loader_;
   std::string response_headers_;
 
   int content_length_ = -1;