Remove bare extension pointer from platform app launcher code.

apps/launcher.cc

 // Copyright 2013 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 "apps/launcher.h"
 
 #include <set>
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
 #include "chrome/browser/extensions/api/file_handlers/mime_util.h"
 #include "chrome/browser/extensions/api/file_system/file_system_api.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/browser/api/app_runtime/app_runtime_api.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_host.h"
 #include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/granted_file_entry.h"
 #include "extensions/browser/lazy_background_task_queue.h"
 #include "extensions/browser/process_manager.h"
 #include "extensions/common/api/app_runtime.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest_handlers/kiosk_mode_info.h"
 #include "net/base/filename_util.h"
 #include "net/base/net_util.h"
 #include "url/gurl.h"
@@ skipping 50 matching lines @@
 // instances is managed by reference counted pointers. As long as an instance
 // has outstanding tasks on a message queue it will be retained; once all
 // outstanding tasks are completed it will be deleted.
 class PlatformAppPathLauncher
     : public base::RefCountedThreadSafe<PlatformAppPathLauncher> {
  public:
   PlatformAppPathLauncher(Profile* profile,
                           const Extension* extension,
                           const std::vector<base::FilePath>& file_paths)
       : profile_(profile),
-        extension_(extension),
+        extension_id(extension->id()),
         file_paths_(file_paths),
         collector_(profile) {}
 
   PlatformAppPathLauncher(Profile* profile,
                           const Extension* extension,
                           const base::FilePath& file_path)
-      : profile_(profile), extension_(extension), collector_(profile) {
+      : profile_(profile), extension_id(extension->id()), collector_(profile) {
     if (!file_path.empty())
       file_paths_.push_back(file_path);
   }
 
   void Launch() {
+    const Extension* extension = GetExtension();
+    if (!extension)
+      return;
+
     DCHECK_CURRENTLY_ON(BrowserThread::UI);

[tapted, 2014/11/20 06:15:29]

nit: maybe have this DCHECK first?

[benwells, 2014/11/20 07:07:18]

Done.

     if (file_paths_.empty()) {
       LaunchWithNoLaunchData();
       return;
     }
 
     for (size_t i = 0; i < file_paths_.size(); ++i) {
       DCHECK(file_paths_[i].IsAbsolute());
     }
 
-    if (HasFileSystemWritePermission(extension_)) {
+    if (HasFileSystemWritePermission(extension)) {
       PrepareFilesForWritableApp(
           file_paths_,
           profile_,
           false,
           base::Bind(&PlatformAppPathLauncher::OnFilesValid, this),
           base::Bind(&PlatformAppPathLauncher::OnFilesInvalid, this));
       return;
     }
 
     OnFilesValid();
@@ skipping 43 matching lines @@
     collector_.CollectForLocalPaths(
         file_paths_,
         base::Bind(&PlatformAppPathLauncher::OnMimeTypesCollected, this));
   }
 
   void OnFilesInvalid(const base::FilePath& /* error_path */) {
     LaunchWithNoLaunchData();
   }
 
   void LaunchWithNoLaunchData() {
+    const Extension* extension = GetExtension();
+    if (!extension)
+      return;
+
     // This method is required as an entry point on the UI thread.
     DCHECK_CURRENTLY_ON(BrowserThread::UI);

[tapted, 2014/11/20 06:15:29]

nit: here too

[benwells, 2014/11/20 07:07:18]

Done.

     AppRuntimeEventRouter::DispatchOnLaunchedEvent(
-        profile_, extension_, extensions::SOURCE_FILE_HANDLER);
+        profile_, extension, extensions::SOURCE_FILE_HANDLER);
   }
 
   void OnMimeTypesCollected(scoped_ptr<std::vector<std::string> > mime_types) {
     DCHECK(file_paths_.size() == mime_types->size());
 
+    const Extension* extension = GetExtension();
+    if (!extension)
+      return;
+
     // If fetching a mime type failed, then use a fallback one.
     for (size_t i = 0; i < mime_types->size(); ++i) {
       const std::string mime_type =
           !(*mime_types)[i].empty() ? (*mime_types)[i] : kFallbackMimeType;
       mime_types_.push_back(mime_type);
     }
 
     // Find file handler from the platform app for the file being opened.
     const extensions::FileHandlerInfo* handler = NULL;
     if (!handler_id_.empty()) {
-      handler = FileHandlerForId(*extension_, handler_id_);
+      handler = FileHandlerForId(*extension, handler_id_);
       if (handler) {
         for (size_t i = 0; i < file_paths_.size(); ++i) {
           if (!FileHandlerCanHandleFile(
                   *handler, mime_types_[i], file_paths_[i])) {
             LOG(WARNING)
                 << "Extension does not provide a valid file handler for "
                 << file_paths_[i].value();
             handler = NULL;
             break;
           }
         }
       }
     } else {
       std::set<std::pair<base::FilePath, std::string> > path_and_file_type_set;
       for (size_t i = 0; i < file_paths_.size(); ++i) {
         path_and_file_type_set.insert(
             std::make_pair(file_paths_[i], mime_types_[i]));
       }
       const std::vector<const extensions::FileHandlerInfo*>& handlers =
           extensions::app_file_handler_util::FindFileHandlersForFiles(
-              *extension_, path_and_file_type_set);
+              *extension, path_and_file_type_set);
       if (!handlers.empty())
         handler = handlers[0];
     }
 
     // If this app doesn't have a file handler that supports the file, launch
     // with no launch data.
     if (!handler) {
       LOG(WARNING) << "Extension does not provide a valid file handler.";
       LaunchWithNoLaunchData();
       return;
     }
 
     if (handler_id_.empty())
       handler_id_ = handler->id;
 
     // Access needs to be granted to the file for the process associated with
     // the extension. To do this the ExtensionHost is needed. This might not be
     // available, or it might be in the process of being unloaded, in which case
     // the lazy background task queue is used to load the extension and then
     // call back to us.
     extensions::LazyBackgroundTaskQueue* const queue =
         ExtensionSystem::Get(profile_)->lazy_background_task_queue();
-    if (queue->ShouldEnqueueTask(profile_, extension_)) {
+    if (queue->ShouldEnqueueTask(profile_, extension)) {
       queue->AddPendingTask(
-          profile_,
-          extension_->id(),
+          profile_, extension_id,
           base::Bind(&PlatformAppPathLauncher::GrantAccessToFilesAndLaunch,
                      this));
       return;
     }
 
     extensions::ProcessManager* const process_manager =
         extensions::ProcessManager::Get(profile_);
     ExtensionHost* const host =
-        process_manager->GetBackgroundHostForExtension(extension_->id());
+        process_manager->GetBackgroundHostForExtension(extension_id);
     DCHECK(host);
     GrantAccessToFilesAndLaunch(host);
   }
 
   void GrantAccessToFilesAndLaunch(ExtensionHost* host) {
+    const Extension* extension = GetExtension();
+    if (!extension)
+      return;
+
     // If there was an error loading the app page, |host| will be NULL.
     if (!host) {
-      LOG(ERROR) << "Could not load app page for " << extension_->id();
+      LOG(ERROR) << "Could not load app page for " << extension_id;
       return;
     }
 
     std::vector<GrantedFileEntry> file_entries;
     for (size_t i = 0; i < file_paths_.size(); ++i) {
-      file_entries.push_back(
-          CreateFileEntry(profile_,
-                          extension_,
-                          host->render_process_host()->GetID(),
-                          file_paths_[i],
-                          false));
+      file_entries.push_back(CreateFileEntry(
+          profile_, extension, host->render_process_host()->GetID(),
+          file_paths_[i], false));
     }
 
     AppRuntimeEventRouter::DispatchOnLaunchedEventWithFileEntries(
-        profile_, extension_, handler_id_, mime_types_, file_entries);
+        profile_, extension, handler_id_, mime_types_, file_entries);
+  }
+
+  const Extension* GetExtension() const {
+    return extensions::ExtensionRegistry::Get(profile_)->GetExtensionById(
+        extension_id, extensions::ExtensionRegistry::EVERYTHING);

[tapted, 2014/11/20 06:15:29]

maybe just extensions::ExtensionRegistry::ENABLED?  (i.e. if the extension is
disabled, or crashes while posting tasks, is it worth bailing out?)

[benwells, 2014/11/20 07:07:18]

I thought about that but didn't want to apply any smart logic about extension
state in this area of the code. I just want it not to crash.

   }
 
   // The profile the app should be run in.
   Profile* profile_;
-  // The extension providing the app.
-  // TODO(benwells): Hold onto the extension ID instead of a pointer as it
-  // is possible the extension will be unloaded while we're doing our thing.
-  // See http://crbug.com/372270 for details.
-  const Extension* extension_;
+  // The id of the extension providing the app. A pointer to the extension is
+  // not kept as the extension may be unloaded and deleted during the course of
+  // the launch.
+  std::string extension_id;

[tapted, 2014/11/20 06:15:29]

nit: declare const?

[benwells, 2014/11/20 07:07:18]

Done.

   // The path to be passed through to the app.
   std::vector<base::FilePath> file_paths_;
   std::vector<std::string> mime_types_;
   // The ID of the file handler used to launch the app.
   std::string handler_id_;
   extensions::app_file_handler_util::MimeTypeCollector collector_;
 
   DISALLOW_COPY_AND_ASSIGN(PlatformAppPathLauncher);
 };
 
@@ skipping 101 matching lines @@
 void LaunchPlatformAppWithUrl(Profile* profile,
                               const Extension* extension,
                               const std::string& handler_id,
                               const GURL& url,
                               const GURL& referrer_url) {
   AppRuntimeEventRouter::DispatchOnLaunchedEventWithUrl(
       profile, extension, handler_id, url, referrer_url);
 }
 
 }  // namespace apps