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

chrome/browser/web_applications/web_app_mac.h

 // Copyright (c) 2012 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.
 
 #ifndef CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_MAC_H_
 #define CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_MAC_H_
 
 #include <string>
 #include <vector>
 
 #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"
 
 namespace base {
 class CommandLine;
 }
 
 // Whether to enable update and launch of app shims in tests. (Normally shims
 // are never created or launched in tests). Note that update only creates
 // internal shim bundles, i.e. it does not create new shims in ~/Applications.
 extern bool g_app_shims_allow_update_and_launch_in_tests;
 
 namespace web_app {
 
 // Returns the full path of the .app shim that would be created by
 // CreateShortcuts().
 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);
 
 // Reveals app shim in Finder given a profile and app.
 // Calls RevealAppShimInFinderForAppOnFileThread and schedules it
 // on the FILE thread.
 void RevealAppShimInFinderForApp(Profile* profile,
                                  const extensions::Extension* app);
 
 // Creates a shortcut for a web application. The shortcut is a stub app
 // that simply loads the browser framework and runs the given app.
 class WebAppShortcutCreator {
  public:
   // 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);
 
   virtual ~WebAppShortcutCreator();
 
   // Returns the base name for the shortcut.
   base::FilePath GetShortcutBasename() const;
 
   // Returns a path to the Chrome Apps folder in the relevant applications
@@ skipping 49 matching lines @@
   // Updates the plist inside |app_path| with information about the app.
   bool UpdatePlist(const base::FilePath& app_path) const;
 
   // Updates the icon for the shortcut.
   bool UpdateIcon(const base::FilePath& app_path) const;
 
   // Path to the data directory for this app. For example:
   // ~/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_;
 
   DISALLOW_COPY_AND_ASSIGN(WebAppShortcutCreator);
 };
 
 }  // namespace web_app
 
 #endif  // CHROME_BROWSER_WEB_APPLICATIONS_WEB_APP_MAC_H_