Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1979733002:
Add ability to clear content licenses (Closed)
DescriptionAdd ability to delete plugin private data
Currently encrypted media content licenses are stored in plugin
private data (as decryption happens in a pepper plugin). Add the
ability to erase the data on request.
As the data is organized by origin and plugin, all files for a
particular origin/plugin combination are removed if any of the files
matches the time criteria. The existing plugins only use 1 file
currently, but this should avoid issues if the plugins change to
use multiple files.
The UI is still being debated, so this is simply adding the ability
to delete the data once called.
BUG=607631
TEST=new content_unittests pass
Committed: https://crrev.com/bb33a9731f1bb62cebcab1dbdb795e6cf9ac1140
Cr-Commit-Position: refs/heads/master@{#399322}
Patch Set 1 #
Total comments: 9
Patch Set 2 : remove ContentLicense #Patch Set 3 : determine plugin list #
Total comments: 22
Patch Set 4 : changes #
Total comments: 20
Patch Set 5 : more changes #
Total comments: 3
Patch Set 6 : FileTaskRunner #
Total comments: 2
Patch Set 7 : FILE -> IO thread #
Total comments: 6
Patch Set 8 : changes #
Total comments: 16
Patch Set 9 : seperate file #
Total comments: 5
Messages
Total messages: 40 (9 generated)
jrummell@chromium.org changed reviewers: + xhwang@chromium.org
PTAL. This is the code needed to hook up with the current chrome://settings/clearBrowsingData (if needed).
Thanks!!! I didn't review details since I am really not familiar with this code. Could you please add a storage owner to review the implementation as well? I'll take another look after that. I do have some high level comments for discussion. https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... File content/browser/storage_partition_impl.cc (right): https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:46: const char kPluginPrivateRootName[] = "pluginprivate"; Is this documented anywhere? Where is this magic string coming from? https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:48: const char kClearKeyCdmPluginId[] = "application_x-ppapi-clearkey-cdm"; In other places the pepper type is application/x-ppapi-clearkey-cdm Is there a special rule about how plugin IDs are generated? https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:229: class PluginPrivateContentLicensesDeletionHelper { About naming: The UI is TBD and is subject to change. So I am not sure about the "ContentLicenses" part. Shall we stick with what we have in the implementation (i.e. PluginPrivate), and try to avoid "content licenses" in this backend code? https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:459: // directly from the filesystem? We should ask storage owners about this. https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:498: void ClearContentLicensesOnFileThread( Can we document about thread safety? For example, this runs on the same thread as other file operations (open/read/write). Also, what happens if we call this after a file is opened, but before read, or before a write? Some kind of failure is okay, but we need to guarantee the integrity of data.
jrummell@chromium.org changed reviewers: + nhiroki@chromium.org
Updated to remove references to ContentLicenses. Adding nhiroki@ as somebody familiar with the plugin filesystem. https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... File content/browser/storage_partition_impl.cc (right): https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:46: const char kPluginPrivateRootName[] = "pluginprivate"; On 2016/05/16 17:35:23, xhwang wrote: > Is this documented anywhere? Where is this magic string coming from? It comes from the pepper code, that converts the pepper filesystem type to a root name. Added a shared definition for it. https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:48: const char kClearKeyCdmPluginId[] = "application_x-ppapi-clearkey-cdm"; On 2016/05/16 17:35:23, xhwang wrote: > In other places the pepper type is application/x-ppapi-clearkey-cdm > > Is there a special rule about how plugin IDs are generated? The conversion is done in GeneratePluginId [1] which takes a mime-type and basically puts an _ between the top_level_type and the subtype. It's in pepper code, and we don't need the other validation it does ("contains only alphabets, digits, or "._-"."). [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re... https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:229: class PluginPrivateContentLicensesDeletionHelper { On 2016/05/16 17:35:23, xhwang wrote: > About naming: The UI is TBD and is subject to change. So I am not sure about the > "ContentLicenses" part. Shall we stick with what we have in the implementation > (i.e. PluginPrivate), and try to avoid "content licenses" in this backend code? Done. https://chromiumcodereview.appspot.com/1979733002/diff/1/content/browser/stor... content/browser/storage_partition_impl.cc:459: // directly from the filesystem? On 2016/05/16 17:35:23, xhwang wrote: > We should ask storage owners about this. Done.
Based on comments from nhiroki@, I've removed the plugin list and now determine the plugin list by reading the directory. https://chromiumcodereview.appspot.com/1979733002/diff/40001/content/browser/... File content/browser/storage_partition_impl.cc (right): https://chromiumcodereview.appspot.com/1979733002/diff/40001/content/browser/... content/browser/storage_partition_impl.cc:465: storage::ObfuscatedFileUtil* obfuscated_file_util = This seems ugly (it's cloned from private PluginPrivateFileSystemBackend::obfuscated_file_util()), but I can't find a better way to get the ObfuscatedFileUtil object. Open to suggestions that avoid casting, if one is possible.
https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:236: const scoped_refptr<storage::FileSystemContext>& filesystem_context, Passing "const scoped_refptr<Foo>&" is not encouraged: "What about passing or returning a smart pointer by reference" http://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-about-pa... https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:242: : filesystem_context_(std::move(filesystem_context)), IIUC, std::move for const-ref object does not move but copy. Maybe you should change the first argument on the ctor to scoped_refptr<FSContext> (or to a rawptr and then remove std::move() here). https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:285: int task_count_ = 0; In general, manual reference counting would be fragile. How about making this helper class RefCounted and making each callback retain a reference to the helper via the second arg of base::Bind? https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:373: const storage::FileSystemURL& file_url, file_url is not used? https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:427: const scoped_refptr<storage::FileSystemContext>& filesystem_context, ditto (passing const scoped_refptr<>&). https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:431: : filesystem_context_(std::move(filesystem_context)), ditto (move for const-ref object). https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:449: int task_count_ = 0; ditto (manual reference counting) https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:468: ->sync_file_util()); You could move these lines out of the for-loop. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:516: const scoped_refptr<storage::FileSystemContext>& filesystem_context, Can you remove const-ref because the caller passes ownership of the context using make_scoped_refptr. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:544: if (it == origins.end()) { Using ContainsKey in base/stl_util.h would be simpler. if (ContainsKey(origins, storage_origin))
Updated. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:236: const scoped_refptr<storage::FileSystemContext>& filesystem_context, On 2016/05/23 05:07:51, nhiroki wrote: > Passing "const scoped_refptr<Foo>&" is not encouraged: > > "What about passing or returning a smart pointer by reference" > http://www.chromium.org/developers/smart-pointer-guidelines#TOC-What-about-pa... Done. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:242: : filesystem_context_(std::move(filesystem_context)), On 2016/05/23 05:07:50, nhiroki wrote: > IIUC, std::move for const-ref object does not move but copy. Maybe you should > change the first argument on the ctor to scoped_refptr<FSContext> (or to a > rawptr and then remove std::move() here). Converted to rawptr (and added a comment about ownership). https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:285: int task_count_ = 0; On 2016/05/23 05:07:50, nhiroki wrote: > In general, manual reference counting would be fragile. How about making this > helper class RefCounted and making each callback retain a reference to the > helper via the second arg of base::Bind? This is the pattern used by the other helpers in this file (QuotaManagedDataDeletionHelper and DataDeletionHelper). Wouldn't RefCounted depend on all the called functions not keeping the callback longer than necessary? https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:373: const storage::FileSystemURL& file_url, On 2016/05/23 05:07:50, nhiroki wrote: > file_url is not used? Done. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:427: const scoped_refptr<storage::FileSystemContext>& filesystem_context, On 2016/05/23 05:07:50, nhiroki wrote: > ditto (passing const scoped_refptr<>&). Done. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:431: : filesystem_context_(std::move(filesystem_context)), On 2016/05/23 05:07:51, nhiroki wrote: > ditto (move for const-ref object). Done. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:449: int task_count_ = 0; On 2016/05/23 05:07:50, nhiroki wrote: > ditto (manual reference counting) See previous comment. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:468: ->sync_file_util()); On 2016/05/23 05:07:51, nhiroki wrote: > You could move these lines out of the for-loop. Done. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:516: const scoped_refptr<storage::FileSystemContext>& filesystem_context, On 2016/05/23 05:07:50, nhiroki wrote: > Can you remove const-ref because the caller passes ownership of the context > using make_scoped_refptr. Done. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:544: if (it == origins.end()) { On 2016/05/23 05:07:51, nhiroki wrote: > Using ContainsKey in base/stl_util.h would be simpler. > > if (ContainsKey(origins, storage_origin)) Done.
Looks good overall. I think it's a good time to ask real owners to review. https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/40001/content/browser/storage... content/browser/storage_partition_impl.cc:285: int task_count_ = 0; On 2016/05/23 22:25:36, jrummell wrote: > On 2016/05/23 05:07:50, nhiroki wrote: > > In general, manual reference counting would be fragile. How about making this > > helper class RefCounted and making each callback retain a reference to the > > helper via the second arg of base::Bind? > > This is the pattern used by the other helpers in this file > (QuotaManagedDataDeletionHelper and DataDeletionHelper). Wouldn't RefCounted > depend on all the called functions not keeping the callback longer than > necessary? Manual reference counting would also depend on lifetime of callbacks because DecrementTaskCount() is called when a callback is completed. I think lifetime of the helper is almost the same between manual reference counting and RefCoutned. In manual reference counting, IncrementTaskCount() is called when a callback is created and DecrementTaskCount() is called when the callback is completed. In RefCounted, the internal refcount is incremented when a callback is created and decremented when the callback is destroyed (slightly after it's completed). Anyway, I don't have strong opinions here. The current code looks correct and it's consistent with other helpers. It's ok to land this as is. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:286: base::Time end_; Could you add 'const' to these fields? https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:345: } You might want to skip remaining tasks if |delete_this_data_| is true. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:365: base::Unretained(this), file.name, file_url)); You can just stop passing |file_url| here. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:387: delete_this_data_ = true; |delete_this_data_| would sound a bit confusing to me. It seems like we delete only |file_name| data. However, actual behavior is to delete all this origin data. Could you rename this to something like |delete_this_origin_data_|? https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:435: : filesystem_context_(filesystem_context), std::move(filesystem_context) because the context is non-const scoped_refptr. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:453: base::Time end_; Could you add 'const' to these fields? https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:474: // Determine the available directories for this origin. "available plugin private filesystems" would be clearer. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:482: base::FileEnumerator file_enumerator(path, false, Could you add comments about why we iterate all directories under the origin directories and what those directory names mean? https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... File content/browser/storage_partition_impl_unittest.cc (right): https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl_unittest.cc:51: const char kPluginPrivateRootName[] = "pluginprivate"; ppapi::kPluginPrivateRootName may work. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl_unittest.cc:313: // musty not already exist or the test will fail. s/musty/must
Description was changed from ========== Add ability to clear content licenses Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=tested by calling from the existing UI ========== to ========== Add ability to clear content licenses Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass ==========
jrummell@chromium.org changed reviewers: + bbudge@chromium.org, kinuko@chromium.org
Adding OWNERS. bbudge@ for ppapi/ kinuko@ for content/ https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:286: base::Time end_; On 2016/05/24 01:40:03, nhiroki wrote: > Could you add 'const' to these fields? Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:345: } On 2016/05/24 01:40:03, nhiroki wrote: > You might want to skip remaining tasks if |delete_this_data_| is true. Done. Not sure if it will help right now, as typically there will only be 1 file in the directory, but in the future if there are lots of files it will help. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:365: base::Unretained(this), file.name, file_url)); On 2016/05/24 01:40:03, nhiroki wrote: > You can just stop passing |file_url| here. Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:387: delete_this_data_ = true; On 2016/05/24 01:40:03, nhiroki wrote: > |delete_this_data_| would sound a bit confusing to me. It seems like we delete > only |file_name| data. However, actual behavior is to delete all this origin > data. Could you rename this to something like |delete_this_origin_data_|? Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:435: : filesystem_context_(filesystem_context), On 2016/05/24 01:40:03, nhiroki wrote: > std::move(filesystem_context) because the context is non-const scoped_refptr. Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:453: base::Time end_; On 2016/05/24 01:40:03, nhiroki wrote: > Could you add 'const' to these fields? Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:474: // Determine the available directories for this origin. On 2016/05/24 01:40:03, nhiroki wrote: > "available plugin private filesystems" would be clearer. Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl.cc:482: base::FileEnumerator file_enumerator(path, false, On 2016/05/24 01:40:03, nhiroki wrote: > Could you add comments about why we iterate all directories under the origin > directories and what those directory names mean? Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... File content/browser/storage_partition_impl_unittest.cc (right): https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl_unittest.cc:51: const char kPluginPrivateRootName[] = "pluginprivate"; On 2016/05/24 01:40:03, nhiroki wrote: > ppapi::kPluginPrivateRootName may work. Done. https://codereview.chromium.org/1979733002/diff/60001/content/browser/storage... content/browser/storage_partition_impl_unittest.cc:313: // musty not already exist or the test will fail. On 2016/05/24 01:40:03, nhiroki wrote: > s/musty/must Done.
https://codereview.chromium.org/1979733002/diff/80001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/80001/content/browser/storage... content/browser/storage_partition_impl.cc:1222: BrowserThread::PostTask( Should this use filesystem_context->default_file_task_runner() rather than FILE thread to avoid running in parallel with other filesystem tasks?
Question on the threading model below. https://codereview.chromium.org/1979733002/diff/80001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/80001/content/browser/storage... content/browser/storage_partition_impl.cc:1222: BrowserThread::PostTask( On 2016/05/25 08:57:53, kinuko wrote: > Should this use filesystem_context->default_file_task_runner() rather than FILE > thread to avoid running in parallel with other filesystem tasks? This seems correct (as GetOriginsForTypeOnFileTaskRunner() and others are called). However, it fails later on when trying to open the plugin private file system. It appears that doesn't work on the file_task_runner, which is a SequencedTaskRunner [1]. PluginPrivateFileSystemBackend::OpenPrivateFileSystem() ultimately uses PostTaskAndReplyRelay, which expects to get a ThreadTaskRunnerHandle [2]. So is the correct solution to switch to the FILE thread (or another thread?) to open the file system, or have PluginPrivateFileSystemBackend::OpenPrivateFileSystem() check if it's on the file task runner and don't bother posting a task? [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fi... [2] https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pos...
Updated to use FileTaskRunner where appropriate. https://codereview.chromium.org/1979733002/diff/80001/content/browser/storage... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/80001/content/browser/storage... content/browser/storage_partition_impl.cc:1222: BrowserThread::PostTask( On 2016/05/26 00:35:10, jrummell wrote: > On 2016/05/25 08:57:53, kinuko wrote: > > Should this use filesystem_context->default_file_task_runner() rather than > FILE > > thread to avoid running in parallel with other filesystem tasks? > > This seems correct (as GetOriginsForTypeOnFileTaskRunner() and others are > called). However, it fails later on when trying to open the plugin private file > system. It appears that doesn't work on the file_task_runner, which is a > SequencedTaskRunner [1]. PluginPrivateFileSystemBackend::OpenPrivateFileSystem() > ultimately uses PostTaskAndReplyRelay, which expects to get a > ThreadTaskRunnerHandle [2]. > > So is the correct solution to switch to the FILE thread (or another thread?) to > open the file system, or have > PluginPrivateFileSystemBackend::OpenPrivateFileSystem() check if it's on the > file task runner and don't bother posting a task? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fi... > [2] > https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/pos... I did some more looking at the code, and it looks like most of the operations on the plugin private file system need to be done on a ThreadTaskRunner. So changed to use the FILE thread when accessing the file system, but use FileTaskRunner for getting and deleting based on the origin.
Thanks for looking into further, I wasn't super sure we specifically use thread task runner for plugin private fs while we don't for others. Let me check this again and respond...
Ah ok, I got it. So most of async operations expect that they're issued on IO thread therefore we use thread task runner, while in this patch we do some sync and async operations on FILE thread which confused me.
https://codereview.chromium.org/1979733002/diff/100001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:489: BrowserThread::FILE, FROM_HERE, It's not obvious why we still need to use FILE thread? In most cases we just call async operations from IO thread (either for regular fs operations or for data remover case), and in general we're trying to deprecate FILE thread (so all blocking operations should migrate to sequenced worker pool and then eventually to be more coordinated by global scheduler etc)
Updated so that PluginPrivateDataByOriginDeletionHelper() runs on the IO thread, plus changes to fix Win compile errors. https://codereview.chromium.org/1979733002/diff/100001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/100001/content/browser/storag... content/browser/storage_partition_impl.cc:489: BrowserThread::FILE, FROM_HERE, On 2016/05/26 02:19:45, kinuko wrote: > It's not obvious why we still need to use FILE thread? In most cases we just > call async operations from IO thread (either for regular fs operations or for > data remover case), and in general we're trying to deprecate FILE thread (so all > blocking operations should migrate to sequenced worker pool and then eventually > to be more coordinated by global scheduler etc) Good to know. Switched to IO thread.
lgtm https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... content/browser/storage_partition_impl.cc:423: // All of the operations in this class are done on the FILE thread. FILE thread -> file task runner https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... content/browser/storage_partition_impl.cc:443: void DecrementTaskCount(bool delete_data_for_origin, GURL origin); GURL -> const GURL& https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... content/browser/storage_partition_impl.cc:520: GURL origin) { GURL -> const GURL&
Pepper lgtm
lgtm
ppapi lgtm
Please update the CL title to reflect what we are doing (deleting plugin private files). Mostly nits. I still have the question about the race condition between deleting the files and read/write operations. We should make sure we never end up with corrupted files or partial read/write. https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... File content/browser/storage_partition_impl.cc (right): https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:243: class PluginPrivateDataByOriginDeletionHelper { I am not a fan of having a non-trivial helper class defined in the same file as the implementation, especially when it needs to be if-defined. This makes this file large and harder to read. I see that we have QuotaManagedDataDeletionHelper, but it's an internal class of StoragePartitionImpl so it's a bit different. Can we have a separate header file that only declares ClearPluginPrivateDataOnFileTaskRunner(). Then in the cc file, we'll have PluginPrivateDataByOriginDeletionHelper declared/defined in the anonymous namespace, similar to what we are doing here? https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:269: // with true if any such file is found, false otherwise. nit: Then what is the GURL in the callback? https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:270: void StartOnIOThread(); nit: Should this be CheckFilesOnIOThread() to be consistent with CheckOriginsOnFileTaskRunner() below? https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:285: void DecrementTaskCount(); The whole {In|De}crementTaskCount business is a bit dangerous. If we forget a call somewhere then the object could be leaked. I think we can achieve the same by just bind a unique_ptr of this object in the |callback_|, so that when the callback is reset after fired or dropped, the object will be destructed automatically. Since you are just being consistent with existing code so you don't have to make this change. Just some suggestions for the owners of this file. https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:298: bool delete_this_origin_data_ = false; What does |delete_this_origin_data_| exactly mean? Could you please add a comment? https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:310: base::Unretained(this))); Comment why base::Unretained is safe, e.g. this class owns itself. Maybe we should have a class level comment about the ownership. https://chromiumcodereview.appspot.com/1979733002/diff/140001/content/browser... content/browser/storage_partition_impl.cc:1248: storage_origin, begin, end, decrement_callback)); Please see my previous comment. Since we could be writing the temporary file in multiple stages, what will happen if we wrote the first part, then the files are deleted, and then we try to write the second part? That should end up with a failure. We shouldn't get corrupted files in any cases. If possible, it'd be nice to add tests to cover this case as well. Also, is it possible that we could read a partial file? https://chromiumcodereview.appspot.com/1979733002/diff/140001/ppapi/shared_im... File ppapi/shared_impl/ppapi_constants.h (right): https://chromiumcodereview.appspot.com/1979733002/diff/140001/ppapi/shared_im... ppapi/shared_impl/ppapi_constants.h:26: const char kPluginPrivateRootName[] = "pluginprivate"; Could you please add a comment? It's not super clear that this for the plugin private "file system".
Moved the code to a separate file, and added a test to try writing a file while deleting the data. PTAL. https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... content/browser/storage_partition_impl.cc:423: // All of the operations in this class are done on the FILE thread. On 2016/05/28 00:47:42, nhiroki wrote: > FILE thread -> file task runner Done. https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... content/browser/storage_partition_impl.cc:443: void DecrementTaskCount(bool delete_data_for_origin, GURL origin); On 2016/05/28 00:47:42, nhiroki wrote: > GURL -> const GURL& Done. https://codereview.chromium.org/1979733002/diff/120001/content/browser/storag... content/browser/storage_partition_impl.cc:520: GURL origin) { On 2016/05/28 00:47:42, nhiroki wrote: > GURL -> const GURL& Done. https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... File content/browser/storage_partition_impl.cc (right): https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:243: class PluginPrivateDataByOriginDeletionHelper { On 2016/06/01 05:44:29, xhwang wrote: > I am not a fan of having a non-trivial helper class defined in the same file as > the implementation, especially when it needs to be if-defined. This makes this > file large and harder to read. I see that we have > QuotaManagedDataDeletionHelper, but it's an internal class of > StoragePartitionImpl so it's a bit different. > > Can we have a separate header file that only declares > ClearPluginPrivateDataOnFileTaskRunner(). Then in the cc file, we'll have > PluginPrivateDataByOriginDeletionHelper declared/defined in the anonymous > namespace, similar to what we are doing here? Done. https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:269: // with true if any such file is found, false otherwise. On 2016/06/01 05:44:29, xhwang wrote: > nit: Then what is the GURL in the callback? Done. https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:270: void StartOnIOThread(); On 2016/06/01 05:44:29, xhwang wrote: > nit: Should this be CheckFilesOnIOThread() to be consistent with > CheckOriginsOnFileTaskRunner() below? Done. https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:285: void DecrementTaskCount(); On 2016/06/01 05:44:29, xhwang wrote: > The whole {In|De}crementTaskCount business is a bit dangerous. If we forget a > call somewhere then the object could be leaked. I think we can achieve the same > by just bind a unique_ptr of this object in the |callback_|, so that when the > callback is reset after fired or dropped, the object will be destructed > automatically. > > Since you are just being consistent with existing code so you don't have to make > this change. Just some suggestions for the owners of this file. Acknowledged. However, this may be tricky as the object will fire a lot of async requests (e.g. GetFileInfo() for every file), and callback should only be called when they are all satisfied (or use WeakPtrs, etc.). https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:298: bool delete_this_origin_data_ = false; On 2016/06/01 05:44:29, xhwang wrote: > What does |delete_this_origin_data_| exactly mean? Could you please add a > comment? Done. https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:310: base::Unretained(this))); On 2016/06/01 05:44:29, xhwang wrote: > Comment why base::Unretained is safe, e.g. this class owns itself. Maybe we > should have a class level comment about the ownership. Done. https://codereview.chromium.org/1979733002/diff/140001/content/browser/storag... content/browser/storage_partition_impl.cc:1248: storage_origin, begin, end, decrement_callback)); On 2016/06/01 05:44:29, xhwang wrote: > Please see my previous comment. Since we could be writing the temporary file in > multiple stages, what will happen if we wrote the first part, then the files are > deleted, and then we try to write the second part? That should end up with a > failure. We shouldn't get corrupted files in any cases. If possible, it'd be > nice to add tests to cover this case as well. > > Also, is it possible that we could read a partial file? Depends on the OS. On Linux the file storage remains available even after the directory is deleted. On Windows DeleteOriginDataOnFileTaskRunner() returns an error but appears to delete the directory anyway (and the file remains accessible). Not sure about other OSes, trybots will tell. Attempting to open a file after the directory is deleted produces an error. Added a test for this. https://codereview.chromium.org/1979733002/diff/140001/ppapi/shared_impl/ppap... File ppapi/shared_impl/ppapi_constants.h (right): https://codereview.chromium.org/1979733002/diff/140001/ppapi/shared_impl/ppap... ppapi/shared_impl/ppapi_constants.h:26: const char kPluginPrivateRootName[] = "pluginprivate"; On 2016/06/01 05:44:29, xhwang wrote: > Could you please add a comment? It's not super clear that this for the plugin > private "file system". Done.
Description was changed from ========== Add ability to clear content licenses Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass ========== to ========== Add ability to delete plugin private data Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass ==========
Thanks! This is much better now. I still have a question about potential race condition between write and delete. Just to be on the safer side. https://chromiumcodereview.appspot.com/1979733002/diff/160001/content/browser... File content/browser/plugin_private_storage_helper.h (right): https://chromiumcodereview.appspot.com/1979733002/diff/160001/content/browser... content/browser/plugin_private_storage_helper.h:26: // origins are checked. |callback| is called when the operation is complete. s/checked/cleared https://chromiumcodereview.appspot.com/1979733002/diff/160001/content/browser... File content/browser/storage_partition_impl_unittest.cc (right): https://chromiumcodereview.appspot.com/1979733002/diff/160001/content/browser... content/browser/storage_partition_impl_unittest.cc:1280: run_loop.Run(); This will finish all the deletion steps then quit the RunLoop. But what if we call file.WriteAtCurrentPos() in between? kinuko / nhiroki: What's the general guideline for preventing racing between file read/write and deletion (e.g. from Clear Browsing Data)?
Comments only. kinuko / nhiroki: Question below on how to prevent races between file read/write and deletion. https://codereview.chromium.org/1979733002/diff/160001/content/browser/plugin... File content/browser/plugin_private_storage_helper.h (right): https://codereview.chromium.org/1979733002/diff/160001/content/browser/plugin... content/browser/plugin_private_storage_helper.h:26: // origins are checked. |callback| is called when the operation is complete. On 2016/06/02 23:51:45, xhwang wrote: > s/checked/cleared All origins are checked for file in the required time period, and cleared if there is such a file. Substituting cleared would imply that they are always cleared if |storage_origin| is empty. https://codereview.chromium.org/1979733002/diff/160001/content/browser/storag... File content/browser/storage_partition_impl_unittest.cc (right): https://codereview.chromium.org/1979733002/diff/160001/content/browser/storag... content/browser/storage_partition_impl_unittest.cc:1280: run_loop.Run(); On 2016/06/02 23:51:45, xhwang wrote: > This will finish all the deletion steps then quit the RunLoop. But what if we > call file.WriteAtCurrentPos() in between? > > kinuko / nhiroki: What's the general guideline for preventing racing between > file read/write and deletion (e.g. from Clear Browsing Data)? The test does different things on different OSes (on Windows clear actually fails (silently) as there is a file open, on Linux it doesn't complain). So Open()->Write()->Write() does succeed even if clearing happens between the writes. However, this doesn't test what happens if the clearing pattern (ReadDirectory()->CheckFile()->CheckFile()->...->DeleteDirectory()) is interrupted by other actions affecting the files for that domain. kinuko / nhiroki: Is the expectation that each step is sufficiently independent so that file corruption doesn't happen?
Sorry for my delated reply. https://codereview.chromium.org/1979733002/diff/160001/content/browser/storag... File content/browser/storage_partition_impl_unittest.cc (right): https://codereview.chromium.org/1979733002/diff/160001/content/browser/storag... content/browser/storage_partition_impl_unittest.cc:1280: run_loop.Run(); On 2016/06/08 23:44:08, jrummell wrote: > On 2016/06/02 23:51:45, xhwang wrote: > > This will finish all the deletion steps then quit the RunLoop. But what if we > > call file.WriteAtCurrentPos() in between? > > > > kinuko / nhiroki: What's the general guideline for preventing racing between > > file read/write and deletion (e.g. from Clear Browsing Data)? > > The test does different things on different OSes (on Windows clear actually > fails (silently) as there is a file open, on Linux it doesn't complain). So > Open()->Write()->Write() does succeed even if clearing happens between the > writes. However, this doesn't test what happens if the clearing pattern > (ReadDirectory()->CheckFile()->CheckFile()->...->DeleteDirectory()) is > interrupted by other actions affecting the files for that domain. > > kinuko / nhiroki: Is the expectation that each step is sufficiently independent > so that file corruption doesn't happen? Yes. Each operation is independent and able to finish without interruptions, so I think file corruption should not happen unless each operation leaves a file half-done.
On 2016/06/09 15:59:48, nhiroki wrote: > Sorry for my delated reply. s/delated/delayed/
Currently we asynchronously process files/directories one by one after this point, so it's possible that other file operations could get in while we're checking the file date/time. While-- deletion itself should be done in a single task so corruption wouldn't happen (as Hiroki wrote too)
Thanks for all the inputs. LGTM!
The CQ bit was checked by jrummell@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, bbudge@chromium.org, nhiroki@chromium.org Link to the patchset: https://codereview.chromium.org/1979733002/#ps160001 (title: "seperate file")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1979733002/160001
Message was sent while issue was closed.
Description was changed from ========== Add ability to delete plugin private data Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass ========== to ========== Add ability to delete plugin private data Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== Add ability to delete plugin private data Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass ========== to ========== Add ability to delete plugin private data Currently encrypted media content licenses are stored in plugin private data (as decryption happens in a pepper plugin). Add the ability to erase the data on request. As the data is organized by origin and plugin, all files for a particular origin/plugin combination are removed if any of the files matches the time criteria. The existing plugins only use 1 file currently, but this should avoid issues if the plugins change to use multiple files. The UI is still being debated, so this is simply adding the ability to delete the data once called. BUG=607631 TEST=new content_unittests pass Committed: https://crrev.com/bb33a9731f1bb62cebcab1dbdb795e6cf9ac1140 Cr-Commit-Position: refs/heads/master@{#399322} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/bb33a9731f1bb62cebcab1dbdb795e6cf9ac1140 Cr-Commit-Position: refs/heads/master@{#399322} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
