[fsp] Unmount file systems when the providing extension gets unloaded.

[fsp] Unmount file systems when the providing extension gets unloaded.

Previously, if the extension got unloaded, provided the file system would still
be active, but not responsive. This CL listens for unload events, and removes
all of the provided file systems associated with the unloaded extension.

TEST=unit_tests:*FileSystemProviderService/UnmountFileSystem_OnExtensionUnload*
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=264967

[mtomasz, 2014-03-27 02:19:05]

@hashimoto: As discussed offline yesterday. PTAL, thanks!
It depends on 210803003 (sorry!)

[hashimoto, 2014-03-27 09:18:49]

looking good.

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:247: LOG(ERROR) <<
extension->id() << " vs. " << file_system_info.extension_id();
No need to corrupt the feedback log with this information.
Please remove this or use VLOG or DVLOG.

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:256: ++it;
nit: How about moving this ++it to above the if-statement to avoid the code
duplication?

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:71:
scoped_refptr<extensions::Extension> createFakeExtension(
Hmm, it's generally better to return raw-pointer than scoped_refptr from factory
functions of ref-counted objects. 
(see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CNTCtqGkk0M/YF-Q-...)

But extensions::Extension::Create is already returning scoped_refptr.
So, in this case, returning scoped_refptr seems the safer choice.

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:246:
"/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-testing_profile-hash",
This EXPECT looks fragile for changes in UserManager/Profile implementation.
Could you make this EXPECT more robust? (e.g. instead of writing this cryptic
hash down directly in this test, get the hash from the profile or somewhere
appropriate)

[mtomasz, 2014-03-28 06:01:10]

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:247: LOG(ERROR) <<
extension->id() << " vs. " << file_system_info.extension_id();
On 2014/03/27 09:18:49, hashimoto wrote:
> No need to corrupt the feedback log with this information.
> Please remove this or use VLOG or DVLOG.

Done.

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service.cc:256: ++it;
On 2014/03/27 09:18:49, hashimoto wrote:
> nit: How about moving this ++it to above the if-statement to avoid the code
> duplication?

Good idea. Done.

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:71:
scoped_refptr<extensions::Extension> createFakeExtension(
On 2014/03/27 09:18:49, hashimoto wrote:
> Hmm, it's generally better to return raw-pointer than scoped_refptr from
factory
> functions of ref-counted objects. 
> (see
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/CNTCtqGkk0M/YF-Q-...)
> 
> But extensions::Extension::Create is already returning scoped_refptr.
> So, in this case, returning scoped_refptr seems the safer choice.

I think in this case, there is no other choice. If we returned a raw pointer, it
would become invalid almost immediately.

https://codereview.chromium.org/213123008/diff/1/chrome/browser/chromeos/file...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:246:
"/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-testing_profile-hash",
On 2014/03/27 09:18:49, hashimoto wrote:
> This EXPECT looks fragile for changes in UserManager/Profile implementation.
> Could you make this EXPECT more robust? (e.g. instead of writing this cryptic
> hash down directly in this test, get the hash from the profile or somewhere
> appropriate)

I was thinking about it, but I was concerned that the test would be less easy to
read, without much advantages. It would be either:

1) EXPECT_EQ(ConstructProvidedMountPath(kExtensionId, 1), ...mount_path());

2) EXPECT_EQ("/provided/mbflcebpggnecokmikipoihdbecnjfoj-1-" +
testing_profile_hash_), ...mount_path()));

These two values: 'testing_profile and 'hash' are hardcoded in testing-only
classes, so I don't think they will change often.

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/t...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

I don't have a strong opinion here though. WDYT?

[mtomasz, 2014-04-09 21:42:08]

@hashimoto: Done. PTAL.

[hashimoto, 2014-04-18 05:35:15]

lgtm

https://codereview.chromium.org/213123008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/213123008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:83:
kExtensionId,
This should be extension_id?

[mtomasz, 2014-04-18 06:01:34]

https://codereview.chromium.org/213123008/diff/100001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/service_unittest.cc (right):

https://codereview.chromium.org/213123008/diff/100001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:83:
kExtensionId,
On 2014/04/18 05:35:15, hashimoto wrote:
> This should be extension_id?

Good catch! Done.

[mtomasz, 2014-04-18 06:01:38]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-18 06:01:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/213123008/120001

[commit-bot: I haz the power, 2014-04-18 07:42:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 07:42:29]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_clang_dbg

[mtomasz, 2014-04-18 08:42:10]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-18 08:42:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/213123008/140001

[commit-bot: I haz the power, 2014-04-18 10:45:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-18 10:45:58]

Failed to apply patch for
chrome/browser/chromeos/file_system_provider/service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/file_system_provider/service.cc
  Hunk #1 FAILED at 13.
  Hunk #3 succeeded at 224 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/file_system_provider/service.cc.rej

Patch:       chrome/browser/chromeos/file_system_provider/service.cc
Index: chrome/browser/chromeos/file_system_provider/service.cc
diff --git a/chrome/browser/chromeos/file_system_provider/service.cc
b/chrome/browser/chromeos/file_system_provider/service.cc
index
3bc173c4e776bbee6a198cad349aba0812f2576d..65e09ee21e0efaf932fb1b9ba008f5020cf5fa7e
100644
--- a/chrome/browser/chromeos/file_system_provider/service.cc
+++ b/chrome/browser/chromeos/file_system_provider/service.cc
@@ -13,7 +13,9 @@
 #include
"chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
 #include "chrome/browser/chromeos/file_system_provider/service_factory.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/common/extension.h"
 #include "webkit/browser/fileapi/external_mount_points.h"
 
 namespace chromeos {
@@ -34,13 +36,20 @@ ProvidedFileSystemInterface* CreateProvidedFileSystem(
 
 }  // namespace
 
-Service::Service(Profile* profile)
+Service::Service(Profile* profile,
+                 extensions::ExtensionRegistry* extension_registry)
     : profile_(profile),
+      extension_registry_(extension_registry),
       file_system_factory_(base::Bind(CreateProvidedFileSystem)),
       next_id_(1),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  extension_registry_->AddObserver(this);
+}
 
-Service::~Service() { STLDeleteValues(&file_system_map_); }
+Service::~Service() {
+  extension_registry_->RemoveObserver(this);
+  STLDeleteValues(&file_system_map_);
+}
 
 // static
 Service* Service::Get(content::BrowserContext* context) {
@@ -218,7 +227,23 @@ ProvidedFileSystemInterface*
Service::GetProvidedFileSystem(
   return file_system_it->second;
 }
 
-void Service::Shutdown() {}
+void Service::OnExtensionUnloaded(content::BrowserContext* browser_context,
+                                  const extensions::Extension* extension) {
+  // Unmount all of the provided file systems associated with this extension.
+  ProvidedFileSystemMap::iterator it = file_system_map_.begin();
+  while (it != file_system_map_.end()) {
+    const ProvidedFileSystemInfo& file_system_info =
+        it->second->GetFileSystemInfo();
+    // Advance the iterator beforehand, otherwise it will become invalidated
+    // by the UnmountFileSystem() call.
+    ++it;
+    if (file_system_info.extension_id() == extension->id()) {
+      bool result = UnmountFileSystem(file_system_info.extension_id(),
+                                      file_system_info.file_system_id());
+      DCHECK(result);
+    }
+  }
+}
 
 void Service::OnRequestUnmountStatus(
     const ProvidedFileSystemInfo& file_system_info,

[mtomasz, 2014-04-21 01:24:30]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-21 01:24:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/213123008/140001

[commit-bot: I haz the power, 2014-04-21 01:24:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-21 01:24:39]

Failed to apply patch for
chrome/browser/chromeos/file_system_provider/service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/file_system_provider/service.cc
  Hunk #1 FAILED at 13.
  Hunk #3 succeeded at 224 (offset -1 lines).
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/file_system_provider/service.cc.rej

Patch:       chrome/browser/chromeos/file_system_provider/service.cc
Index: chrome/browser/chromeos/file_system_provider/service.cc
diff --git a/chrome/browser/chromeos/file_system_provider/service.cc
b/chrome/browser/chromeos/file_system_provider/service.cc
index
3bc173c4e776bbee6a198cad349aba0812f2576d..65e09ee21e0efaf932fb1b9ba008f5020cf5fa7e
100644
--- a/chrome/browser/chromeos/file_system_provider/service.cc
+++ b/chrome/browser/chromeos/file_system_provider/service.cc
@@ -13,7 +13,9 @@
 #include
"chrome/browser/chromeos/file_system_provider/provided_file_system_interface.h"
 #include "chrome/browser/chromeos/file_system_provider/service_factory.h"
 #include "content/public/browser/browser_thread.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/common/extension.h"
 #include "webkit/browser/fileapi/external_mount_points.h"
 
 namespace chromeos {
@@ -34,13 +36,20 @@ ProvidedFileSystemInterface* CreateProvidedFileSystem(
 
 }  // namespace
 
-Service::Service(Profile* profile)
+Service::Service(Profile* profile,
+                 extensions::ExtensionRegistry* extension_registry)
     : profile_(profile),
+      extension_registry_(extension_registry),
       file_system_factory_(base::Bind(CreateProvidedFileSystem)),
       next_id_(1),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  extension_registry_->AddObserver(this);
+}
 
-Service::~Service() { STLDeleteValues(&file_system_map_); }
+Service::~Service() {
+  extension_registry_->RemoveObserver(this);
+  STLDeleteValues(&file_system_map_);
+}
 
 // static
 Service* Service::Get(content::BrowserContext* context) {
@@ -218,7 +227,23 @@ ProvidedFileSystemInterface*
Service::GetProvidedFileSystem(
   return file_system_it->second;
 }
 
-void Service::Shutdown() {}
+void Service::OnExtensionUnloaded(content::BrowserContext* browser_context,
+                                  const extensions::Extension* extension) {
+  // Unmount all of the provided file systems associated with this extension.
+  ProvidedFileSystemMap::iterator it = file_system_map_.begin();
+  while (it != file_system_map_.end()) {
+    const ProvidedFileSystemInfo& file_system_info =
+        it->second->GetFileSystemInfo();
+    // Advance the iterator beforehand, otherwise it will become invalidated
+    // by the UnmountFileSystem() call.
+    ++it;
+    if (file_system_info.extension_id() == extension->id()) {
+      bool result = UnmountFileSystem(file_system_info.extension_id(),
+                                      file_system_info.file_system_id());
+      DCHECK(result);
+    }
+  }
+}
 
 void Service::OnRequestUnmountStatus(
     const ProvidedFileSystemInfo& file_system_info,

[mtomasz, 2014-04-21 01:49:43]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-21 01:49:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/213123008/160001

[commit-bot: I haz the power, 2014-04-21 03:20:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-21 03:20:33]

Try jobs failed on following builders:
  tryserver.chromium on mac_chromium_rel

[mtomasz, 2014-04-21 04:30:01]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-21 04:30:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/213123008/180001

[commit-bot: I haz the power, 2014-04-21 05:04:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-21 05:04:53]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_x64_rel

[mtomasz, 2014-04-21 05:06:04]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-04-21 05:06:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtomasz@chromium.org/213123008/180001

[commit-bot: I haz the power, 2014-04-21 06:06:06]

Change committed as 264967