FileSystem: Modify ObfucatedFileUtil to delete contents of the plugin private filesystem

FileSystem: Modify ObfucatedFileUtil to delete contents of the plugin private filesystem

Database destruction functions haven't correctly worked for plugin private
filesystems since the filesystem has a different directory structure
from other filesystems as follows:

- PluginPrivate FS: /path/to/File System/Plugins/<origins>/<plugin_id>/<paths>/<file>
- Other FS: /path/to/File System/<origins>/<storage type>/<paths>/<file>

This patch tweaks those functions correctly to delete contents and
metadata of the plugin private filesystem.

NOTE: A part of tests is contributed by xhwang@chromium.org:
https://codereview.chromium.org/580363003/

BUG=326429
TEST=content_unittests --gtest_filter=PluginPrivateFileSystemBackendTest.*
TEST=content_unittests --gtest_filter=ObfuscatedFileUtilTest.*

Committed: https://crrev.com/ac30625dc4b1e28a685936ad48b95e8db2140af5
Cr-Commit-Position: refs/heads/master@{#303213}

[nhiroki, 2014-09-22 10:27:59]

Patchset #1 (id:1) has been deleted

[nhiroki, 2014-09-22 10:30:56]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2014-09-22 10:31:36]

nhiroki@chromium.org changed reviewers:
+ tzik@chromium.org

[nhiroki, 2014-09-22 10:31:54]

tzik@, can you review this? Thanks!

[xhwang, 2014-09-22 16:41:25]

drive-by nits...

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:920: delete database;
If you use ScopedPtrHashMap for DirectoryMap you don't need to worry about
"delete" here:

https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.h (right):

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.h:204: // filesystem.  This will
delete its directory database.
Add a note that this will delete dirs/files for all plugin types.

[nhiroki, 2014-09-24 06:00:32]

Thank you for reviewing! Updated.

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:920: delete database;
On 2014/09/22 16:41:25, xhwang wrote:
> If you use ScopedPtrHashMap for DirectoryMap you don't need to worry about
> "delete" here:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...

Good idea! I'll make a separate patch to replace them.

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.h (right):

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.h:204: // filesystem.  This will
delete its directory database.
On 2014/09/22 16:41:25, xhwang wrote:
> Add a note that this will delete dirs/files for all plugin types.

Done.

[nhiroki, 2014-09-24 09:56:59]

tzik@: ping?

[kinuko, 2014-09-24 15:51:37]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2014-09-24 15:51:37]

Drive-by.  This code looks good (looks to work) while I have a few
design-related comments.

(Also I think the CL subject should be more specific)

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:872: if (!type_string.empty())
These '!type_string.empty()' conditions (here and line 882) are added when we
introduce PluginPrivate filesystem, if we don't use this method for it I think
we should probably simply disallow empty type_string here (unless we're passing
empty type_string for other purpose).

Alternatively we could fix this method to correctly work for empty type_string
case.

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:910: bool
ObfuscatedFileUtil::DeleteOriginDirectoryForPluginPrivateFileSystem(
So far we hadn't introduced the term 'plugin' in this module
(ObfuscatedFileUtil), do we want to make this class 'PluginPrivate' aware?  To
make this module free from 'plugin' terminology, and also to make it clear that
this method works similar to DeleteDirectoryForOriginAndType(), it might make
better sense to rename this to DeleteDirectoryForOrigin.

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:932: }
I wonder if supporting type_string.empty() case in DestroyDirectoryDatabase()
(by moving 
moving line 913-921 into DestroyDirectoryDatabase()) and just calling it instead
of line 913-932 might make the code a bit simpler, less dup?  (Either we add
this new method or not)  Wdyt?

[kinuko, 2014-09-24 15:59:29]

kinuko@chromium.org changed reviewers:
- kinuko@chromium.org

[tzik, 2014-09-24 18:48:12]

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:919: directories_.erase(iter++);
As we chatted locally. (Sorry, I forget to leave that as a written text.)
This deletes http://example.com:1234 for http://example.com/12 deletion.
Please add extra check to ensure iter->first is the database to delete.
And for that, adding a separator between origin and type_string will be easiest
way.

[nhiroki, 2014-09-25 10:26:39]

Patchset #4 (id:100001) has been deleted

[nhiroki, 2014-09-25 10:34:16]

Thank you for reviewing. Can you take another look?

(CL title and description will be updated later)

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/60001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:920: delete database;
On 2014/09/24 06:00:31, nhiroki wrote:
> On 2014/09/22 16:41:25, xhwang wrote:
> > If you use ScopedPtrHashMap for DirectoryMap you don't need to worry about
> > "delete" here:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/sc...
> 
> Good idea! I'll make a separate patch to replace them.

I tried to replace them with ScopedPtrHashMap in
https://codereview.chromium.org/602043002/ and found we cannot use it in this
case because the hash map is not ordered and doesn't work for the prefix match.

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:872: if (!type_string.empty())
On 2014/09/24 15:51:37, kinuko (slow to respond) wrote:
> These '!type_string.empty()' conditions (here and line 882) are added when we
> introduce PluginPrivate filesystem, if we don't use this method for it I think
> we should probably simply disallow empty type_string here (unless we're
passing
> empty type_string for other purpose).
> 
> Alternatively we could fix this method to correctly work for empty type_string
> case.

I chose the latter way and made this function usable for the empty type_string
case.

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:910: bool
ObfuscatedFileUtil::DeleteOriginDirectoryForPluginPrivateFileSystem(
On 2014/09/24 15:51:37, kinuko (slow to respond) wrote:
> So far we hadn't introduced the term 'plugin' in this module
> (ObfuscatedFileUtil), do we want to make this class 'PluginPrivate' aware?  To
> make this module free from 'plugin' terminology, and also to make it clear
that
> this method works similar to DeleteDirectoryForOriginAndType(), it might make
> better sense to rename this to DeleteDirectoryForOrigin.

You're right. We don't have to add a specialized function for the plugin private
filesystem. I made DeleteDirectoryForOriginAndType() available for the empty
type_string case and let PluginPrivateFSBackend call it.

(At first I thought a separated function makes things simpler :)

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:919: directories_.erase(iter++);
On 2014/09/24 18:48:12, tzik wrote:
> As we chatted locally. (Sorry, I forget to leave that as a written text.)
> This deletes http://example.com:1234 for http://example.com/12 deletion.

"http://example.com/12" -> "http://example.com:12" (just in case)

> Please add extra check to ensure iter->first is the database to delete.
> And for that, adding a separator between origin and type_string will be
easiest
> way.

Thank you for pointing it out! I added a whitespace (' ') as a key separator
which is never used for origins and readable when debugging. Wdyt?

https://codereview.chromium.org/579083004/diff/80001/storage/browser/fileapi/...
storage/browser/fileapi/obfuscated_file_util.cc:932: }
On 2014/09/24 15:51:37, kinuko (slow to respond) wrote:
> I wonder if supporting type_string.empty() case in DestroyDirectoryDatabase()
> (by moving 
> moving line 913-921 into DestroyDirectoryDatabase()) and just calling it
instead
> of line 913-932 might make the code a bit simpler, less dup?  (Either we add
> this new method or not)  Wdyt?

(Removed entirely this function)

[kinuko, 2014-09-25 16:33:01]

lgtm (assuming tzik@ will take a closer look :))

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
storage/browser/fileapi/obfuscated_file_util.cc:881:
GetDirectoryForOrigin(origin, false, NULL).value());
nit: This is checking the return val of GetDirectoryForOrigin() to the same
value.

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
storage/browser/fileapi/obfuscated_file_util.cc:919: // If |type_string| is
empty, delete all filesystem types under |origin|.
Probably we could add a test to make sure this doesn't delete
"http://example.com:1234" for "http://example.com:12"?

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
storage/browser/fileapi/obfuscated_file_util.cc:929: return false;
It's a bit hard to say what we should do when we could destroy some of the DBs
but not all...

[tzik, 2014-09-26 02:35:28]

lgtm

[nhiroki, 2014-11-07 08:11:24]

Patchset #6 (id:160001) has been deleted

[nhiroki, 2014-11-07 08:15:42]

Sorry for my super delayed reply...

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
File storage/browser/fileapi/obfuscated_file_util.cc (right):

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
storage/browser/fileapi/obfuscated_file_util.cc:881:
GetDirectoryForOrigin(origin, false, NULL).value());
On 2014/09/25 16:33:00, kinuko (slow to respond) wrote:
> nit: This is checking the return val of GetDirectoryForOrigin() to the same
> value.

Done.

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
storage/browser/fileapi/obfuscated_file_util.cc:919: // If |type_string| is
empty, delete all filesystem types under |origin|.
On 2014/09/25 16:33:00, kinuko (slow to respond) wrote:
> Probably we could add a test to make sure this doesn't delete
> "http://example.com:1234" for "http://example.com:12"?

Added.

https://codereview.chromium.org/579083004/diff/120001/storage/browser/fileapi...
storage/browser/fileapi/obfuscated_file_util.cc:929: return false;
On 2014/09/25 16:33:00, kinuko (slow to respond) wrote:
> It's a bit hard to say what we should do when we could destroy some of the DBs
> but not all...

I see... I made DestroyDatabase() ignore the failure as the original code is
because this failure does not affect subsequent operations and the final result.

[nhiroki, 2014-11-07 09:04:52]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-11-07 09:05:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/579083004/180001

[commit-bot: I haz the power, 2014-11-07 09:56:48]

Committed patchset #6 (id:180001)

[commit-bot: I haz the power, 2014-11-07 09:58:04]

Patchset 6 (id:??) landed as
https://crrev.com/ac30625dc4b1e28a685936ad48b95e8db2140af5
Cr-Commit-Position: refs/heads/master@{#303213}