[fsp] Store mounted file systems in preferences.

[fsp] Store mounted file systems in preferences.

Providing extensions will very often mount provided file systems on startup.
When an extension is installed, the background is executed, and hence the file
system registered.

Since providing extensions should be event pages, they will also register
all of the request events, so the extension is woken up once there is a request
sent to it.

However, the background page is not run after a reboot. All of the registered
events are remembered in preferences, but mounted file systems not. As a result
after a reboot, the file systems are lost.

To overcome this issue, this CL introduces storing mounted file systems to
preferences, so they are remounted automatically after a reboot, once the
extensions are loaded. This is consistent with remembering registered
event handlers in preferences.

Note, that if the extension is gone after a reboot, then remounting will not
be performed, since it is done after the extension is loaded.

All of the mounted file systems are written to preferences during shutdown.

TEST=TBD
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274214

[mtomasz, 2014-05-26 04:50:51]

@hashimoto: PTAL. Thanks.

[hashimoto, 2014-05-26 08:35:38]

extensions::EventRouter may clear the registered events at some points.
Shouldn't we clear the remembered file systems at the same timings?

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:338:
file_system->SetString("file_system_name", it->file_system_name());
These keys should be constants.

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:344:
pref_service->Set(prefs::kFSPMountedFileSystems, extensions);
This overwrites the previously saved data.
Doesn't this result in erasing the mounted file systems when the device is
restarted twice, without loading the extension?

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:367: const
base::ListValue* file_systems;
nit: Initialize this with NULL.

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:369: if
(!extensions->GetList(extension_id, &file_systems))
Why other GetXXX calls are only DCHECKing the results while this line and L.385
abort on errors?

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

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:93: void
rememberFakeFileSystem(TestingProfile* profile,
nit: "RememberFakeFileSystem"

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:430: #endif
nit: Why did you remove this blank line?

https://codereview.chromium.org/295413002/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:873: const char kFSPMountedFileSystems[] =
"fsp.mounted_file_systems";
"FileSystemProvider" instead of "FSP"?
Our style guide discourages uses of abbreviations "unfamiliar to readers outside
your project".
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

[mtomasz, 2014-05-27 01:12:48]

On 2014/05/26 08:35:38, hashimoto wrote:
> extensions::EventRouter may clear the registered events at some points.
> Shouldn't we clear the remembered file systems at the same timings?

I don't think so. Basically what we want to achieve is: mounted file systems
should stay mounted until they are unmounted or the extension removed.
There is no dependency on event pages implementation details. Event pages are
guaranteed to run on events they register, and only this is what we depend on.

[mtomasz, 2014-05-27 01:13:14]

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:369: if
(!extensions->GetList(extension_id, &file_systems))
On 2014/05/26 08:35:38, hashimoto wrote:
> Why other GetXXX calls are only DCHECKing the results while this line and
L.385
> abort on errors?

Extension doesn't need to be in preferences, eg. if it just got installed. In
such situation, we shouldn't crash, since it is not an error.

However, #374, #379 catch malformed preferences, so we should crash.

https://codereview.chromium.org/295413002/diff/40001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/common/pref_names...
chrome/common/pref_names.cc:873: const char kFSPMountedFileSystems[] =
"fsp.mounted_file_systems";
On 2014/05/26 08:35:38, hashimoto wrote:
> "FileSystemProvider" instead of "FSP"?
> Our style guide discourages uses of abbreviations "unfamiliar to readers
outside
> your project".
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

I was torn between fsp and fileSystemProvider, but the reason for using fsp is
that it was approved here: https://codereview.chromium.org/296003012/.

If we don't want it, then chrome://fsp-internals should be renamed, eg. to
chrome://file-system-providers. WDYT?

[mtomasz, 2014-05-27 02:10:27]

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:338:
file_system->SetString("file_system_name", it->file_system_name());
On 2014/05/26 08:35:38, hashimoto wrote:
> These keys should be constants.

Done.

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:344:
pref_service->Set(prefs::kFSPMountedFileSystems, extensions);
On 2014/05/26 08:35:38, hashimoto wrote:
> This overwrites the previously saved data.
> Doesn't this result in erasing the mounted file systems when the device is
> restarted twice, without loading the extension?

Yes, this basically remembers all *currently mounted* file systems, overriding
previous values. We don't want to remember file systems which failed to remount.

Shutdown can't happen before loading is finished, so there should be no
situation that RememberFileSystems() is called, before all installed extensions
are loaded.

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:367: const
base::ListValue* file_systems;
On 2014/05/26 08:35:38, hashimoto wrote:
> nit: Initialize this with NULL.

Done.

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

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service_unittest.cc:93: void
rememberFakeFileSystem(TestingProfile* profile,
On 2014/05/26 08:35:38, hashimoto wrote:
> nit: "RememberFakeFileSystem"

Done.

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/prefs/bro...
File chrome/browser/prefs/browser_prefs.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/prefs/bro...
chrome/browser/prefs/browser_prefs.cc:430: #endif
On 2014/05/26 08:35:38, hashimoto wrote:
> nit: Why did you remove this blank line?

Accident. Done.

[mtomasz, 2014-05-27 02:13:43]

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:344:
pref_service->Set(prefs::kFSPMountedFileSystems, extensions);
On 2014/05/27 02:10:27, mtomasz wrote:
> On 2014/05/26 08:35:38, hashimoto wrote:
> > This overwrites the previously saved data.
> > Doesn't this result in erasing the mounted file systems when the device is
> > restarted twice, without loading the extension?
> 
> Yes, this basically remembers all *currently mounted* file systems, overriding
> previous values. We don't want to remember file systems which failed to
remount.
> 
> Shutdown can't happen before loading is finished, so there should be no
> situation that RememberFileSystems() is called, before all installed
extensions
> are loaded.

This is because loading is synchronous. All installed extensions are loaded
synchronously in a loop. Hence, shutdown can't happen before all of them are
loaded.

[mtomasz, 2014-05-27 02:21:06]

On 2014/05/27 02:13:43, mtomasz wrote:
>
https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/file_system_provider/service.cc (right):
> 
>
https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/file_system_provider/service.cc:344:
> pref_service->Set(prefs::kFSPMountedFileSystems, extensions);
> On 2014/05/27 02:10:27, mtomasz wrote:
> > On 2014/05/26 08:35:38, hashimoto wrote:
> > > This overwrites the previously saved data.
> > > Doesn't this result in erasing the mounted file systems when the device is
> > > restarted twice, without loading the extension?
> > 
> > Yes, this basically remembers all *currently mounted* file systems,
overriding
> > previous values. We don't want to remember file systems which failed to
> remount.
> > 
> > Shutdown can't happen before loading is finished, so there should be no
> > situation that RememberFileSystems() is called, before all installed
> extensions
> > are loaded.
> 
> This is because loading is synchronous. All installed extensions are loaded
> synchronously in a loop. Hence, shutdown can't happen before all of them are
> loaded.

I've renamed kFSPMountedFileSystems -> kFileSystemProviderMounted. PTAL.

[hashimoto, 2014-05-27 11:29:16]

On 2014/05/27 01:12:48, mtomasz wrote:
> On 2014/05/26 08:35:38, hashimoto wrote:
> > extensions::EventRouter may clear the registered events at some points.
> > Shouldn't we clear the remembered file systems at the same timings?
> 
> I don't think so. Basically what we want to achieve is: mounted file systems
> should stay mounted until they are unmounted or the extension removed.
> There is no dependency on event pages implementation details. Event pages are
> guaranteed to run on events they register, and only this is what we depend on.

Sorry, not ExtensionRouter, but ExtensionPrefs is the class which may clear the
prefs.
(https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...)

[hashimoto, 2014-05-27 11:29:24]

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:369: if
(!extensions->GetList(extension_id, &file_systems))
On 2014/05/27 01:13:14, mtomasz wrote:
> On 2014/05/26 08:35:38, hashimoto wrote:
> > Why other GetXXX calls are only DCHECKing the results while this line and
> L.385
> > abort on errors?
> 
> Extension doesn't need to be in preferences, eg. if it just got installed. In
> such situation, we shouldn't crash, since it is not an error.
> 
> However, #374, #379 catch malformed preferences, so we should crash.

Then maybe we should emit LOG for empty file_system_id and file_system_name too?

[mtomasz, 2014-05-28 02:02:18]

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/service.cc (right):

https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/service.cc:369: if
(!extensions->GetList(extension_id, &file_systems))
On 2014/05/27 11:29:24, hashimoto wrote:
> On 2014/05/27 01:13:14, mtomasz wrote:
> > On 2014/05/26 08:35:38, hashimoto wrote:
> > > Why other GetXXX calls are only DCHECKing the results while this line and
> > L.385
> > > abort on errors?
> > 
> > Extension doesn't need to be in preferences, eg. if it just got installed.
In
> > such situation, we shouldn't crash, since it is not an error.
> > 
> > However, #374, #379 catch malformed preferences, so we should crash.
> 
> Then maybe we should emit LOG for empty file_system_id and file_system_name
too?

Done.

[mtomasz, 2014-05-29 09:38:09]

On 2014/05/28 02:02:18, mtomasz wrote:
>
https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/file_system_provider/service.cc (right):
> 
>
https://codereview.chromium.org/295413002/diff/40001/chrome/browser/chromeos/...
> chrome/browser/chromeos/file_system_provider/service.cc:369: if
> (!extensions->GetList(extension_id, &file_systems))
> On 2014/05/27 11:29:24, hashimoto wrote:
> > On 2014/05/27 01:13:14, mtomasz wrote:
> > > On 2014/05/26 08:35:38, hashimoto wrote:
> > > > Why other GetXXX calls are only DCHECKing the results while this line
and
> > > L.385
> > > > abort on errors?
> > > 
> > > Extension doesn't need to be in preferences, eg. if it just got installed.
> In
> > > such situation, we shouldn't crash, since it is not an error.
> > > 
> > > However, #374, #379 catch malformed preferences, so we should crash.
> > 
> > Then maybe we should emit LOG for empty file_system_id and file_system_name
> too?
> 
> Done.

@hashimoto: Ping.

[hashimoto, 2014-05-30 01:15:08]

Sorry for being late to respond.
lgtm

[mtomasz, 2014-05-30 14:34:31]

@sky: PTAL at browser_prefs.cc. Thanks.

[sky, 2014-05-30 16:27:13]

LGTM

[mtomasz, 2014-05-31 01:05:00]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-05-31 01:09:10]

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

[commit-bot: I haz the power, 2014-05-31 05:35:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-05-31 06:54:25]

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

[commit-bot: I haz the power, 2014-05-31 06:54:26]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[mtomasz, 2014-06-02 00:29:11]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-06-02 00:29:24]

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

[commit-bot: I haz the power, 2014-06-02 01:33:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-06-02 02:49:05]

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

[commit-bot: I haz the power, 2014-06-02 02:49:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[mtomasz, 2014-06-02 07:45:12]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-06-02 07:45:37]

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

[commit-bot: I haz the power, 2014-06-02 12:37:57]

Change committed as 274214