Fixed thread-unsafe use of gfx::Image in app shortcut creation.

chrome/browser/web_applications/web_app_mac.h

Index: chrome/browser/web_applications/web_app_mac.h
diff --git a/chrome/browser/web_applications/web_app_mac.h b/chrome/browser/web_applications/web_app_mac.h
index 49964a300097e6587c354d7e3cb75f76563df761..b41a04692a7b85ee1a75e1cd698a667e002609e8 100644
--- a/chrome/browser/web_applications/web_app_mac.h
+++ b/chrome/browser/web_applications/web_app_mac.h
@@ -11,6 +11,7 @@
 #include "base/basictypes.h"
 #include "base/files/file_path.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/scoped_ptr.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "extensions/common/manifest_handlers/file_handler_info.h"
 
@@ -30,7 +31,7 @@ namespace web_app {
 base::FilePath GetAppInstallPath(const ShortcutInfo& shortcut_info);
 
 // If necessary, launch the shortcut for an app.
-void MaybeLaunchShortcut(const ShortcutInfo& shortcut_info);
+void MaybeLaunchShortcut(scoped_ptr<ShortcutInfo> shortcut_info);
 
 // Rebuild the shortcut and relaunch it.
 bool MaybeRebuildShortcut(const base::CommandLine& command_line);
@@ -48,6 +49,8 @@ class WebAppShortcutCreator {
   // Creates a new shortcut based on information in |shortcut_info|.
   // A copy of the shortcut is placed in |app_data_dir|.
   // |chrome_bundle_id| is the CFBundleIdentifier of the Chrome browser bundle.
+  // Retains a reference to |shortcut_info|; the ShortcutInfo object must
+  // outlive the WebAppShortcutCreator.

[benwells, 2015/04/01 02:01:33]

This is unfortunate. Do you need much info from the shortcut? Can you just copy
out what you need?

[Matt Giuca, 2015/04/01 03:31:47]

Well, you do need |favicon|, which is the reason it is non-copyable in the first
place. Of course, favicon *is* copyable (with internal ref counting), so you
could copy all of the data fields including favicon into the
WebAppShortcutCreator. But then if you wanted to go down that route, you would
just make ShortcutInfo copyable again.

As I said in my previous comment, that *is* a reasonable thing to do, but it
makes it trickier to get threading right, since you might accidentally copy a
ShortcutInfo across a thread.

[Matt Giuca, 2015/04/01 03:54:26]

As discussed, not doing this.

   WebAppShortcutCreator(const base::FilePath& app_data_dir,
                         const ShortcutInfo& shortcut_info,
                         const extensions::FileHandlersInfo& file_handlers_info);
@@ -117,8 +120,8 @@ class WebAppShortcutCreator {
   // ~/Library/Application Support/Chromium/Default/Web Applications/_crx_abc/
   base::FilePath app_data_dir_;
 
-  // Information about the app.
-  ShortcutInfo info_;
+  // Information about the app. Owned by the caller of the constructor.
+  const ShortcutInfo& info_;

[benwells, 2015/04/01 02:01:33]

Maybe it would be better to use a raw pointer here. That is more typically
associated with 'owned by something else'.

[Matt Giuca, 2015/04/01 03:31:47]

OK, I'll do that if you agree that we should still not take a copy of the data
in the above discussion.

[Matt Giuca, 2015/04/01 03:54:26]

Done.

 
   // The app's file handlers.
   extensions::FileHandlersInfo file_handlers_info_;