Migrate URLLoaderWrapper to base::OnceCallback

Switches chrome_pdf::URLLoaderWrapper's API from pp::CompletionCallback
to base::OnceCallback. pp::CompletionCallback is still used internally
when calling Pepper APIs., but clients of URLLoaderWrapper no longer
need to deal with pp::CompletionCallback.

Using base::OnceCallback also simplifies resource management,
eliminating the need to manage references to pp::CompletionCallback.

With respect to thread safety, most Pepper APIs invoke asynchronous
callbacks on the original caller's thread, so there should be no new
thread safety issues. The base:: APIs also do sequence checking in
DCHECK mode, which should catch any unanticipated issues.

Bug: 1099022, 1101169
Change-Id: Ia355588b25b54a776ac972a7b5b2104b1a3b18c8
Cq-Do-Not-Cancel-Tryjobs: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2278169
Commit-Queue: K. Moon <kmoon@chromium.org>
Reviewed-by: Henrique Nakashima <hnakashima@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785816}

pdf/document_loader_impl.cc

@@ -10,6 +10,8 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/bind.h"
+#include "base/callback.h"
 #include "base/check_op.h"
 #include "base/notreached.h"
 #include "base/numerics/safe_math.h"
@@ -63,8 +65,7 @@ void DocumentLoaderImpl::Chunk::Clear() {
   chunk_data.reset();
 }
 
-DocumentLoaderImpl::DocumentLoaderImpl(Client* client)
-    : client_(client), loader_factory_(this) {}
+DocumentLoaderImpl::DocumentLoaderImpl(Client* client) : client_(client) {}
 
 DocumentLoaderImpl::~DocumentLoaderImpl() = default;
 
@@ -257,9 +258,9 @@ void DocumentLoaderImpl::ContinueDownload() {
 
   loader_ = client_->CreateURLLoader();
 
-  loader_->OpenRange(
-      url_, url_, start, length,
-      loader_factory_.NewCallback(&DocumentLoaderImpl::DidOpenPartial));
+  loader_->OpenRange(url_, url_, start, length,
+                     base::BindOnce(&DocumentLoaderImpl::DidOpenPartial,
+                                    weak_factory_.GetWeakPtr()));
 }
 
 void DocumentLoaderImpl::DidOpenPartial(int32_t result) {
@@ -298,7 +299,7 @@ void DocumentLoaderImpl::DidOpenPartial(int32_t result) {
 void DocumentLoaderImpl::ReadMore() {
   loader_->ReadResponseBody(
       buffer_, sizeof(buffer_),
-      loader_factory_.NewCallback(&DocumentLoaderImpl::DidRead));
+      base::BindOnce(&DocumentLoaderImpl::DidRead, weak_factory_.GetWeakPtr()));
 }
 
 void DocumentLoaderImpl::DidRead(int32_t result) {

pdf/document_loader_impl.h

@@ -5,13 +5,14 @@
 #ifndef PDF_DOCUMENT_LOADER_IMPL_H_
 #define PDF_DOCUMENT_LOADER_IMPL_H_
 
+#include <stdint.h>
+
 #include <memory>
 #include <string>
 
-#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "pdf/chunk_stream.h"
 #include "pdf/document_loader.h"
-#include "ppapi/utility/completion_callback_factory.h"
 
 namespace chrome_pdf {
 
@@ -21,6 +22,8 @@ class DocumentLoaderImpl : public DocumentLoader {
   static constexpr uint32_t kDefaultRequestSize = 65536;
 
   explicit DocumentLoaderImpl(Client* client);
+  DocumentLoaderImpl(const DocumentLoaderImpl&) = delete;
+  DocumentLoaderImpl& operator=(const DocumentLoaderImpl&) = delete;
   ~DocumentLoaderImpl() override;
 
   // DocumentLoader:
@@ -75,8 +78,6 @@ class DocumentLoaderImpl : public DocumentLoader {
   std::string url_;
   std::unique_ptr<URLLoaderWrapper> loader_;
 
-  pp::CompletionCallbackFactory<DocumentLoaderImpl> loader_factory_;
-
   DataStream chunk_stream_;
   bool partial_loading_enabled_ = true;
   bool is_partial_loader_active_ = false;
@@ -92,7 +93,7 @@ class DocumentLoaderImpl : public DocumentLoader {
 
   uint32_t bytes_received_ = 0;
 
-  DISALLOW_COPY_AND_ASSIGN(DocumentLoaderImpl);
+  base::WeakPtrFactory<DocumentLoaderImpl> weak_factory_{this};
 };
 
 }  // namespace chrome_pdf

pdf/document_loader_impl_unittest.cc

@@ -7,9 +7,12 @@
 #include <algorithm>
 #include <memory>
 #include <string>
+#include <utility>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/check.h"
+#include "pdf/ppapi_migration/callback.h"
 #include "pdf/url_loader_wrapper.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -86,38 +89,38 @@ class TestURLLoader : public URLLoaderWrapper {
       open_byte_range_ = open_byte_range;
     }
 
-    bool IsWaitRead() const { return !did_read_callback_.IsOptional(); }
-    bool IsWaitOpen() const { return !did_open_callback_.IsOptional(); }
+    bool IsWaitRead() const { return !did_read_callback_.is_null(); }
+    bool IsWaitOpen() const { return !did_open_callback_.is_null(); }
     char* buffer() const { return buffer_; }
     int buffer_size() const { return buffer_size_; }
 
-    void SetReadCallback(const pp::CompletionCallback& read_callback,
+    void SetReadCallback(ResultCallback read_callback,
                          char* buffer,
                          int buffer_size) {
-      did_read_callback_ = read_callback;
+      did_read_callback_ = std::move(read_callback);
       buffer_ = buffer;
       buffer_size_ = buffer_size;
     }
 
-    void SetOpenCallback(const pp::CompletionCallback& open_callback,
+    void SetOpenCallback(ResultCallback open_callback,
                          gfx::Range req_byte_range) {
-      did_open_callback_ = open_callback;
+      did_open_callback_ = std::move(open_callback);
       set_open_byte_range(req_byte_range);
     }
 
     void CallOpenCallback(int result) {
       DCHECK(IsWaitOpen());
-      did_open_callback_.RunAndClear(result);
+      std::move(did_open_callback_).Run(result);
     }
 
     void CallReadCallback(int result) {
       DCHECK(IsWaitRead());
-      did_read_callback_.RunAndClear(result);
+      std::move(did_read_callback_).Run(result);
     }
 
    private:
-    pp::CompletionCallback did_open_callback_;
-    pp::CompletionCallback did_read_callback_;
+    ResultCallback did_open_callback_;
+    ResultCallback did_read_callback_;
     char* buffer_ = nullptr;
     int buffer_size_ = 0;
 
@@ -171,14 +174,15 @@ class TestURLLoader : public URLLoaderWrapper {
                  const std::string& referrer_url,
                  uint32_t position,
                  uint32_t size,
-                 const pp::CompletionCallback& cc) override {
-    data_->SetOpenCallback(cc, gfx::Range(position, position + size));
+                 ResultCallback callback) override {
+    data_->SetOpenCallback(std::move(callback),
+                           gfx::Range(position, position + size));
   }
 
   void ReadResponseBody(char* buffer,
                         int buffer_size,
-                        const pp::CompletionCallback& cc) override {
-    data_->SetReadCallback(cc, buffer, buffer_size);
+                        ResultCallback callback) override {
+    data_->SetReadCallback(std::move(callback), buffer, buffer_size);
   }
 
   bool GetDownloadProgress(int64_t* bytes_received,

pdf/url_loader_wrapper.h

@@ -5,10 +5,11 @@
 #ifndef PDF_URL_LOADER_WRAPPER_H_
 #define PDF_URL_LOADER_WRAPPER_H_
 
+#include <stdint.h>
+
 #include <string>
 
-#include "base/macros.h"
-#include "ppapi/cpp/completion_callback.h"
+#include "pdf/ppapi_migration/callback.h"
 
 namespace chrome_pdf {
 
@@ -49,14 +50,15 @@ class URLLoaderWrapper {
                          const std::string& referrer_url,
                          uint32_t position,
                          uint32_t size,
-                         const pp::CompletionCallback& cc) = 0;
+                         ResultCallback callback) = 0;
 
   // Read the response body. The size of the buffer must be large enough to
   // hold the specified number of bytes to read.
   // This function might perform a partial read.
   virtual void ReadResponseBody(char* buffer,
                                 int buffer_size,
-                                const pp::CompletionCallback& cc) = 0;
+                                ResultCallback callback) = 0;
+
   // Returns the current download progress.
   // Progress only refers to the response body and does not include the headers.
   // If false, progress is unknown, bytes_received/total_bytes_to_be_received

pdf/url_loader_wrapper_impl.cc

@@ -5,13 +5,16 @@
 #include "pdf/url_loader_wrapper_impl.h"
 
 #include <memory>
+#include <utility>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/check_op.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "net/http/http_util.h"
 #include "ppapi/c/pp_errors.h"
+#include "ppapi/cpp/completion_callback.h"
 #include "ppapi/cpp/logging.h"
 #include "ppapi/cpp/url_request_info.h"
 #include "ppapi/cpp/url_response_info.h"
@@ -99,21 +102,12 @@ bool IsDoubleEndLineAtEnd(const char* buffer, int size) {
 
 URLLoaderWrapperImpl::URLLoaderWrapperImpl(pp::Instance* plugin_instance,
                                            const pp::URLLoader& url_loader)
-    : plugin_instance_(plugin_instance),
-      url_loader_(url_loader),
-      callback_factory_(this) {
+    : plugin_instance_(plugin_instance), url_loader_(url_loader) {
   SetHeadersFromLoader();
 }
 
 URLLoaderWrapperImpl::~URLLoaderWrapperImpl() {
   Close();
-  // We should call callbacks to prevent memory leaks.
-  // The callbacks don't do anything, because the objects that created the
-  // callbacks have been destroyed.
-  if (!did_open_callback_.IsOptional())
-    did_open_callback_.RunAndClear(-1);
-  if (!did_read_callback_.IsOptional())
-    did_read_callback_.RunAndClear(-1);
 }
 
 int URLLoaderWrapperImpl::GetContentLength() const {
@@ -165,35 +159,35 @@ void URLLoaderWrapperImpl::OpenRange(const std::string& url,
                                      const std::string& referrer_url,
                                      uint32_t position,
                                      uint32_t size,
-                                     const pp::CompletionCallback& cc) {
-  did_open_callback_ = cc;
-  pp::CompletionCallback callback =
-      callback_factory_.NewCallback(&URLLoaderWrapperImpl::DidOpen);
+                                     ResultCallback callback) {
+  pp::CompletionCallback cc = PPCompletionCallbackFromResultCallback(
+      base::BindOnce(&URLLoaderWrapperImpl::DidOpen, weak_factory_.GetWeakPtr(),
+                     std::move(callback)));
   int rv = url_loader_.Open(
       MakeRangeRequest(plugin_instance_, url, referrer_url, position, size),
-      callback);
+      cc);
   if (rv != PP_OK_COMPLETIONPENDING)
-    callback.Run(rv);
+    cc.Run(rv);
 }
 
 void URLLoaderWrapperImpl::ReadResponseBody(char* buffer,
                                             int buffer_size,
-                                            const pp::CompletionCallback& cc) {
-  did_read_callback_ = cc;
+                                            ResultCallback callback) {
   buffer_ = buffer;
   buffer_size_ = buffer_size;
   read_starter_.Start(
       FROM_HERE, kReadDelayMs,
       base::BindOnce(&URLLoaderWrapperImpl::ReadResponseBodyImpl,
-                     base::Unretained(this)));
+                     base::Unretained(this), std::move(callback)));
 }
 
-void URLLoaderWrapperImpl::ReadResponseBodyImpl() {
-  pp::CompletionCallback callback =
-      callback_factory_.NewCallback(&URLLoaderWrapperImpl::DidRead);
-  int rv = url_loader_.ReadResponseBody(buffer_, buffer_size_, callback);
+void URLLoaderWrapperImpl::ReadResponseBodyImpl(ResultCallback callback) {
+  pp::CompletionCallback cc = PPCompletionCallbackFromResultCallback(
+      base::BindOnce(&URLLoaderWrapperImpl::DidRead, weak_factory_.GetWeakPtr(),
+                     std::move(callback)));
+  int rv = url_loader_.ReadResponseBody(buffer_, buffer_size_, cc);
   if (rv != PP_OK_COMPLETIONPENDING) {
-    callback.Run(rv);
+    cc.Run(rv);
   }
 }
 
@@ -255,12 +249,12 @@ void URLLoaderWrapperImpl::ParseHeaders() {
   }
 }
 
-void URLLoaderWrapperImpl::DidOpen(int32_t result) {
+void URLLoaderWrapperImpl::DidOpen(ResultCallback callback, int32_t result) {
   SetHeadersFromLoader();
-  did_open_callback_.RunAndClear(result);
+  std::move(callback).Run(result);
 }
 
-void URLLoaderWrapperImpl::DidRead(int32_t result) {
+void URLLoaderWrapperImpl::DidRead(ResultCallback callback, int32_t result) {
   if (multi_part_processed_) {
     // Reset this flag so we look inside the buffer in calls of DidRead for this
     // response only once.  Note that this code DOES NOT handle multi part
@@ -269,12 +263,12 @@ void URLLoaderWrapperImpl::DidRead(int32_t result) {
     is_multipart_ = false;
   }
   if (result <= 0 || !is_multipart_) {
-    did_read_callback_.RunAndClear(result);
+    std::move(callback).Run(result);
     return;
   }
   if (result <= 2) {
     // TODO(art-snake): Accumulate data for parse headers.
-    did_read_callback_.RunAndClear(result);
+    std::move(callback).Run(result);
     return;
   }
 
@@ -297,12 +291,12 @@ void URLLoaderWrapperImpl::DidRead(int32_t result) {
   result = length;
   if (result == 0) {
     // Continue receiving.
-    return ReadResponseBodyImpl();
+    return ReadResponseBodyImpl(std::move(callback));
   }
   DCHECK_GT(result, 0);
   memmove(buffer_, start, result);
 
-  did_read_callback_.RunAndClear(result);
+  std::move(callback).Run(result);
 }
 
 void URLLoaderWrapperImpl::SetHeadersFromLoader() {

pdf/url_loader_wrapper_impl.h

@@ -5,14 +5,16 @@
 #ifndef PDF_URL_LOADER_WRAPPER_IMPL_H_
 #define PDF_URL_LOADER_WRAPPER_IMPL_H_
 
+#include <stdint.h>
+
 #include <memory>
 #include <string>
 
-#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "base/timer/timer.h"
+#include "pdf/ppapi_migration/callback.h"
 #include "pdf/url_loader_wrapper.h"
 #include "ppapi/cpp/url_loader.h"
-#include "ppapi/utility/completion_callback_factory.h"
 #include "ui/gfx/range/range.h"
 
 namespace pp {
@@ -25,6 +27,8 @@ class URLLoaderWrapperImpl : public URLLoaderWrapper {
  public:
   URLLoaderWrapperImpl(pp::Instance* plugin_instance,
                        const pp::URLLoader& url_loader);
+  URLLoaderWrapperImpl(const URLLoaderWrapperImpl&) = delete;
+  URLLoaderWrapperImpl& operator=(const URLLoaderWrapperImpl&) = delete;
   ~URLLoaderWrapperImpl() override;
 
   // URLLoaderWrapper overrides:
@@ -43,20 +47,20 @@ class URLLoaderWrapperImpl : public URLLoaderWrapper {
                  const std::string& referrer_url,
                  uint32_t position,
                  uint32_t size,
-                 const pp::CompletionCallback& cc) override;
+                 ResultCallback callback) override;
   void ReadResponseBody(char* buffer,
                         int buffer_size,
-                        const pp::CompletionCallback& cc) override;
+                        ResultCallback callback) override;
 
   void SetResponseHeaders(const std::string& response_headers);
 
  private:
   void SetHeadersFromLoader();
   void ParseHeaders();
-  void DidOpen(int32_t result);
-  void DidRead(int32_t result);
+  void DidOpen(ResultCallback callback, int32_t result);
+  void DidRead(ResultCallback callback, int32_t result);
 
-  void ReadResponseBodyImpl();
+  void ReadResponseBodyImpl(ResultCallback callback);
 
   pp::Instance* const plugin_instance_;
   pp::URLLoader url_loader_;
@@ -74,13 +78,9 @@ class URLLoaderWrapperImpl : public URLLoaderWrapper {
   uint32_t buffer_size_ = 0;
   bool multi_part_processed_ = false;
 
-  pp::CompletionCallback did_open_callback_;
-  pp::CompletionCallback did_read_callback_;
-  pp::CompletionCallbackFactory<URLLoaderWrapperImpl> callback_factory_;
-
   base::OneShotTimer read_starter_;
 
-  DISALLOW_COPY_AND_ASSIGN(URLLoaderWrapperImpl);
+  base::WeakPtrFactory<URLLoaderWrapperImpl> weak_factory_{this};
 };
 
 }  // namespace chrome_pdf