Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1839283002:
[Cronet] Purge storage directory if version does not match (Closed)
Description[Cronet] Purge storage directory if version does not match
This CL adds a version check to make sure storage directory layout
is up-to-date. This CL moves disk cache to its sub directory, and
local_pref.json to a different sub directory.
BUG=598834
Committed: https://crrev.com/0205413389339f92868e2c62ef0f56f2793bbff0
Cr-Commit-Position: refs/heads/master@{#385260}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address Matt's comments #Patch Set 3 : self review #
Total comments: 10
Patch Set 4 : Address Misha's comments #
Total comments: 10
Patch Set 5 : Address Misha's comments and test suggestion #
Total comments: 4
Patch Set 6 : Address comments #Patch Set 7 : self review #
Dependent Patchsets: Messages
Total messages: 28 (9 generated)
Description was changed from ========== temp BUG= ========== to ========== [Cronet] Purge storage directory if version does not match This CL adds a version check to make sure storage directory layout is up-to-date. This CL moves disk cache to its sub directory, and local_pref.json to a different sub directory. BUG=598834 ==========
xunjieli@chromium.org changed reviewers: + mef@chromium.org, mmenke@chromium.org
PTAL. Thanks!
On 2016/03/30 01:50:19, xunjieli wrote: > PTAL. Thanks! Ping?
Sorry, started to review yesterday and got distracted. Here are the comments I had - hope to get back to this later today, but I may not. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:62: VersionHeader() { std::memset(this, 0, sizeof(*this)); }; Think we should just just direct initialize version. I'd add a kDiskCacheVersionUnknown = 0 or something, and use that, just to be paranoid. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:63: uint32_t version; include <stdint.h> https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:296: if (!(base::DeleteFile(dir, true) && base::CreateDirectory(dir))) { Think it's worth a comment that DeleteFile returns true if the directory does not exist, since we're strongly depending on that unexpected behavior here. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:300: base::FilePath new_version = dir.Append("version"); This is the same as version_filepath. Just using that seems less regression prone. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:504: InitializeCacheDiskCacheDirectory(storage_path); We're doing file IO on the network thread. This doesn't trigger any DCHECKs? We don't actually get anything at all from not doing on the network thread, since we're blocked on waiting for this to complete anyways, just wondering about the checks.
Patchset #2 (id:20001) has been deleted
Thanks a lot for the review! https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:62: VersionHeader() { std::memset(this, 0, sizeof(*this)); }; On 2016/03/31 15:39:07, mmenke wrote: > Think we should just just direct initialize version. I'd add a > kDiskCacheVersionUnknown = 0 or something, and use that, just to be paranoid. Done. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:63: uint32_t version; On 2016/03/31 15:39:07, mmenke wrote: > include <stdint.h> Done. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:296: if (!(base::DeleteFile(dir, true) && base::CreateDirectory(dir))) { On 2016/03/31 15:39:07, mmenke wrote: > Think it's worth a comment that DeleteFile returns true if the directory does > not exist, since we're strongly depending on that unexpected behavior here. Done. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:300: base::FilePath new_version = dir.Append("version"); On 2016/03/31 15:39:07, mmenke wrote: > This is the same as version_filepath. Just using that seems less regression > prone. Done. https://codereview.chromium.org/1839283002/diff/1/components/cronet/android/c... components/cronet/android/cronet_url_request_context_adapter.cc:504: InitializeCacheDiskCacheDirectory(storage_path); On 2016/03/31 15:39:07, mmenke wrote: > We're doing file IO on the network thread. This doesn't trigger any DCHECKs? > We don't actually get anything at all from not doing on the network thread, > since we're blocked on waiting for this to complete anyways, just wondering > about the checks. No checks are triggered when I run the tests locally with a debug build. I looked through the code. It seems that File I/O is permitted on thread started with base::MessageLoop::TYPE_IO option. The network thread has this option. in base/files/file_posix.h: ThreadRestrictions::AssertIOAllowed https://code.google.com/p/chromium/codesearch#chromium/src/base/files/file_po...
cronet_url_request_context_adapter.cc LGTM. I haven't looked at DiskCacheTest.java. Should I?
On 2016/03/31 19:40:57, mmenke wrote: > cronet_url_request_context_adapter.cc LGTM. I haven't looked at > DiskCacheTest.java. Should I? The tests are just sanity checks. I think it'd be fine to defer to Misha :)
Misha: a friendly ping.. Thanks!
https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:60: const int32_t kDiskCacheVersion = 1; how would we know when to change it? https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:297: if (!(base::DeleteFile(dir, true) && base::CreateDirectory(dir))) { I think it makes sense to have disk cache directory be a subdirectory of |dir|, so we could delete just that in the future without deleting prefs and other stuff. Ah, I see that it set in config processor. So, maybe rename here CacheDiskCacheDirectory -> StorageDirectory? https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:503: InitializeCacheDiskCacheDirectory(storage_path); so, what do we do if this fails? https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:505: storage_path.Append(FILE_PATH_LITERAL("prefs")) can we have a constant for "prefs" directory name? https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:506: .Append(FILE_PATH_LITERAL("local_prefs.json")); can we have a constant for "prefs" file name?
Thanks a lot for the review! PTAL. https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:60: const int32_t kDiskCacheVersion = 1; On 2016/04/01 18:41:46, mef wrote: > how would we know when to change it? If we moved things around, for instance, right now we have: storage_path/ disk_cache/ prefs/ version If in the future, we decide that disk_cache should no longer be in a dir named "disk_cache", and instead, it should be "awesome_disk_cache", then we will update the version number. https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:297: if (!(base::DeleteFile(dir, true) && base::CreateDirectory(dir))) { On 2016/04/01 18:41:46, mef wrote: > I think it makes sense to have disk cache directory be a subdirectory of |dir|, > so we could delete just that in the future without deleting prefs and other > stuff. > > Ah, I see that it set in config processor. So, maybe rename here > CacheDiskCacheDirectory -> StorageDirectory? Done. https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:503: InitializeCacheDiskCacheDirectory(storage_path); On 2016/04/01 18:41:46, mef wrote: > so, what do we do if this fails? I believe we will just fail silently. Nothing will crash. https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:505: storage_path.Append(FILE_PATH_LITERAL("prefs")) On 2016/04/01 18:41:46, mef wrote: > can we have a constant for "prefs" directory name? Done. https://codereview.chromium.org/1839283002/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:506: .Append(FILE_PATH_LITERAL("local_prefs.json")); On 2016/04/01 18:41:46, mef wrote: > can we have a constant for "prefs" file name? Done.
https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:515: base::FilePath storage_path(config->storage_path); So, what will happen if 2 engines are racing to initialize the same storage path? https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java (right): https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:19: public class DiskCacheTest extends CronetTestBase { Maybe rename DiskCacheTest -> DiskStorageTest? I wonder whether we should add a test for when storage path is set to readonly directory (It checks that directory exists, so at least name is valid). I wouldn't be surprised if it will break, but I wouldn't be surprised if it breaks without this change as well. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:44: versionOut.write(new byte[] {2, 0, 0, 0}, 0, 4); this is actually newer version, right? So, if we increment version at some point, then this will break. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:103: versionOut.write(new byte[] {1, 0, 0, 0}, 0, 4); This looks very brittle as it depends on actual format and content of version file. Can we do this: 1. initialize the storage, 2. shutdown the engine, 3. create the dummy file, 4. create another engine, 5. verify that it doesn't delete the dummy file?
Thanks for the review! PTAL. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:515: base::FilePath storage_path(config->storage_path); On 2016/04/01 21:16:03, mef wrote: > So, what will happen if 2 engines are racing to initialize the same storage > path? That will be bad. That will happen without this change. If there are 2 CronetEngines with the same path, two disk cache backends will be writing to same path on two different threads! I think we should document that if there are multiple CronetEngines, don't use the same storage path. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java (right): https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:19: public class DiskCacheTest extends CronetTestBase { On 2016/04/01 21:16:03, mef wrote: > Maybe rename DiskCacheTest -> DiskStorageTest? > > I wonder whether we should add a test for when storage path is set to readonly > directory (It checks that directory exists, so at least name is valid). > > I wouldn't be surprised if it will break, but I wouldn't be surprised if it > breaks without this change as well. Done. The directory is initialized as normal. The test instrumentation has the same UID as the test app, so I guess setting the directory to readOnly doesn't do much. The test has no permission to set a directory in external storage (sdcard) to readOnly, so I didn't use that. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:44: versionOut.write(new byte[] {2, 0, 0, 0}, 0, 4); On 2016/04/01 21:16:03, mef wrote: > this is actually newer version, right? So, if we increment version at some > point, then this will break. Done. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:103: versionOut.write(new byte[] {1, 0, 0, 0}, 0, 4); On 2016/04/01 21:16:03, mef wrote: > This looks very brittle as it depends on actual format and content of version > file. > > Can we do this: > > 1. initialize the storage, > 2. shutdown the engine, > 3. create the dummy file, > 4. create another engine, > 5. verify that it doesn't delete the dummy file? Done.
Misha: a friendly ping. Thanks!
On 2016/04/05 14:34:55, xunjieli wrote: > Misha: a friendly ping. Thanks! Is it possible to get this before the dev channel is cut? so it can be just in time for our embedder's next experiments?
On 2016/04/05 17:51:28, xunjieli wrote: > On 2016/04/05 14:34:55, xunjieli wrote: > > Misha: a friendly ping. Thanks! > > Is it possible to get this before the dev channel is cut? so it can be just in > time for our embedder's next experiments? Yep, looking now.
lgtm https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:515: base::FilePath storage_path(config->storage_path); On 2016/04/04 15:01:24, xunjieli wrote: > On 2016/04/01 21:16:03, mef wrote: > > So, what will happen if 2 engines are racing to initialize the same storage > > path? > > That will be bad. That will happen without this change. If there are 2 > CronetEngines with the same path, two disk cache backends will be writing to > same path on two different threads! I think we should document that if there are > multiple CronetEngines, don't use the same storage path. Acknowledged. https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java (right): https://codereview.chromium.org/1839283002/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/DiskCacheTest.java:19: public class DiskCacheTest extends CronetTestBase { On 2016/04/04 15:01:24, xunjieli wrote: > On 2016/04/01 21:16:03, mef wrote: > > Maybe rename DiskCacheTest -> DiskStorageTest? > > > > I wonder whether we should add a test for when storage path is set to readonly > > directory (It checks that directory exists, so at least name is valid). > > > > I wouldn't be surprised if it will break, but I wouldn't be surprised if it > > breaks without this change as well. > > Done. The directory is initialized as normal. The test instrumentation has the > same UID as the test app, so I guess setting the directory to readOnly doesn't > do much. The test has no permission to set a directory in external storage > (sdcard) to readOnly, so I didn't use that. Acknowledged. https://codereview.chromium.org/1839283002/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/DiskStorageTest.java (right): https://codereview.chromium.org/1839283002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/DiskStorageTest.java:138: // directory. nit: would this fit onto previous line? https://codereview.chromium.org/1839283002/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1839283002/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.cc:283: .Append(FILE_PATH_LITERAL(kDiskCacheDirectoryName)); Do we have and/or need a test that 'disk_cache' directory gets created?
Patchset #6 (id:120001) has been deleted
Thanks for the review! https://codereview.chromium.org/1839283002/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/DiskStorageTest.java (right): https://codereview.chromium.org/1839283002/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/DiskStorageTest.java:138: // directory. On 2016/04/05 18:00:50, mef wrote: > nit: would this fit onto previous line? Done. https://codereview.chromium.org/1839283002/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1839283002/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.cc:283: .Append(FILE_PATH_LITERAL(kDiskCacheDirectoryName)); On 2016/04/05 18:00:50, mef wrote: > Do we have and/or need a test that 'disk_cache' directory gets created? Done. Added the assertions in the original tests.
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1839283002/#ps150001 (title: "self review")
The CQ bit was checked by xunjieli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1839283002/150001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1839283002/150001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Purge storage directory if version does not match This CL adds a version check to make sure storage directory layout is up-to-date. This CL moves disk cache to its sub directory, and local_pref.json to a different sub directory. BUG=598834 ========== to ========== [Cronet] Purge storage directory if version does not match This CL adds a version check to make sure storage directory layout is up-to-date. This CL moves disk cache to its sub directory, and local_pref.json to a different sub directory. BUG=598834 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:150001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Purge storage directory if version does not match This CL adds a version check to make sure storage directory layout is up-to-date. This CL moves disk cache to its sub directory, and local_pref.json to a different sub directory. BUG=598834 ========== to ========== [Cronet] Purge storage directory if version does not match This CL adds a version check to make sure storage directory layout is up-to-date. This CL moves disk cache to its sub directory, and local_pref.json to a different sub directory. BUG=598834 Committed: https://crrev.com/0205413389339f92868e2c62ef0f56f2793bbff0 Cr-Commit-Position: refs/heads/master@{#385260} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0205413389339f92868e2c62ef0f56f2793bbff0 Cr-Commit-Position: refs/heads/master@{#385260} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
