Revert 184900

I think this revert, oddly, caused a series of failures on chromeos bots
http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20x64/builds/30936
like the top of the failures that it was supposed to have solved.

browser_tests browser_tests failed 3
Flakiness dashboard
 ( 44 mins, 15 secs )
stdio
FileSystemApiOpenBackgroundTest
FileSystemApiSaveBackgroundTest
FileAccessIsSavedToPrefs

Perhaps it wasn't the root cause after all.

This re-landing is an attempt to investigate what's going on here.

I reverted another change in the original blamelist an attempt to investigate further.
https://codereview.chromium.org/12334129/

I also tried disabling these tests to get more green in the tree.  That hasn't kicked in yet, oddly.
https://codereview.chromium.org/12334128/


> Revert "Track and persist what file entries an extension has access to."
> 
> This reverts commit 4fe61bc (r184878).
> 
> This was causing test failures in FileSystemApiOpenBackgroundTest and
> FileSystemApiSaveBackgroundTest on linux (example
> http://build.chromium.org/p/chromium.linux/buildstatus?builder=Linux%20Tests%20%28dbg%29%281%29&number=23657 )
> 
> TBR=koz@chromium.org
> BUG=
> 
> Review URL: https://codereview.chromium.org/12319140

TBR=iannucci@chromium.org
Review URL: https://codereview.chromium.org/12317156

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@185007 0039d316-1c4b-4281-b951-d872f2087c98

apps/app_restore_service.cc

@@ -5,6 +5,7 @@
 #include "apps/app_restore_service.h"
 
 #include "chrome/browser/extensions/api/app_runtime/app_runtime_api.h"
+#include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
 #include "chrome/browser/extensions/event_router.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_service.h"
@@ -47,9 +48,11 @@ void AppRestoreService::HandleStartup(bool should_restore_apps) {
       it != extensions->end(); ++it) {
     const Extension* extension = *it;
     if (extension_prefs->IsExtensionRunning(extension->id())) {
+      std::vector<SavedFileEntry> file_entries;
+      extension_prefs->GetSavedFileEntries(extension->id(), &file_entries);
       RecordAppStop(extension->id());
       if (should_restore_apps)
-        RestoreApp(*it);
+        RestoreApp(*it, file_entries);
     }
   }
 }
@@ -94,9 +97,13 @@ void AppRestoreService::RecordAppStop(const std::string& extension_id) {
   ExtensionPrefs* extension_prefs =
       ExtensionSystem::Get(profile_)->extension_service()->extension_prefs();
   extension_prefs->SetExtensionRunning(extension_id, false);
+  extension_prefs->ClearSavedFileEntries(extension_id);
 }
 
-void AppRestoreService::RestoreApp(const Extension* extension) {
+void AppRestoreService::RestoreApp(
+    const Extension* extension,
+    const std::vector<SavedFileEntry>& file_entries) {
+  // TODO(koz): Make |file_entries| available to the newly restarted app.
   AppEventRouter::DispatchOnRestartedEvent(profile_, extension);
 }
 

apps/app_restore_service.h

@@ -11,12 +11,19 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 
-class Profile;
-
 namespace extensions {
 class Extension;
+
+namespace app_file_handler_util {
+struct SavedFileEntry;
+}
+
 }
 
+class Profile;
+
+using extensions::app_file_handler_util::SavedFileEntry;
+
 namespace apps {
 
 // Tracks what apps need to be restarted when the browser restarts.
@@ -37,7 +44,9 @@ class AppRestoreService : public ProfileKeyedService,
 
   void RecordAppStart(const std::string& extension_id);
   void RecordAppStop(const std::string& extension_id);
-  void RestoreApp(const extensions::Extension* extension);
+  void RestoreApp(
+      const extensions::Extension* extension,
+      const std::vector<SavedFileEntry>& file_entries);
 
   content::NotificationRegistrar registrar_;
   Profile* profile_;

apps/app_restore_service_browsertest.cc

@@ -4,6 +4,8 @@
 
 #include "apps/app_restore_service.h"
 #include "apps/app_restore_service_factory.h"
+#include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
+#include "chrome/browser/extensions/api/file_system/file_system_api.h"
 #include "chrome/browser/extensions/extension_prefs.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
@@ -12,9 +14,12 @@
 #include "chrome/common/extensions/extension.h"
 #include "content/public/test/test_utils.h"
 
+using extensions::app_file_handler_util::SavedFileEntry;
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionSystem;
+using extensions::FileSystemChooseEntryFunction;
+
 // TODO(benwells): Move PlatformAppBrowserTest to apps namespace in apps
 // component.
 using extensions::PlatformAppBrowserTest;
@@ -52,4 +57,45 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, RunningAppsAreRecorded) {
   restart_listener.WaitUntilSatisfied();
 }
 
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, FileAccessIsSavedToPrefs) {
+  content::WindowedNotificationObserver extension_suspended(
+      chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
+      content::NotificationService::AllSources());
+
+  base::ScopedTempDir temp_directory;
+  ASSERT_TRUE(temp_directory.CreateUniqueTempDir());
+  base::FilePath temp_file;
+  ASSERT_TRUE(file_util::CreateTemporaryFileInDir(temp_directory.path(),
+                                                  &temp_file));
+
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
+      &temp_file);
+
+  ExtensionTestMessageListener file_written_listener("fileWritten", false);
+  ExtensionTestMessageListener access_ok_listener(
+      "restartedFileAccessOK", false);
+
+  const Extension* extension =
+      LoadAndLaunchPlatformApp("file_access_saved_to_prefs_test");
+  ASSERT_TRUE(extension);
+  file_written_listener.WaitUntilSatisfied();
+
+  ExtensionService* extension_service =
+      ExtensionSystem::Get(browser()->profile())->extension_service();
+  ExtensionPrefs* extension_prefs = extension_service->extension_prefs();
+
+  // Record the file entries in prefs because when the app gets suspended it
+  // will have them all cleared.
+  std::vector<SavedFileEntry> file_entries;
+  extension_prefs->GetSavedFileEntries(extension->id(), &file_entries);
+  // One for the read-only file entry and one for the writable file entry.
+  ASSERT_EQ(2u, file_entries.size());
+
+  extension_suspended.Wait();
+  file_entries.clear();
+  extension_prefs->GetSavedFileEntries(extension->id(), &file_entries);
+  // File entries should be cleared when the extension is suspended.
+  ASSERT_TRUE(file_entries.empty());
+}
+
 }  // namespace apps

chrome/browser/extensions/api/file_handlers/app_file_handler_util.cc

@@ -4,7 +4,13 @@
 
 #include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
 
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/extensions/extension_system.h"
+#include "content/public/browser/child_process_security_policy.h"
 #include "net/base/mime_util.h"
+#include "webkit/fileapi/file_system_types.h"
+#include "webkit/fileapi/isolated_context.h"
 
 namespace extensions {
 
@@ -82,6 +88,42 @@ bool FileHandlerCanHandleFileWithMimeType(
   return false;
 }
 
+GrantedFileEntry CreateFileEntry(
+    Profile* profile,
+    const std::string& extension_id,
+    int renderer_id,
+    const base::FilePath& path,
+    bool writable) {
+  GrantedFileEntry result;
+  fileapi::IsolatedContext* isolated_context =
+      fileapi::IsolatedContext::GetInstance();
+  DCHECK(isolated_context);
+
+  result.filesystem_id = isolated_context->RegisterFileSystemForPath(
+      fileapi::kFileSystemTypeNativeLocal, path, &result.registered_name);
+
+  content::ChildProcessSecurityPolicy* policy =
+      content::ChildProcessSecurityPolicy::GetInstance();
+  policy->GrantReadFileSystem(renderer_id, result.filesystem_id);
+  if (writable)
+    policy->GrantWriteFileSystem(renderer_id, result.filesystem_id);
+
+  result.id = result.filesystem_id + ":" + result.registered_name;
+
+  // We only need file level access for reading FileEntries. Saving FileEntries
+  // just needs the file system to have read/write access, which is granted
+  // above if required.
+  if (!policy->CanReadFile(renderer_id, path))
+    policy->GrantReadFile(renderer_id, path);
+
+  ExtensionPrefs* prefs = extensions::ExtensionSystem::Get(profile)->
+      extension_service()->extension_prefs();
+  // Save this file entry in the prefs.
+  prefs->AddSavedFileEntry(extension_id, result.id, path, writable);
+
+  return result;
+}
+
 }  // namespace app_file_handler_util
 
 }  // namespace extensions

chrome/browser/extensions/api/file_handlers/app_file_handler_util.h

@@ -12,6 +12,8 @@
 #include "chrome/common/extensions/api/file_handlers/file_handlers_parser.h"
 #include "chrome/common/extensions/extension.h"
 
+class Profile;
+
 namespace extensions {
 
 // TODO(benwells): move this to platform_apps namespace.
@@ -34,6 +36,38 @@ FindFileHandlersForMimeTypes(const Extension& extension,
 bool FileHandlerCanHandleFileWithMimeType(const FileHandlerInfo& handler,
                                           const std::string& mime_type);
 
+// Represents a file entry that a user has given an extension permission to
+// access. Intended to be persisted to disk (in the Preferences file), so should
+// remain serializable.
+struct SavedFileEntry {
+  SavedFileEntry(const std::string& id,
+                 const base::FilePath& path,
+                 bool writable)
+      : id(id),
+        path(path),
+        writable(writable) {}
+
+  std::string id;
+  base::FilePath path;
+  bool writable;
+};
+
+// Refers to a file entry that a renderer has been given access to.
+struct GrantedFileEntry {
+  std::string id;
+  std::string filesystem_id;
+  std::string registered_name;
+};
+
+// Creates a new file entry and allows |renderer_id| to access |path|. This
+// registers a new file system for |path|.
+GrantedFileEntry CreateFileEntry(
+    Profile* profile,
+    const std::string& extension_id,
+    int renderer_id,
+    const base::FilePath& path,
+    bool writable);
+
 }  // namespace app_file_handler_util
 
 }  // namespace extensions

chrome/browser/extensions/api/file_system/file_system_api.cc

@@ -12,6 +12,7 @@
 #include "base/string_util.h"
 #include "base/sys_string_conversions.h"
 #include "base/utf_string_conversions.h"
+#include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/platform_util.h"
 #include "chrome/browser/ui/chrome_select_file_policy.h"
@@ -40,7 +41,8 @@ using fileapi::IsolatedContext;
 
 const char kInvalidParameters[] = "Invalid parameters";
 const char kSecurityError[] = "Security error";
-const char kInvalidCallingPage[] = "Invalid calling page";
+const char kInvalidCallingPage[] = "Invalid calling page. This function can't "
+    "be called from a background page.";
 const char kUserCancelled[] = "User cancelled";
 const char kWritableFileError[] = "Invalid file for writing";
 const char kRequiresFileSystemWriteError[] =
@@ -300,27 +302,18 @@ void FileSystemEntryFunction::RegisterFileSystemAndSendResponse(
       fileapi::IsolatedContext::GetInstance();
   DCHECK(isolated_context);
 
-  std::string registered_name;
-  std::string filesystem_id = isolated_context->RegisterFileSystemForPath(
-      fileapi::kFileSystemTypeNativeLocal, path, &registered_name);
-
-  content::ChildProcessSecurityPolicy* policy =
-      content::ChildProcessSecurityPolicy::GetInstance();
-  int renderer_id = render_view_host_->GetProcess()->GetID();
-  policy->GrantReadFileSystem(renderer_id, filesystem_id);
-  if (entry_type == WRITABLE)
-    policy->GrantWriteFileSystem(renderer_id, filesystem_id);
-
-  // We only need file level access for reading FileEntries. Saving FileEntries
-  // just needs the file system to have read/write access, which is granted
-  // above if required.
-  if (!policy->CanReadFile(renderer_id, path))
-    policy->GrantReadFile(renderer_id, path);
+  bool writable = entry_type == WRITABLE;
+  extensions::app_file_handler_util::GrantedFileEntry file_entry =
+      extensions::app_file_handler_util::CreateFileEntry(profile(),
+          GetExtension()->id(), render_view_host_->GetProcess()->GetID(), path,
+          writable);
 
   DictionaryValue* dict = new DictionaryValue();
   SetResult(dict);
-  dict->SetString("fileSystemId", filesystem_id);
-  dict->SetString("baseName", registered_name);
+  dict->SetString("fileSystemId", file_entry.filesystem_id);
+  dict->SetString("baseName", file_entry.registered_name);
+  dict->SetString("id", file_entry.id);
+
   SendResponse(true);
 }
 

chrome/browser/extensions/extension_prefs.cc

@@ -12,6 +12,7 @@
 #include "base/utf_string_conversions.h"
 #include "base/version.h"
 #include "chrome/browser/extensions/admin_policy.h"
+#include "chrome/browser/extensions/api/file_handlers/app_file_handler_util.h"
 #include "chrome/browser/extensions/api/omnibox/omnibox_api.h"
 #include "chrome/browser/extensions/extension_pref_store.h"
 #include "chrome/browser/extensions/extension_sorting.h"
@@ -41,6 +42,15 @@ namespace {
 
 // Additional preferences keys
 
+// The file entries that an extension had permission to access.
+const char kFileEntries[] = "file_entries";
+
+// The path to a file entry that an extension had permission to access.
+const char kFileEntryPath[] = "path";
+
+// Whether or not an extension had write access to a file entry.
+const char kFileEntryWritable[] = "writable";
+
 // Whether this extension was running when chrome last shutdown.
 const char kPrefRunning[] = "running";
 
@@ -1132,6 +1142,61 @@ bool ExtensionPrefs::IsExtensionRunning(const std::string& extension_id) {
   return running;
 }
 
+void ExtensionPrefs::AddSavedFileEntry(
+    const std::string& extension_id,
+    const std::string& file_entry_id,
+    const base::FilePath& file_path,
+    bool writable) {
+  ScopedExtensionPrefUpdate update(prefs_, extension_id);
+  DictionaryValue* extension_dict = update.Get();
+  DictionaryValue* file_entries = NULL;
+  if (!extension_dict->GetDictionary(kFileEntries, &file_entries)) {
+    file_entries = new DictionaryValue();
+    extension_dict->SetWithoutPathExpansion(kFileEntries, file_entries);
+  }
+  // Once a file's permissions are set they can't be changed.
+  DictionaryValue* file_entry_dict = NULL;
+  if (file_entries->GetDictionary(file_entry_id, &file_entry_dict))
+    return;
+
+  file_entry_dict = new DictionaryValue();
+  file_entry_dict->SetString(kFileEntryPath, file_path.value());
+  file_entry_dict->SetBoolean(kFileEntryWritable, writable);
+  file_entries->SetWithoutPathExpansion(file_entry_id, file_entry_dict);
+}
+
+void ExtensionPrefs::ClearSavedFileEntries(
+    const std::string& extension_id) {
+  ScopedExtensionPrefUpdate update(prefs_, extension_id);
+  DictionaryValue* extension_dict = update.Get();
+  extension_dict->Remove(kFileEntries, NULL);
+}
+
+void ExtensionPrefs::GetSavedFileEntries(
+    const std::string& extension_id,
+    std::vector<app_file_handler_util::SavedFileEntry>* out) {
+  const DictionaryValue* prefs = GetExtensionPref(extension_id);
+  const DictionaryValue* file_entries = NULL;
+  if (!prefs->GetDictionary(kFileEntries, &file_entries))
+    return;
+  for (DictionaryValue::key_iterator it = file_entries->begin_keys();
+       it != file_entries->end_keys(); ++it) {
+    std::string id = *it;
+    const DictionaryValue* file_entry = NULL;
+    if (!file_entries->GetDictionaryWithoutPathExpansion(id, &file_entry))
+      continue;
+    base::FilePath::StringType path_string;
+    if (!file_entry->GetString(kFileEntryPath, &path_string))
+      continue;
+    bool writable = false;
+    if (!file_entry->GetBoolean(kFileEntryWritable, &writable))
+      continue;
+    base::FilePath file_path(path_string);
+    out->push_back(app_file_handler_util::SavedFileEntry(
+        id, file_path, writable));
+  }
+}
+
 ExtensionOmniboxSuggestion
 ExtensionPrefs::GetOmniboxDefaultSuggestion(const std::string& extension_id) {
   ExtensionOmniboxSuggestion suggestion;