Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2901083002:
[CacheStorage] Pad and bin opaque resource sizes. (Closed)
Description[CacheStorage] Pad and bin opaque resource sizes.
BUG=617963
Review-Url: https://codereview.chromium.org/2901083002
Cr-Commit-Position: refs/heads/master@{#494384}
Committed: https://chromium.googlesource.com/chromium/src/+/8421200aab24ddf59dfea7256ccffed8f7a61e0b
Patch Set 1 #Patch Set 2 : Rebased and resolved conflicts. #
Total comments: 9
Patch Set 3 : Storing padding key in cache. #
Total comments: 16
Patch Set 4 : y #
Total comments: 16
Patch Set 5 : Addressed comments for Patch Set 4. #
Total comments: 2
Patch Set 6 : Creating single padding key per session. #
Total comments: 17
Patch Set 7 : 1) key as string, flags back to enum, & other patch set 6 fixes. #Patch Set 8 : Padding side data and writing padding alg. version to index. #
Total comments: 44
Patch Set 9 : Patch Set 8 changes. #
Total comments: 33
Patch Set 10 : Patch set 9 changes #
Total comments: 2
Patch Set 11 : s/also also/also/ and EXPECT_GT ↔ EXPECT_LT #Messages
Total messages: 59 (29 generated)
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jkarlin@ Here's the most basic implementation of response padding. It does not support POST data (not sure if it needs to), nor vary headers. The distribution has test values in ResponsePaddingDistribution. It's light on testing, so suggestions are welcome. The cache querying (CacheStorageCache::QueryCacheDidReadMetadata) was modified to support querying the responses without bodies (to entry padding). If desired this CL could be split into two and that change landed first to simplify this CL.
cmumford@chromium.org changed reviewers: + jkarlin@chromium.org
A couple initial comments. https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1085: // |rv| is ignored as doom entry can fail if the entry doesn't exist. Why can't we just use doom as we did before and check the return value to see if we need to remove the padding? All we need is the URL to determine the padding right? https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1586: CalculateCacheSizePadding(base::Bind(&CacheStorageCache::InitGotCacheSize, QueryCache calls that require enumerations are *incredibly* expensive and we need to reduce them, not add more. Calling it on cache open is particularly painful because caches open and close frequently. We need another solution. Just random thoughts here: 1. Store the padding size with the cache and in the origin as we did for the cache size. This also means we'd have to store the private key, which smells of trouble (not the fact that it can be read from disk but that different caches/origins would have different keys) 2. Store the padding size without the key. Use a new key on startup and if you delete stuff you run the risk of going negative. That seems not so bad? It gives the attacker another sample each time the browser restarts, but that too seems reasonable.
https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1085: // |rv| is ignored as doom entry can fail if the entry doesn't exist. On 2017/05/26 13:21:25, jkarlin wrote: > Why can't we just use doom as we did before and check the return value to see if > we need to remove the padding? All we need is the URL to determine the padding > right? Currently I use the last URL in the URL list. That was an open issue that I wanted to discuss with you (i.e. should we hash them all, or just the last one?). We need to know if it's an opaque request (see ShouldPadResourceSize). https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1586: CalculateCacheSizePadding(base::Bind(&CacheStorageCache::InitGotCacheSize, Understood. I understand your concerns about #1, but I'm still inclined to go with it since I'm worried about the negative padding issues. I'm also worried about accumulated error, so I'm thinking about doing an open and cheap size calculation (as it does today), but if the sizes don't match (where ServiceWorkerCache.IndexSizeDifference is logged) I'd trigger a recalculation of the cache padding to set that value to be correct. Sound like a plan?
https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1586: CalculateCacheSizePadding(base::Bind(&CacheStorageCache::InitGotCacheSize, On 2017/05/30 20:56:13, cmumford wrote: > Understood. I understand your concerns about #1, but I'm still inclined to go > with it since I'm worried about the negative padding issues. I'm also worried > about accumulated error, so I'm thinking about doing an open and cheap size > calculation (as it does today), but if the sizes don't match (where > ServiceWorkerCache.IndexSizeDifference is logged) I'd trigger a recalculation of > the cache padding to set that value to be correct. > > Sound like a plan? So, the plan is for each browsing session to have a secret key, and any new cache created during that session gets that key (and the key is stored with it in CacheStorage's index file). If corruption is detected, recalculate the slow way. Is that right? If so, sgtm.
https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1085: // |rv| is ignored as doom entry can fail if the entry doesn't exist. On 2017/05/30 20:56:13, cmumford wrote: > On 2017/05/26 13:21:25, jkarlin wrote: > > Why can't we just use doom as we did before and check the return value to see > if > > we need to remove the padding? All we need is the URL to determine the padding > > right? > > Currently I use the last URL in the URL list. That was an open issue that I > wanted to discuss with you (i.e. should we hash them all, or just the last > one?). I'm not following what you're saying here. >We need to know if it's an opaque request (see ShouldPadResourceSize). Ah, good point about not knowing if the cached resource is opaque.
https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1085: // |rv| is ignored as doom entry can fail if the entry doesn't exist. In answer to your initial question; DoomEntry returns net::ERR_SUCCESS even when the key is not present in the cache, but we only need to decrement the padding if actually present in the cache. I don't see a way around querying the cache entries, just to get the response metadata, to know if the padding needs to be decremented. https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1586: CalculateCacheSizePadding(base::Bind(&CacheStorageCache::InitGotCacheSize, On 2017/05/31 11:44:48, jkarlin wrote: > On 2017/05/30 20:56:13, cmumford wrote: > > Understood. I understand your concerns about #1, but I'm still inclined to go > > with it since I'm worried about the negative padding issues. I'm also worried > > about accumulated error, so I'm thinking about doing an open and cheap size > > calculation (as it does today), but if the sizes don't match (where > > ServiceWorkerCache.IndexSizeDifference is logged) I'd trigger a recalculation > of > > the cache padding to set that value to be correct. > > > > Sound like a plan? > > So, the plan is for each browsing session to have a secret key, and any new > cache created during that session gets that key (and the key is stored with it > in CacheStorage's index file). If corruption is detected, recalculate the slow > way. Is that right? If so, sgtm. Yes.
https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (left): https://codereview.chromium.org/2901083002/diff/20001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:1085: // |rv| is ignored as doom entry can fail if the entry doesn't exist. On 2017/06/08 17:54:04, cmumford wrote: > In answer to your initial question; DoomEntry returns net::ERR_SUCCESS even when > the key is not present in the cache, but we only need to decrement the padding > if actually present in the cache. I don't see a way around querying the cache > entries, just to get the response metadata, to know if the padding needs to be > decremented. Got it, thanks!
Heading out but here is what I have so far. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:106: int64_t cache_paddinge, cache_padding https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:292: CacheStorage::kSizeUnknown, Can't we use 0 for initial padding? https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1050: DCHECK_NE(metadata, nullptr); DCHECK(metadata); https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:307: const size_t kMinPadding = 20 * 1024; I don't believe having a min padding > 0 has any security benefit, but does inflate the cache size. So let's use 0 here? That makes the test a bit trickier because sometimes the padding will be 0. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:746: bool PadResources() const override { It's unclear to me what PadResources is being used for? I don't see it affecting any of the test behavior. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1627: EXPECT_EQ(CacheStorageCache::CalculateResponsePadding(non_opaque_response, EXPECT_EQ(0, ...) https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1642: CacheStorageCache::CalculateResponsePadding(opaque_response, padding_key); I'd rather that we not call this private function and just use the test below to verify that we more than doubled in size using the public function. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.cc:83: DCHECK_NE(cache_name, doomed_cache_metadata_.name); DCHECK(!has_doomed_cache || cache_name == doomed_cache_metadata_.name) << cache_name << " != " << doomed_cache_metadata_.name;
https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:265: blink::kWebServiceWorkerResponseTypeOpaque || Let's change this to a switch on response.response_type so we can be sure that if anyone updates the enum that they'll have to update this too. e.g., switch(response.response_type) { case blink::kWebServiceWorkerResponseTypeBasic: case blink::kWebServiceWorkerResponseTypeCORS: case blink::kWebServiceWorkerResponseTypeDefault: case blink::kWebServiceWorkerResponseTypeError: return false; case blink::kWebServiceWorkerResponseTypeOpaque: case blink::kWebServiceWorkerResponseTypeOpaqueRedirect: return !response.url_list.empty(); } NOTREACHED(); return blink::kWebServiceWorkerResponseTypeOpaque; https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:277: size_t h = std::hash<std::string>{}(response.url_list.back().spec()); std::hash isn't meant for security. In fact, I don't think this function is necessary, we want to pass the url to the HMAC.sign() (the document was wrong). https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:281: class PaddingHMACKey : public std::string { Let's use crypto::SymmetricKey to generate and serialize/deserialize the key instead of making a new class. The HMAC algorithm takes a SymmetricKey as an argument. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:290: std::string CalculatePaddingHMAC(const std::string& value, Since we never use the full length of the result, how about we just return a uint64_t here? https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:293: std::string result(hmac.DigestLength(), '\0'); Rather than a string, how about a vector<uint8_t>? How about using a std::ByteVector, as such: vector<uint8_t> result(hmac.DigestLength()); ... if (!hmac.Sign(data, &result[0], result.size())) https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:299: return result; Rather than return the result, we could return the first 8 bytes: return *(reinterpret_cast<uint64_t*>(&result[0])) https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:304: int64_t ResponsePaddingDistribution(const std::string& response_hmac) { If response_hmac turns into a uint64_t, then this function is just: return response_hmac % kPaddingRange; https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:903: CalculateResponsePaddingHash(response), padding_key)); Ah, the document was wrong. You don't want to hash the response url before passing it to the HMAC algorithm. You want to pass the whole URL. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:115: virtual bool PadResources() const { return false; } This isn't used anywhere?
https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:106: int64_t cache_paddinge, On 2017/06/08 18:58:48, jkarlin wrote: > cache_padding Done. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:292: CacheStorage::kSizeUnknown, On 2017/06/08 18:58:48, jkarlin wrote: > Can't we use 0 for initial padding? In CacheStorageCache::InitGotCacheSize we will only traverse all entry metadata if the padding is unknown (or known to be wrong), so we need to be able to know the difference between a cache that has no padded resources, and one that is not yet padded. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:1050: DCHECK_NE(metadata, nullptr); On 2017/06/08 18:58:48, jkarlin wrote: > DCHECK(metadata); Done. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:307: const size_t kMinPadding = 20 * 1024; On 2017/06/08 18:58:48, jkarlin wrote: > I don't believe having a min padding > 0 has any security benefit, but does > inflate the cache size. So let's use 0 here? > > That makes the test a bit trickier because sometimes the padding will be 0. Done. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:746: bool PadResources() const override { It was, but unnecessarily, so I removed it. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1627: EXPECT_EQ(CacheStorageCache::CalculateResponsePadding(non_opaque_response, On 2017/06/08 18:58:48, jkarlin wrote: > EXPECT_EQ(0, ...) Done. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache_unittest.cc:1642: CacheStorageCache::CalculateResponsePadding(opaque_response, padding_key); Will do. Technically padding can be zero, so the test is relying on the hard-coded padding_key + URL returning > 0. I'll add a comment to make this clear. https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_index.cc (right): https://codereview.chromium.org/2901083002/diff/40001/content/browser/cache_s... content/browser/cache_storage/cache_storage_index.cc:83: DCHECK_NE(cache_name, doomed_cache_metadata_.name); On 2017/06/08 18:58:48, jkarlin wrote: > DCHECK(!has_doomed_cache || cache_name == doomed_cache_metadata_.name) << > cache_name << " != " << doomed_cache_metadata_.name; Done. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:265: blink::kWebServiceWorkerResponseTypeOpaque || On 2017/06/09 15:26:20, jkarlin wrote: > Let's change this to a switch on response.response_type so we can be sure that > if anyone updates the enum that they'll have to update this too. > > e.g., > > switch(response.response_type) { > case blink::kWebServiceWorkerResponseTypeBasic: > case blink::kWebServiceWorkerResponseTypeCORS: > case blink::kWebServiceWorkerResponseTypeDefault: > case blink::kWebServiceWorkerResponseTypeError: > return false; > case blink::kWebServiceWorkerResponseTypeOpaque: > case blink::kWebServiceWorkerResponseTypeOpaqueRedirect: > return !response.url_list.empty(); > } > NOTREACHED(); > return blink::kWebServiceWorkerResponseTypeOpaque; > Done. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:290: std::string CalculatePaddingHMAC(const std::string& value, On 2017/06/09 15:26:20, jkarlin wrote: > Since we never use the full length of the result, how about we just return a > uint64_t here? Done. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:293: std::string result(hmac.DigestLength(), '\0'); On 2017/06/09 15:26:20, jkarlin wrote: > Rather than a string, how about a vector<uint8_t>? > > > How about using a std::ByteVector, as such: > > vector<uint8_t> result(hmac.DigestLength()); > ... > if (!hmac.Sign(data, &result[0], result.size())) Done. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:299: return result; On 2017/06/09 15:26:20, jkarlin wrote: > Rather than return the result, we could return the first 8 bytes: > > return *(reinterpret_cast<uint64_t*>(&result[0])) Done. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:304: int64_t ResponsePaddingDistribution(const std::string& response_hmac) { Done - I collapsed these functions into CacheStorageCache::CalculateResponsePadding as I felt it was cleaner. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_cache.cc:903: CalculateResponsePaddingHash(response), padding_key)); On 2017/06/09 15:26:20, jkarlin wrote: > Ah, the document was wrong. You don't want to hash the response url before > passing it to the HMAC algorithm. You want to pass the whole URL. Done. https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/60001/content/browser/cache_s... content/browser/cache_storage/cache_storage_manager_unittest.cc:115: virtual bool PadResources() const { return false; } On 2017/06/09 15:26:20, jkarlin wrote: > This isn't used anywhere? Not meaningfully, so I reverted it.
The CQ bit was checked by cmumford@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2901083002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2901083002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:65: std::unique_ptr<SymmetricKey> GeneratePaddingKey() { We want a single key per browser sesssion, not a key per-cache. Having one per-cache means the attacker can get extra samples for a url by creating new caches.
https://codereview.chromium.org/2901083002/diff/80001/content/browser/cache_s... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2901083002/diff/80001/content/browser/cache_s... content/browser/cache_storage/cache_storage.cc:65: std::unique_ptr<SymmetricKey> GeneratePaddingKey() { Doh - of course <sigh>
https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage.proto (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.proto:16: optional bytes padding_key = 4; This should be a string now right? https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1122: DeleteImpl(std::move(delete_request), query_options, It hurts to call DeleteImpl here, as it's going to seriously slow down put operations, but so be it. Can you add a TODO to see if we can find a faster way? For instance, something that morlovich@ is working on is adding a few bits to the backend's index that we can put whatever we want in and access synchronously. We might use a bit to signal that the resource is opaque. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1138: cache_padding_ -= CalculateResponsePadding(*put_context->response, Hmm, shouldn't delete take care of removing the cache_padding in case of error? https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1244: cache_padding_ += CalculateResponsePadding(*put_context->response, Ah, there is another place that we need to bump the cache_padding_, and that's when side data (e.g., compiled javascript) is written to the cache such as in WriteSideDataDidWrite. I need to think more about this, but perhaps for metadata we could add HMAC(private_key, URL + "METADATA") so that it has a different (but also consistent for the url) pad. WDYT? We could not count the side data against the cache's quota, but implementing that would be tricky since we rely on the backend's CalculateSizeOfAllEntries to get the size quickly and that includes the side data size. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1315: QUERY_CACHE_REQUESTS | QUERY_CACHE_RESPONSES_WITH_BODIES, You want QUERY_CACHE_RESPONSES_NO_BODIES here I think as you just need the URL and response type right? I don't think you need QUERY_CACHE_REQUESTS either. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1600: if (cache_padding_ == CacheStorage::kSizeUnknown || cache_padding_ < 0) { Can you add a comment somewhere around here that we're assuming that if cache_size_ matches cache_size then we also assume that the padding is correct since it's stored at the same time. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:192: const uint32_t QUERY_CACHE_REQUESTS = 0x1; I'd prefer an enum (but not an enum class since that's not converted implicitly to an int) since you can exhaustively switch on that. Also, could you add a new type for the flags that you pass around? E.g., using QueryTypes = uint32_t; https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.cc (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.cc:124: void CacheStorageIndex::UpdateStoragePadding() { Perhaps rename to CalculateStoragePadding?
https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage.proto (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage.proto:16: optional bytes padding_key = 4; On 2017/06/15 16:08:35, jkarlin_OOO wrote: > This should be a string now right? Done. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1122: DeleteImpl(std::move(delete_request), query_options, On 2017/06/15 16:08:35, jkarlin_OOO wrote: > It hurts to call DeleteImpl here, as it's going to seriously slow down put > operations, but so be it. Can you add a TODO to see if we can find a faster way? > > For instance, something that morlovich@ is working on is adding a few bits to > the backend's index that we can put whatever we want in and access > synchronously. We might use a bit to signal that the resource is opaque. Done. Unfortunately morlovich's abandoned his index change. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1138: cache_padding_ -= CalculateResponsePadding(*put_context->response, This is the read of the old cache entry that must currently be done to know if it's there, and even opaque. I don't believe it's possible to get an error and also get response. That being said, I moved this outside of the check as it doesn't impact performance and is less confusing. Also, I should have been using the query_result's response instead of the one from put_context. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1244: cache_padding_ += CalculateResponsePadding(*put_context->response, A few thoughts. 1. If we did ignore the side data, as we do now, what are the security implications of that? 2. If we pad the side data then we'd also need to decrement it in PutDidDeleteEntry. I don't believe that data comes back in QueryCacheResult, so it would be another call to DiskCache::GetDataSize() in order to determine if we've cached compiled JavaScript. Or am I missing something obvious in #2? https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1315: QUERY_CACHE_REQUESTS | QUERY_CACHE_RESPONSES_WITH_BODIES, Yes, that was unnecessary - thx. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1600: if (cache_padding_ == CacheStorage::kSizeUnknown || cache_padding_ < 0) { The comment above on line 1589 is almost the inverse of your requested comment. I'll invert the comment, but leave it at that location. Let me know if you want it moved. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:192: const uint32_t QUERY_CACHE_REQUESTS = 0x1; On 2017/06/15 16:08:35, jkarlin_OOO wrote: > I'd prefer an enum (but not an enum class since that's not converted implicitly > to an int) since you can exhaustively switch on that. > > Also, could you add a new type for the flags that you pass around? > > E.g., using QueryTypes = uint32_t; Done. https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.cc (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.cc:124: void CacheStorageIndex::UpdateStoragePadding() { On 2017/06/15 16:08:35, jkarlin_OOO wrote: > Perhaps rename to CalculateStoragePadding? Done.
https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/100001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1244: cache_padding_ += CalculateResponsePadding(*put_context->response, On 2017/06/20 18:22:04, cmumford wrote: > A few thoughts. > > 1. If we did ignore the side data, as we do now, what are the security > implications of that? We'd leak the size of whatever side-data is stored. I'm aware of compiled javascript being stored, I'm not sure if there are other consumers. So if the attacker knows the size of the compiled javascript for a resource, then they know what the original script is. That doesn't seem terrible, but it's enough to make me uncomfortable. Especially since we don't know what other consumers may come in the future. > > 2. If we pad the side data then we'd also need to decrement it in > PutDidDeleteEntry. I don't believe that data comes back in QueryCacheResult, so > it would be another call to DiskCache::GetDataSize() in order to determine if > we've cached compiled JavaScript. Yes, we'd need to call GetDataSize. That's synchronous and fast, no concerns there.
Also writing the padding algorithm version, so that if we ever decide to change it, we can know when to invalidate and recalc.
Looking good. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:290: // Remove this line https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:296: // Remove this line https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:301: const T* response, Hmm, response is only passed in to be DCHECK'd? That doesn't seem right. Perhaps run the DCHECK at the calling site? https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:310: std::string data = side_data_size ? response_url + "METADATA" : response_url; URLs can be long, let's avoid this string copy in the cases where we can. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:316: uint64_t val = *(reinterpret_cast<uint64_t*>(&digest[0])); Let's DCHECK that digest is at least uint64_t in size for peace of mind https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:389: QueryTypes query_types = 0x0; 0? https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:916: match->blob_handle = std::move(blob_data_handle); Seems like we could combine the above lines: match->blob_handle = PopulateResponseBody(std::move(entry), match->response.get()); https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1151: response.get(), cache_padding_key_.get(), side_data_size_before_write); Let's add a test where we write side data twice (each with a different size) and verify that the padded cache size is the same (delta the difference in side data size). https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1239: cache_padding_ -= This still confuses me. DeleteImpl() already alters cache_padding_ in DeleteDidQueryCache for the deleted results, why are we doing it again here? And if it's not needed, we can remove the new code where Delete returns entries. Let's add a test that shows putting the same entry in twice results in the same cache size after eat put operation. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1420: Please update the README.md for this directory with a description of padding and how it works into the architecture https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1424: // Enumerating entries is only done during cache initialization. only done during cache initialization and only if necessary https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1482: int64_t size_delta = PaddedCacheSize() - last_reported_size_; last_reported_size_ initializes to 0 and doesn't get updated before this point. Which means after loading a preexisting cache, the first time you change something you're going to have the wrong size_delta and the QuotaManager will have the wrong size. Please fix and add relevant test. I believe we have some tests that measure notifystoragemodified calls. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1517: void CacheStorageCache::DeleteDidDelete( This function isn't in the same order as the header and needs to be moved down below DeleteDidQueryCache. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1517: void CacheStorageCache::DeleteDidDelete( Can you add a comment specifying that the purpose of this function is to ignore the query results? https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1793: cache_padding_ == CacheStorage::kSizeUnknown) { Can we DCHECK that the cache hasn't initialized yet if we're in this state? https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1628: // Padding can be zero bytes, so if the opaque URL + padding key changes in Suggest: // This test is fragile. Right now it deterministically adds non-zero padding. But if the url, padding key, or padding algorithm change it might become zero. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1641: EXPECT_NE(opaque_cache_size_with_side_data, opaque_cache_size); How is this testing that the padding changed? It's just showing that the size grew after writing side data, which you would expect with or without padding. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1650: EXPECT_EQ(size_after_delete, opaque_cache_size); Also delete the opaque data and verify that the size returns to non_padded_cache_size. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1652: We need some cache_storage_manager_unittest.cc tests that open/close the caches, and change the CacheStorageCache static key to ensure that the key is properly stored on disk. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:59: const CacheMetadata* FindMetadata(const std::string& cache_name) const; nit: I slightly prefer GetMetadata to FindMetadata, especially since there is a Set* function as well as a "GetPaddedStorageSize". https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:96: int64_t GetCacheSize(const std::string& cache_name) const; GetCacheSizeForTesting https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:100: int64_t GetCachePadding(const std::string& cache_name) const; GetCachePaddingForTesting
https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:290: // On 2017/07/21 21:18:08, jkarlin wrote: > Remove this line Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:296: // On 2017/07/21 21:18:09, jkarlin wrote: > Remove this line Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:301: const T* response, On 2017/07/21 21:18:08, jkarlin wrote: > Hmm, response is only passed in to be DCHECK'd? That doesn't seem right. Perhaps > run the DCHECK at the calling site? Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:310: std::string data = side_data_size ? response_url + "METADATA" : response_url; On 2017/07/21 21:18:08, jkarlin wrote: > URLs can be long, let's avoid this string copy in the cases where we can. Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:316: uint64_t val = *(reinterpret_cast<uint64_t*>(&digest[0])); On 2017/07/21 21:18:08, jkarlin wrote: > Let's DCHECK that digest is at least uint64_t in size for peace of mind Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:389: QueryTypes query_types = 0x0; On 2017/07/21 21:18:08, jkarlin wrote: > 0? Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:916: match->blob_handle = std::move(blob_data_handle); On 2017/07/21 21:18:08, jkarlin wrote: > Seems like we could combine the above lines: > > match->blob_handle = PopulateResponseBody(std::move(entry), > match->response.get()); Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1151: response.get(), cache_padding_key_.get(), side_data_size_before_write); Done: TestDifferentOpaqueSideDataSizes. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1239: cache_padding_ -= Yes, you were correct. I added TestDoubleOpaquePut and deleted this code. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1420: On 2017/07/21 21:18:08, jkarlin wrote: > Please update the README.md for this directory with a description of padding and > how it works into the architecture Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1424: // Enumerating entries is only done during cache initialization. On 2017/07/21 21:18:08, jkarlin wrote: > only done during cache initialization and only if necessary Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1482: int64_t size_delta = PaddedCacheSize() - last_reported_size_; I added CacheStorageManagerTest.CacheSizePaddedAfterReopen which was a needed test - thx. However, I'm not sure I'm seeing the problem you describe. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1517: void CacheStorageCache::DeleteDidDelete( On 2017/07/21 21:18:09, jkarlin wrote: > Can you add a comment specifying that the purpose of this function is to ignore > the query results? Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1517: void CacheStorageCache::DeleteDidDelete( On 2017/07/21 21:18:09, jkarlin wrote: > This function isn't in the same order as the header and needs to be moved down > below DeleteDidQueryCache. Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1793: cache_padding_ == CacheStorage::kSizeUnknown) { On 2017/07/21 21:18:08, jkarlin wrote: > Can we DCHECK that the cache hasn't initialized yet if we're in this state? Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1628: // Padding can be zero bytes, so if the opaque URL + padding key changes in On 2017/07/21 21:18:09, jkarlin wrote: > Suggest: > > // This test is fragile. Right now it deterministically adds non-zero padding. > But if the url, padding key, or padding algorithm change it might become zero. Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1641: EXPECT_NE(opaque_cache_size_with_side_data, opaque_cache_size); Agreed. I've changed a fair amount of this function to hopefully be a better test and more clear. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1650: EXPECT_EQ(size_after_delete, opaque_cache_size); On 2017/07/21 21:18:09, jkarlin wrote: > Also delete the opaque data and verify that the size returns to > non_padded_cache_size. Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1652: I added CacheStorageManagerTest.CacheSizePaddedAfterReopen and CacheStorageManagerTest.PersistedCacheKeyUsed. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... File content/browser/cache_storage/cache_storage_index.h (right): https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:59: const CacheMetadata* FindMetadata(const std::string& cache_name) const; On 2017/07/21 21:18:09, jkarlin wrote: > nit: I slightly prefer GetMetadata to FindMetadata, especially since there is a > Set* function as well as a "GetPaddedStorageSize". Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:96: int64_t GetCacheSize(const std::string& cache_name) const; On 2017/07/21 21:18:09, jkarlin wrote: > GetCacheSizeForTesting Done. https://codereview.chromium.org/2901083002/diff/140001/content/browser/cache_... content/browser/cache_storage/cache_storage_index.h:100: int64_t GetCachePadding(const std::string& cache_name) const; On 2017/07/21 21:18:09, jkarlin wrote: > GetCachePaddingForTesting Done.
The CQ bit was checked by jkarlin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Think we're pretty much there. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:159: Opaque responses may only be cached, and in order to prevent "leaking" the size what do you mean "may only be cached". What is it that they can't do? https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:162: size. actual resource size via quota apis. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:166: for opaque resources. This key is also persisted to the cache index. How about replacing the last line with: Each cache's key is persisted to disk in the cache index file. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:172: The algorithm version is also written to each cache. This allows for the s/algorithm/padding algorithm/ https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:54: class KeyOwner { optional nit: SymmetricKeyOwner? https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:765: padded_size - cache->cache_padding()); I'd rather both sizes (padded and cache) were supplied to this function as arguments, or neither. You can always use cache->cache_size() and cache->cache_padding(). https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1559: void CacheStorageCache::DeleteDidDelete( This method shouldn't be needed any longer once we revert the query callback to an error callback. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:369: QueryCacheCallback callback); Why not revert this back to an ErrorCallback? I don't believe any consumer is using the query results at this point. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:447: // The actual physical cache size (not including padding). Remove the word physical? https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1652: EXPECT_GT(opaque_padding, 0); Let's make this an ASSERT https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1658: EXPECT_NE(current_padding, opaque_padding); swap the argument order to the EXPECT https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:974: EXPECT_EQ(GetQuotaOriginUsage(origin1_), cache_size_before_close); swap argument order https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1013: EXPECT_GT(cache_size_after_put, 0); swap argument order https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1033: EXPECT_EQ(Size(origin1_), cache_size_after_put); swap argument order https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1038: EXPECT_EQ(Size(origin1_), 0); swap argument order https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1047: EXPECT_NE(Size(origin1_), cache_size_after_put); swap argument order
https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:159: Opaque responses may only be cached, and in order to prevent "leaking" the size On 2017/07/31 14:03:24, jkarlin wrote: > what do you mean "may only be cached". What is it that they can't do? haha - yeah, good point. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:162: size. On 2017/07/31 14:03:24, jkarlin wrote: > actual resource size via quota apis. Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:166: for opaque resources. This key is also persisted to the cache index. On 2017/07/31 14:03:24, jkarlin wrote: > How about replacing the last line with: > > Each cache's key is persisted to disk in the cache index file. Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/README.md:172: The algorithm version is also written to each cache. This allows for the On 2017/07/31 14:03:24, jkarlin wrote: > s/algorithm/padding algorithm/ Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:54: class KeyOwner { On 2017/07/31 14:03:24, jkarlin wrote: > optional nit: SymmetricKeyOwner? Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage.cc:765: padded_size - cache->cache_padding()); On 2017/07/31 14:03:24, jkarlin wrote: > I'd rather both sizes (padded and cache) were supplied to this function as > arguments, or neither. You can always use cache->cache_size() and > cache->cache_padding(). You're right - way nicer that way. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.cc:1559: void CacheStorageCache::DeleteDidDelete( Thx for noticing. Much simpler w/o it. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache.h (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:369: QueryCacheCallback callback); On 2017/07/31 14:03:24, jkarlin wrote: > Why not revert this back to an ErrorCallback? I don't believe any consumer is > using the query results at this point. Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache.h:447: // The actual physical cache size (not including padding). On 2017/07/31 14:03:24, jkarlin wrote: > Remove the word physical? Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_cache_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1652: EXPECT_GT(opaque_padding, 0); On 2017/07/31 14:03:24, jkarlin wrote: > Let's make this an ASSERT Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_cache_unittest.cc:1658: EXPECT_NE(current_padding, opaque_padding); On 2017/07/31 14:03:24, jkarlin wrote: > swap the argument order to the EXPECT Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:974: EXPECT_EQ(GetQuotaOriginUsage(origin1_), cache_size_before_close); On 2017/07/31 14:03:25, jkarlin wrote: > swap argument order Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1013: EXPECT_GT(cache_size_after_put, 0); Are you sure? EXPECT_GT(arg1, arg2) is expecting that arg1 > arg2 right? https://github.com/google/googletest/blob/master/googletest/docs/Primer.md https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1033: EXPECT_EQ(Size(origin1_), cache_size_after_put); On 2017/07/31 14:03:25, jkarlin wrote: > swap argument order Done. https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1047: EXPECT_NE(Size(origin1_), cache_size_after_put); Done, but I don't think that either of these are "expected" arguments are they?
lgtm! Woot! (with some minor comments) https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... File content/browser/cache_storage/cache_storage_manager_unittest.cc (right): https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1013: EXPECT_GT(cache_size_after_put, 0); On 2017/08/10 16:37:11, cmumford wrote: > Are you sure? EXPECT_GT(arg1, arg2) is expecting that arg1 > arg2 right? > > https://github.com/google/googletest/blob/master/googletest/docs/Primer.md Sorry. What I'm looking for is to have the expected value as the first argument, so: EXPECT_LT(0, cache_size_after_put); https://codereview.chromium.org/2901083002/diff/160001/content/browser/cache_... content/browser/cache_storage/cache_storage_manager_unittest.cc:1047: EXPECT_NE(Size(origin1_), cache_size_after_put); On 2017/08/10 16:37:11, cmumford wrote: > Done, but I don't think that either of these are "expected" arguments are they? cache_size_after_put is the thing that is known (via previous checks) to be correct, and Size(origin1_) is what's being tested here. https://codereview.chromium.org/2901083002/diff/180001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2901083002/diff/180001/content/browser/cache_... content/browser/cache_storage/README.md:159: Opaque responses are also also cached, but in order to prevent "leaking" the size s/also also/also/
https://codereview.chromium.org/2901083002/diff/180001/content/browser/cache_... File content/browser/cache_storage/README.md (right): https://codereview.chromium.org/2901083002/diff/180001/content/browser/cache_... content/browser/cache_storage/README.md:159: Opaque responses are also also cached, but in order to prevent "leaking" the size On 2017/08/11 15:04:37, jkarlin wrote: > s/also also/also/ Done.
The CQ bit was checked by cmumford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org Link to the patchset: https://codereview.chromium.org/2901083002/#ps200001 (title: "s/also also/also/ and EXPECT_GT ↔ EXPECT_LT")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by cmumford@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1502805820608670,
"parent_rev": "57d922f5c5192622791005aecc78e5f92df78025", "commit_rev":
"8421200aab24ddf59dfea7256ccffed8f7a61e0b"}
Message was sent while issue was closed.
Description was changed from ========== [CacheStorage] Pad and bin opaque resource sizes. BUG=617963 ========== to ========== [CacheStorage] Pad and bin opaque resource sizes. BUG=617963 Review-Url: https://codereview.chromium.org/2901083002 Cr-Commit-Position: refs/heads/master@{#494384} Committed: https://chromium.googlesource.com/chromium/src/+/8421200aab24ddf59dfea7256ccf... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/8421200aab24ddf59dfea7256ccf...
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/3002693002/ by reillyg@chromium.org. The reason for reverting is: Crashes in these virtual test suites: * virtual/mojo-blobs/external/wpt/service-workers/service-worker/fetch-canvas-tainting-cache.https.html * virtual/off-main-thread-fetch/external/wpt/service-workers/service-worker/fetch-canvas-tainting-cache.https.html https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Linu.... |
