base: Make RefCountedBytes::TakeVector return ref ptr instead of raw ptr

This patch changes RefCountedBytes::TakeVector to return a ref ptr instead of a raw pointer. This minimizes the changes of a leak if a caller forgets to delete it, or forgets to wrap it in a ref ptr. R=danakj@chromium.org, thakis@chromium.org TBR=thakis@chromium.org BUG=595163 Review URL: https://codereview.chromium.org/1803263002 Cr-Commit-Position: refs/heads/master@{#381512}
marcosholgado · Mar 16, 2016 · 219dc04 · 219dc04
1 parent 5db661f
commit 219dc04
Show file tree

Hide file tree

Showing 11 changed files with 33 additions and 27 deletions.
diff --git a/base/memory/ref_counted_memory.cc b/base/memory/ref_counted_memory.cc
@@ -38,9 +38,9 @@ RefCountedBytes::RefCountedBytes(const std::vector<unsigned char>& initializer)
 RefCountedBytes::RefCountedBytes(const unsigned char* p, size_t size)
     : data_(p, p + size) {}
 
-RefCountedBytes* RefCountedBytes::TakeVector(
+scoped_refptr<RefCountedBytes> RefCountedBytes::TakeVector(
     std::vector<unsigned char>* to_destroy) {
-  RefCountedBytes* bytes = new RefCountedBytes;
+  scoped_refptr<RefCountedBytes> bytes(new RefCountedBytes);
   bytes->data_.swap(*to_destroy);
   return bytes;
 }

diff --git a/base/memory/ref_counted_memory.h b/base/memory/ref_counted_memory.h
@@ -81,7 +81,8 @@ class BASE_EXPORT RefCountedBytes : public RefCountedMemory {
   // Constructs a RefCountedBytes object by performing a swap. (To non
   // destructively build a RefCountedBytes, use the constructor that takes a
   // vector.)
-  static RefCountedBytes* TakeVector(std::vector<unsigned char>* to_destroy);
+  static scoped_refptr<RefCountedBytes> TakeVector(
+      std::vector<unsigned char>* to_destroy);
 
   // Overridden from RefCountedMemory:
   const unsigned char* front() const override;

diff --git a/chrome/browser/history/android/sqlite_cursor_unittest.cc b/chrome/browser/history/android/sqlite_cursor_unittest.cc
@@ -151,7 +151,7 @@ TEST_F(SQLiteCursorTest, Run) {
   row.set_url(GURL("http://www.google.com/"));
   std::vector<unsigned char> favicon_data;
   favicon_data.push_back(1);
-  base::RefCountedBytes *data_bytes =
+  scoped_refptr<base::RefCountedBytes> data_bytes =
       base::RefCountedBytes::TakeVector(&favicon_data);
   row.set_favicon(data_bytes);
   row.set_last_visit_time(Time::Now());

diff --git a/chrome/browser/printing/cloud_print/privet_http_unittest.cc b/chrome/browser/printing/cloud_print/privet_http_unittest.cc
@@ -771,8 +771,7 @@ class PrivetLocalPrintTest : public PrivetHTTPTest {
       std::string str) {
     std::vector<unsigned char> str_vec;
     str_vec.insert(str_vec.begin(), str.begin(), str.end());
-    return scoped_refptr<base::RefCountedBytes>(
-        base::RefCountedBytes::TakeVector(&str_vec));
+    return base::RefCountedBytes::TakeVector(&str_vec);
   }
 
  protected:

diff --git a/chrome/browser/printing/print_preview_data_service.cc b/chrome/browser/printing/print_preview_data_service.cc
@@ -41,11 +41,12 @@ class PrintPreviewDataStore : public base::RefCounted<PrintPreviewDataStore> {
   }
 
   // Set/Update the preview data entry for the specified |index|.
-  void SetPreviewDataForIndex(int index, const base::RefCountedBytes* data) {
+  void SetPreviewDataForIndex(int index,
+                              scoped_refptr<base::RefCountedBytes> data) {
     if (IsInvalidIndex(index))
       return;
 
-    page_data_map_[index] = const_cast<base::RefCountedBytes*>(data);
+    page_data_map_[index] = std::move(data);
   }
 
   // Returns the available draft page count.
@@ -103,11 +104,14 @@ void PrintPreviewDataService::GetDataEntry(
 void PrintPreviewDataService::SetDataEntry(
     int32_t preview_ui_id,
     int index,
-    const base::RefCountedBytes* data_bytes) {
-  if (!ContainsKey(data_store_map_, preview_ui_id))
-    data_store_map_[preview_ui_id] = new PrintPreviewDataStore();
+    scoped_refptr<base::RefCountedBytes> data_bytes) {
+  if (!ContainsKey(data_store_map_, preview_ui_id)) {
+    data_store_map_[preview_ui_id] =
+        make_scoped_refptr(new PrintPreviewDataStore());
+  }
 
-  data_store_map_[preview_ui_id]->SetPreviewDataForIndex(index, data_bytes);
+  data_store_map_[preview_ui_id]->SetPreviewDataForIndex(index,
+                                                         std::move(data_bytes));
 }
 
 void PrintPreviewDataService::RemoveEntry(int32_t preview_ui_id) {

diff --git a/chrome/browser/printing/print_preview_data_service.h b/chrome/browser/printing/print_preview_data_service.h
@@ -39,8 +39,9 @@ class PrintPreviewDataService {
   // preview data. Use |index| to set/update a specific preview page data.
   // NOTE: PrintPreviewDataStore owns the data. Do not refcount |data| before
   // calling this function. It will be refcounted in PrintPreviewDataStore.
-  void SetDataEntry(int32_t preview_ui_id, int index,
-                    const base::RefCountedBytes* data);
+  void SetDataEntry(int32_t preview_ui_id,
+                    int index,
+                    scoped_refptr<base::RefCountedBytes> data);
 
   // Remove the corresponding PrintPreviewUI entry from the map.
   void RemoveEntry(int32_t preview_ui_id);

diff --git a/chrome/browser/printing/print_preview_message_handler.cc b/chrome/browser/printing/print_preview_message_handler.cc
@@ -49,8 +49,9 @@ void StopWorker(int document_cookie) {
   }
 }
 
-base::RefCountedBytes* GetDataFromHandle(base::SharedMemoryHandle handle,
-                                         uint32_t data_size) {
+scoped_refptr<base::RefCountedBytes> GetDataFromHandle(
+    base::SharedMemoryHandle handle,
+    uint32_t data_size) {
   scoped_ptr<base::SharedMemory> shared_buf(
       new base::SharedMemory(handle, true));
   if (!shared_buf->Map(data_size)) {
@@ -124,11 +125,12 @@ void PrintPreviewMessageHandler::OnDidPreviewPage(
   if (!print_preview_ui)
     return;
 
-  base::RefCountedBytes* data_bytes =
+  scoped_refptr<base::RefCountedBytes> data_bytes =
       GetDataFromHandle(params.metafile_data_handle, params.data_size);
   DCHECK(data_bytes);
 
-  print_preview_ui->SetPrintPreviewDataForIndex(page_number, data_bytes);
+  print_preview_ui->SetPrintPreviewDataForIndex(page_number,
+                                                std::move(data_bytes));
   print_preview_ui->OnDidPreviewPage(page_number, params.preview_request_id);
 }
 
@@ -149,13 +151,13 @@ void PrintPreviewMessageHandler::OnMetafileReadyForPrinting(
   // TODO(joth): This seems like a good match for using RefCountedStaticMemory
   // to avoid the memory copy, but the SetPrintPreviewData call chain below
   // needs updating to accept the RefCountedMemory* base class.
-  base::RefCountedBytes* data_bytes =
+  scoped_refptr<base::RefCountedBytes> data_bytes =
       GetDataFromHandle(params.metafile_data_handle, params.data_size);
   if (!data_bytes || !data_bytes->size())
     return;
 
   print_preview_ui->SetPrintPreviewDataForIndex(COMPLETE_PREVIEW_DOCUMENT_INDEX,
-                                                data_bytes);
+                                                std::move(data_bytes));
   print_preview_ui->OnPreviewDataIsAvailable(
       params.expected_pages_count, params.preview_request_id);
 }

diff --git a/chrome/browser/themes/browser_theme_pack.cc b/chrome/browser/themes/browser_theme_pack.cc
@@ -389,7 +389,7 @@ bool HasFrameBorder() {
 }
 
 // Returns a piece of memory with the contents of the file |path|.
-base::RefCountedMemory* ReadFileData(const base::FilePath& path) {
+scoped_refptr<base::RefCountedMemory> ReadFileData(const base::FilePath& path) {
   if (!path.empty()) {
     base::File file(path, base::File::FLAG_OPEN | base::File::FLAG_READ);
     if (file.IsValid()) {
@@ -405,7 +405,7 @@ base::RefCountedMemory* ReadFileData(const base::FilePath& path) {
     }
   }
 
-  return NULL;
+  return nullptr;
 }
 
 // Shifts an image's HSL values. The caller is responsible for deleting

diff --git a/chrome/browser/ui/webui/print_preview/print_preview_ui.cc b/chrome/browser/ui/webui/print_preview/print_preview_ui.cc
@@ -442,8 +442,8 @@ void PrintPreviewUI::GetPrintPreviewDataForIndex(
 
 void PrintPreviewUI::SetPrintPreviewDataForIndex(
     int index,
-    const base::RefCountedBytes* data) {
-  print_preview_data_service()->SetDataEntry(id_, index, data);
+    scoped_refptr<base::RefCountedBytes> data) {
+  print_preview_data_service()->SetDataEntry(id_, index, std::move(data));
 }
 
 void PrintPreviewUI::ClearAllPreviewData() {

diff --git a/chrome/browser/ui/webui/print_preview/print_preview_ui.h b/chrome/browser/ui/webui/print_preview/print_preview_ui.h
@@ -51,7 +51,7 @@ class PrintPreviewUI : public ConstrainedWebDialogUI {
   // |printing::COMPLETE_PREVIEW_DOCUMENT_INDEX| to set the entire preview
   // document.
   void SetPrintPreviewDataForIndex(int index,
-                                   const base::RefCountedBytes* data);
+                                   scoped_refptr<base::RefCountedBytes> data);
 
   // Clear the existing print preview data.
   void ClearAllPreviewData();

diff --git a/ui/base/x/selection_requestor.cc b/ui/base/x/selection_requestor.cc
@@ -54,8 +54,7 @@ scoped_refptr<base::RefCountedMemory> CombineRefCountedMemory(
                          data[i]->front(),
                          data[i]->front() + data[i]->size());
   }
-  return scoped_refptr<base::RefCountedMemory>(
-      base::RefCountedBytes::TakeVector(&combined_data));
+  return base::RefCountedBytes::TakeVector(&combined_data);
 }
 
 }  // namespace