Remove bare extension pointer from platform app launcher code.

apps/launcher.cc

Index: apps/launcher.cc
diff --git a/apps/launcher.cc b/apps/launcher.cc
index 25c3a5f2bd14e4de31ab50dfba264101792a4054..c6cdc52c4ecb86b86a235839befe62a0107f968a 100644
--- a/apps/launcher.cc
+++ b/apps/launcher.cc
@@ -27,6 +27,7 @@
 #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"
@@ -97,19 +98,23 @@ class PlatformAppPathLauncher
                           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();
@@ -120,7 +125,7 @@ class PlatformAppPathLauncher
       DCHECK(file_paths_[i].IsAbsolute());
     }
 
-    if (HasFileSystemWritePermission(extension_)) {
+    if (HasFileSystemWritePermission(extension)) {
       PrepareFilesForWritableApp(
           file_paths_,
           profile_,
@@ -184,15 +189,23 @@ class PlatformAppPathLauncher
   }
 
   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 =
@@ -203,7 +216,7 @@ class PlatformAppPathLauncher
     // 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(
@@ -224,7 +237,7 @@ class PlatformAppPathLauncher
       }
       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];
     }
@@ -247,10 +260,9 @@ class PlatformAppPathLauncher
     // 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;
@@ -259,39 +271,44 @@ class PlatformAppPathLauncher
     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_;