Destroy web_app::ShortcutInfo on UI thread

chrome/browser/web_applications/web_app.cc

Index: chrome/browser/web_applications/web_app.cc
diff --git a/chrome/browser/web_applications/web_app.cc b/chrome/browser/web_applications/web_app.cc
index d0da8543b40f3381214cef41238a0f50b9bd3003..94db1bf6ce74bd59e93f516bb7b9d92f1cc51647 100644
--- a/chrome/browser/web_applications/web_app.cc
+++ b/chrome/browser/web_applications/web_app.cc
@@ -90,15 +90,14 @@ void UpdateAllShortcutsForShortcutInfo(
     const base::Closure& callback,
     std::unique_ptr<web_app::ShortcutInfo> shortcut_info) {
   base::FilePath shortcut_data_dir = GetShortcutDataDir(*shortcut_info);
-  base::Closure task = base::Bind(&web_app::internals::UpdatePlatformShortcuts,
-                                  shortcut_data_dir, old_app_title,
-                                  base::Passed(&shortcut_info));
-  if (callback.is_null()) {
-    BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, task);
-  } else {
-    BrowserThread::PostTaskAndReply(BrowserThread::FILE, FROM_HERE, task,
-                                    callback);
-  }
+  web_app::ShortcutInfo* shortcut_info_ptr = shortcut_info.get();

[tapted, 2017/03/01 10:38:57]

(I see this as taking an extra reference to a ShortcutInfo, but worse than
refcounted - it's not counted at all :/).

[Matt Giuca, 2017/03/01 23:17:28]

The difference is that this code (you can verify by reading this and nothing
else) guarantees that |shortcut_info| outlives the pointer passed to
UpdatePlatformShortcuts.

Really this is no different to having a unique_ptr<X>, and passing it to a
function that expects a const X&; sure there's temporarily an extra reference
but there are static guarantees about the lifetime. Whereas if we used a
refcount then who knows whether UpdatePlatformShortcuts would hold onto the
reference or not.

(With the const X* version, there's a chance of miscommunication where the
receiver holds onto the pointer for longer than they're supposed to, but the
const X& version is very clear that you aren't supposed to do that.)

+  BrowserThread::PostTaskAndReply(
+      BrowserThread::FILE, FROM_HERE,
+      base::Bind(&web_app::internals::UpdatePlatformShortcuts,
+                 shortcut_data_dir, old_app_title,
+                 base::Unretained(shortcut_info_ptr)),
+      base::Bind(&web_app::internals::DeleteShortcutInfoOnUIThread,
+                 base::Passed(&shortcut_info), callback));
 }
 
 void OnImageLoaded(std::unique_ptr<web_app::ShortcutInfo> shortcut_info,
@@ -130,11 +129,16 @@ void ScheduleCreatePlatformShortcut(
     const web_app::ShortcutLocations& locations,
     std::unique_ptr<web_app::ShortcutInfo> shortcut_info) {
   base::FilePath shortcut_data_dir = GetShortcutDataDir(*shortcut_info);
-  BrowserThread::PostTask(
+
+  web_app::ShortcutInfo* shortcut_info_ptr = shortcut_info.get();
+  BrowserThread::PostTaskAndReply(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(
           base::IgnoreResult(&web_app::internals::CreatePlatformShortcuts),
-          shortcut_data_dir, base::Passed(&shortcut_info), locations, reason));
+          shortcut_data_dir, base::Unretained(shortcut_info_ptr), locations,
+          reason),
+      base::Bind(&web_app::internals::DeleteShortcutInfoOnUIThread,
+                 base::Passed(&shortcut_info), base::Closure()));
 }
 
 }  // namespace
@@ -161,6 +165,14 @@ base::FilePath GetSanitizedFileName(const base::string16& name) {
   return base::FilePath(file_name);
 }
 
+void DeleteShortcutInfoOnUIThread(
+    std::unique_ptr<web_app::ShortcutInfo> shortcut_info,
+    const base::Closure& callback) {
+  shortcut_info.reset();
+  if (callback)
+    callback.Run();
+}
+
 }  // namespace internals
 
 ShortcutInfo::ShortcutInfo() {}
@@ -430,10 +442,14 @@ void DeleteAllShortcuts(Profile* profile, const extensions::Extension* app) {
   std::unique_ptr<ShortcutInfo> shortcut_info(
       ShortcutInfoForExtensionAndProfile(app, profile));
   base::FilePath shortcut_data_dir = GetShortcutDataDir(*shortcut_info);
-  BrowserThread::PostTask(
+  web_app::ShortcutInfo* shortcut_info_ptr = shortcut_info.get();
+
+  BrowserThread::PostTaskAndReply(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&web_app::internals::DeletePlatformShortcuts,
-                 shortcut_data_dir, base::Passed(&shortcut_info)));
+                 shortcut_data_dir, base::Unretained(shortcut_info_ptr)),
+      base::Bind(&web_app::internals::DeleteShortcutInfoOnUIThread,
+                 base::Passed(&shortcut_info), base::Closure()));
 }
 
 void UpdateAllShortcuts(const base::string16& old_app_title,