Remove ListValue::Append(raw ptr) on Android

chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.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 "chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.h"
 
 #include <stdint.h>
 #include <stdlib.h>
+#include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/guid.h"
+#include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"

[jdoerrie, 2017/04/12 16:08:55]

#include "base/values.h"

[vabr (Chromium), 2017/04/12 16:40:52]

Done.

 #include "chrome/browser/android/offline_pages/offline_page_model_factory.h"
 #include "chrome/browser/android/offline_pages/request_coordinator_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "components/offline_pages/core/client_namespace_constants.h"
 #include "content/public/browser/web_ui.h"
 #include "net/base/network_change_notifier.h"
 
 namespace offline_internals {
 
 OfflineInternalsUIMessageHandler::OfflineInternalsUIMessageHandler()
@@ skipping 102 matching lines @@
       base::Value(callback_id),
       base::Value(GetStringFromDeleteRequestResults(results)));
 }
 
 void OfflineInternalsUIMessageHandler::HandleStoredPagesCallback(
     std::string callback_id,
     const offline_pages::MultipleOfflinePageItemResult& pages) {
   base::ListValue results;
 
   for (const auto& page : pages) {
-    base::DictionaryValue* offline_page = new base::DictionaryValue();
-    results.Append(offline_page);
+    auto offline_page = base::MakeUnique<base::DictionaryValue>();
+    results.Append(std::move(offline_page));

[jdoerrie, 2017/04/12 16:08:55]

This moves out of offline_page. Move this line to the end of this block.

[vabr (Chromium), 2017/04/12 16:40:52]

Oops, thanks for catching!

     offline_page->SetString("onlineUrl", page.url.spec());
     offline_page->SetString("namespace", page.client_id.name_space);
     offline_page->SetDouble("size", page.file_size);
     offline_page->SetString("id", std::to_string(page.offline_id));
     offline_page->SetString("filePath", page.file_path.MaybeAsASCII());
     offline_page->SetDouble("creationTime", page.creation_time.ToJsTime());
     offline_page->SetDouble("lastAccessTime", page.last_access_time.ToJsTime());
     offline_page->SetInteger("accessCount", page.access_count);
     offline_page->SetString("originalUrl", page.original_url.spec());
   }
   ResolveJavascriptCallback(base::Value(callback_id), results);
 }
 
 void OfflineInternalsUIMessageHandler::HandleRequestQueueCallback(
     std::string callback_id,
     offline_pages::GetRequestsResult result,
     std::vector<std::unique_ptr<offline_pages::SavePageRequest>> requests) {
   base::ListValue save_page_requests;
   if (result == offline_pages::GetRequestsResult::SUCCESS) {
     for (const auto& request : requests) {
-      base::DictionaryValue* save_page_request = new base::DictionaryValue();
-      save_page_requests.Append(save_page_request);
+      auto save_page_request = base::MakeUnique<base::DictionaryValue>();
+      save_page_requests.Append(std::move(save_page_request));

[jdoerrie, 2017/04/12 16:08:55]

Same here. I am surprised I didn't hit this code during development of r463684,
this code should result in a use-after-free as is.

[vabr (Chromium), 2017/04/12 16:40:52]

I guess that's either lack of test coverage in this code, or the unfortunate
property of use-after-free that it sometimes works fine by accident.

In any case, I'm wondering what I did in this file to not notice it :(, thanks
for your careful review.

       save_page_request->SetString("onlineUrl", request->url().spec());
       save_page_request->SetDouble("creationTime",
                                    request->creation_time().ToJsTime());
       save_page_request->SetString("status", GetStringFromSavePageStatus());
       save_page_request->SetString("namespace",
                                    request->client_id().name_space);
       save_page_request->SetDouble("lastAttempt",
                                    request->last_attempt_time().ToJsTime());
       save_page_request->SetString("id", std::to_string(request->request_id()));
       save_page_request->SetString("originalUrl",
@@ skipping 170 matching lines @@
 
   // Get the offline page model associated with this web ui.
   Profile* profile = Profile::FromWebUI(web_ui());
   offline_page_model_ =
       offline_pages::OfflinePageModelFactory::GetForBrowserContext(profile);
   request_coordinator_ =
       offline_pages::RequestCoordinatorFactory::GetForBrowserContext(profile);
 }
 
 }  // namespace offline_internals