Postpone setting up file handler's file permissions if handler is running lazy background page.

chrome/browser/chromeos/extensions/file_handler_util.cc

Index: chrome/browser/chromeos/extensions/file_handler_util.cc
diff --git a/chrome/browser/chromeos/extensions/file_handler_util.cc b/chrome/browser/chromeos/extensions/file_handler_util.cc
index b3ad38411c3a273c591de9b395559a0bc2cfa14d..7780ae6c4a9f39e7691dce39a776b7936430618d 100644
--- a/chrome/browser/chromeos/extensions/file_handler_util.cc
+++ b/chrome/browser/chromeos/extensions/file_handler_util.cc
@@ -14,15 +14,18 @@
 #include "chrome/browser/chromeos/gdata/gdata_util.h"
 #include "chrome/browser/chromeos/extensions/file_manager_util.h"
 #include "chrome/browser/extensions/extension_event_router.h"
+#include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/extensions/file_browser_handler.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_security_policy.h"
+#include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
@@ -449,11 +452,6 @@ class FileTaskExecutor::ExecuteTasksFileSystemCallbackDispatcher {
       }
     }
 
-    ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
-        handler_pid_,
-        final_file_path,
-        GetAccessPermissionsForHandler(handler_extension_.get(), action_id_));
-
     // Grant access to this particular file to target extension. This will
     // ensure that the target extension can access only this FS entry and
     // prevent from traversing FS hierarchy upward.
@@ -491,8 +489,8 @@ FileTaskExecutor::FileTaskExecutor(Profile* profile,
   : profile_(profile),
     source_url_(source_url),
     extension_id_(extension_id),
-    action_id_(action_id)
-{}
+    action_id_(action_id) {
+}
 
 FileTaskExecutor::~FileTaskExecutor() {}
 
@@ -508,8 +506,11 @@ bool FileTaskExecutor::Execute(const std::vector<GURL>& file_urls) {
     return false;
 
   int handler_pid = ExtractProcessFromExtensionId(handler->id(), profile_);
-  if (handler_pid < 0)
-    return false;
+  if (handler_pid <= 0) {
+    if (!handler->has_lazy_background_page())
+      return false;
+    RegisterNotificationObservers();
+  }
 
   // Get local file system instance on file thread.
   BrowserThread::PostTask(
@@ -547,21 +548,6 @@ void FileTaskExecutor::ExecuteFailedOnUIThread() {
   Done(false);
 }
 
-void FileTaskExecutor::SetupFileAccessPermissionsForGDataCache(
-    const FileDefinitionList& file_list,
-    int handler_pid) {
-  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-
-  for (FileDefinitionList::const_iterator iter = file_list.begin();
-       iter != file_list.end();
-       ++iter) {
-    if (!gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
-      continue;
-    gdata::util::SetPermissionsForGDataCacheFiles(profile_, handler_pid,
-        iter->absolute_path);
-  }
-}
-
 void FileTaskExecutor::ExecuteFileActionsOnUIThread(
     const std::string& file_system_name,
     const GURL& file_system_root,
@@ -587,7 +573,10 @@ void FileTaskExecutor::ExecuteFileActionsOnUIThread(
     return;
   }
 
-  SetupFileAccessPermissionsForGDataCache(file_list, handler_pid);
+  InitHandlerHostFileAccessPermissions(file_list, extension, action_id_);
+
+  if (handler_pid > 0)
+    SetupHandlerHostFileAccessPermissions(handler_pid);
 
   scoped_ptr<ListValue> event_args(new ListValue());
   event_args->Append(Value::CreateStringValue(action_id_));
@@ -623,7 +612,89 @@ void FileTaskExecutor::ExecuteFileActionsOnUIThread(
       extension_id_, std::string("fileBrowserHandler.onExecute"),
       json_args, profile_,
       GURL());
-  Done(true);
+
+  // If we don't have handler process id, we'll have to wait until the
+  // background host loads so we can setup file access permissions.
+  if (handler_pid > 0)
+    Done(true);
+}
+
+void FileTaskExecutor::InitHandlerHostFileAccessPermissions(
+    const FileDefinitionList& file_list,
+    const Extension* handler_extension,
+    const std::string& action_id) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  for (FileDefinitionList::const_iterator iter = file_list.begin();
+       iter != file_list.end();
+       ++iter) {
+    // Setup permission for file's absolute file.
+    handler_host_permissions_.push_back(std::make_pair(
+        iter->absolute_path,
+        GetAccessPermissionsForHandler(handler_extension, action_id)));
+
+    if (!gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
+      continue;
+
+    // If the file is on gdata mount point, we'll have to give handler host
+    // permissions for file's gdata cache paths.
+    // This has to be called on UI thread.
+    gdata::util::InsertGDataCachePathsPermissions(profile_, iter->absolute_path,
+        &handler_host_permissions_);
+  }
+}
+
+void FileTaskExecutor::SetupHandlerHostFileAccessPermissions(int handler_pid) {
+  for (size_t i = 0; i < handler_host_permissions_.size(); i++) {
+    content::ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
+        handler_pid,
+        handler_host_permissions_[i].first,
+        handler_host_permissions_[i].second);
+  }
+
+  // We don't need this anymore.
+  handler_host_permissions_.clear();
+}
+
+void FileTaskExecutor::RegisterNotificationObservers() {
+  // We should do this only once.
+  DCHECK(registrar_.IsEmpty());

[dgozman, 2012/04/13 12:41:04]

Don't we need to unregister observers?

[tbarzic, 2012/04/13 16:07:58]

Not really. They will get unregistered when the object goes away.
And this object should be used to execute only one task, so this method should
be called at most once (so at this point registrar_ should always be empty) 

+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING,
+                 content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
+                 content::Source<Profile>(profile_));
+}
+
+void FileTaskExecutor::Observe(
+    int type,
+    const content::NotificationSource& source,
+    const content::NotificationDetails& details) {
+  switch (type) {
+    case chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING: {
+      ExtensionHost* host = content::Details<ExtensionHost>(details).ptr();
+      if (host->profile()->IsSameProfile(profile_) &&
+          host->extension()->id() == extension_id_) {
+        CHECK(host->did_stop_loading());
+        DCHECK(host->extension()->has_lazy_background_page());
+
+        SetupHandlerHostFileAccessPermissions(
+            host->render_process_host()->GetID());
+        Done(true);
+        break;
+      }
+    }
+    case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
+      UnloadedExtensionInfo* unloaded =
+          content::Details<UnloadedExtensionInfo>(details).ptr();
+      if (unloaded->extension->id() == extension_id_) {

[dgozman, 2012/04/13 12:41:04]

Why don't we check profile here (as above)?

[tbarzic, 2012/04/13 16:07:58]

NOTIFICATION_EXTENSION_UNLOADED has registered only for notifications from
|proflie_|, so we know we have the right profile (line 665)

+        Done(false);
+        break;
+      }
+    }
+    default:
+      NOTREACHED();
+      break;
+  }
 }
 
 } // namespace file_handler_util