[Fonts] Enforce per-document CSP on cached CSS subresources [chromium/src : main]
Dileep Maurya (Gerrit)
Dileep Maurya voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Divyansh Mangal
- Gaurav Kumar
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaurav Kumar (Gerrit)
Gaurav Kumar added 4 comments![]( "Open in Gerrit")
// Fall through to a fresh fetch; do not overwrite `fetched_` so that
// other documents sharing this value still get a cache hit.Move it after line 165.
// FIXME: CSSFontFaceSrcValue::Fetch is invoked when @font-face rule
// is processed by StyleResolver / StyleEngine.Is it fixed ? if not please add this comment.
const CSSUrlData& url_data = src_value_->UrlData();
const CSSUrlRequestModifiers& modifiers = url_data.GetModifiers();
const Referrer& referrer = url_data.GetReferrer();
ResourceRequest resource_request(url_data.ResolvedUrl());
if (modifiers.referrer_policy) {
resource_request.SetReferrerPolicy(*modifiers.referrer_policy);
} else {
resource_request.SetReferrerPolicy(
ReferrerUtils::MojoReferrerPolicyResolveDefault(
referrer.referrer_policy));
}
resource_request.SetReferrerString(referrer.referrer);
if (url_data.IsAdRelated()) {
resource_request.SetIsAdResource();
}
ResourceLoaderOptions options(world_);
options.initiator_info.name = fetch_initiator_type_names::kCSS;
if (referrer.referrer != Referrer::ClientReferrerString()) {
options.initiator_info.referrer = referrer.referrer;
}
FetchParameters params(std::move(resource_request), options);
if (base::FeatureList::IsEnabled(
features::kWebFontsCacheAwareTimeoutAdaption)) {
params.SetCacheAwareLoadingEnabled(kIsCacheAwareLoadingEnabled);
}
params.SetFromOriginDirtyStyleSheet(!url_data.IsFromOriginCleanStyleSheet());
const SecurityOrigin* security_origin = context->GetSecurityOrigin();
// Local fonts are accessible from file: URLs even when
// allowFileAccessFromFileURLs is false.
if (!params.Url().IsLocalFile()) {
CrossOriginAttributeValue cross_origin =
modifiers.cross_origin != kCrossOriginAttributeNotSet
? modifiers.cross_origin
: kCrossOriginAttributeAnonymous;
params.SetCrossOriginAccessControl(security_origin, cross_origin);
}
if (!modifiers.integrity.IsNull()) {
IntegrityMetadataSet metadata_set;
SubresourceIntegrity::ParseIntegrityAttribute(modifiers.integrity,
metadata_set, context);
params.SetIntegrityMetadata(metadata_set);
params.MutableResourceRequest().SetFetchIntegrity(modifiers.integrity,
context);
}Can we add this in else block ?
Also since this code is not changed can you format is so that only the diff is visible while review.
Related details
- Dileep Maurya
- Divyansh Mangal
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaurav Kumar (Gerrit)
Gaurav Kumar added 2 comments![]( "Open in Gerrit")
// A StyleSheetContents (and the CSSFontFaceSrcValues it owns) is shared
// across documents in the same renderer process via the MemoryCache.
// Re-run the current document's security checks (CSP, etc.) before
// surfacing a previously-fetched FontResource so this document is not
// handed a resource that its own policies would have blocked.NIT: We can remove the HOW part from above comment.
// The cached image is shared via a memory-cached StyleSheetContents
// across documents with different CSPs; reject it for this document
// without clearing the shared cache.NIT : IMO we do not need it. It is evident from the like 235.
Vinay Singh (Gerrit)
Vinay Singh added 2 comments![]( "Open in Gerrit")
http/tests/security/contentSecurityPolicy/font-src/.Consider wrapping this inside backticks.
```suggestion
`http/tests/security/contentSecurityPolicy/font-src/`
```
// We already fired DevTools / violation-report events on this fetcher for
// this resource. Re-run CanRequest with reporting suppressed so callers
// surfacing a cached resource still get the current verdict without
// duplicating reports.This function here need not understand what has happened before it.
```suggestion
// Re-run CanRequest with reporting suppressed so callers surfacing a
// cached resource still get the current verdict without duplicating
// reports.
```
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
return *fresh;return *fetched_
I think when CSP is set (or condition at L:216 return false) shouldn't a fresh reload of the resource should be returned (IIUC thats the security bug)? that is isn't the current code already captures it correctly?
Related details
- Dileep Maurya
Divyansh Mangal (Gerrit)
Divyansh Mangal added 1 comment![]( "Open in Gerrit")
Moving myself to CC but the plumbing logic looks sound
Related details
- Dileep Maurya
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Divyansh Mangal (Gerrit)
Divyansh Mangal added 5 comments![]( "Open in Gerrit")
FontResource* fresh = FontResource::Fetch(params, context->Fetcher(), client);When `RestoreCachedResourceIfNeeded()` returns a blocked reason (reporting the violation), the fall-through to `FontResource::Fetch()` will re-enter the path
`RequestResource()` -> `PrepareResourceRequestForCacheAccess()` -> `CanRequest(..., kReport)`, which fires a second `securitypolicyviolation` event for the same resource. (Please correct me if I am wrong here)
Could we either suppress reporting on the fallback fetch (e.g. via `params.SetSpeculativePreload()` or a dedicated suppress flag), or skip the fetch entirely and return an error `FontResource` directly?
return *fresh;nit: Add DCHECK(fresh); before return *fresh; to encode the non-null invariant locally.
StyleImage* CSSImageValue::CacheImage(
const Document& document,
CrossOriginAttributeValue cross_origin,
const float override_image_resolution) {
if (!cached_image_) {
const CSSUrlData& url_data = UrlData();
if (url_data.ResolvedUrl().empty()) {
url_data.ReResolveUrl(document);
}
FetchParameters params = PrepareFetch(document, cross_origin);
ImageResourceContent* image_content =
document.GetStyleEngine().CacheImageContent(params);
cached_image_ = MakeGarbageCollected<StyleFetchedImage>(
image_content, *url_data.MakeResolvedIfDanglingMarkup(document),
document,
params.Url(), override_image_resolution);
}
return cached_image_.Get();`CacheImage()` returns `cached_image_` without any per-document CSP check when it's already populated. This means `cross-fade()` and `image-set()`, which always go through `StyleImageLoader::Load()` → `CacheImage()`, can still serve a cached image from a permissive-CSP document to a strict-CSP document, bypassing the new enforcement.
Consider adding a CSP revalidation here when `cached_image_` already exists, similar to how `CachedStyleImage()` now calls `RestoreCachedResourceIfNeeded()`. Something like:
```
if (cached_image_) {
auto blocked = RestoreCachedResourceIfNeeded(document);
if (blocked) return nullptr;
return cached_image_.Get();
}
```
return Context().CanRequest(resource->GetType(), last_resource_request,
last_resource_request.Url(), silent_options,
ReportingDisposition::kSuppressReporting,
last_resource_request.GetRedirectInfo());Re-running `CanRequest(..., kSuppressReporting)` on the "already-emulated" path happens on every style recalc for elements using @font-face. For pages with many cached fonts, this adds per-recalc overhead.
Not blocking, but worth considering whether the result could be memoized per-fetcher if this shows up in profiles. (Maybe a TODO can be added?)
<!doctype html>Should we extend coverage by considering below cases too?
1. Consider adding a test for `img-src` / `background-image` via a shared stylesheet to cover the `CachedStyleImage()` returning `nullptr` path.
2. Consider triggering the font lookup twice in the strict iframe to exercise the "already-emulated" path and verify no duplicate violation event fires.
3. Optional: add a third iframe (permissive CSP) after the strict one, to prove that blocking in one document doesn't poison the shared cache for later allowed documents.
Virali Purbey (Gerrit)
Virali Purbey added 2 comments![]( "Open in Gerrit")
FontResource* fresh = FontResource::Fetch(params, context->Fetcher(), client);If the strict-CSP document loads this stylesheet first (before any permissive one), `fetched_` is null, `!fetched_` is true, `fetched_` gets set to the blocked/errored `FontResource`. A later permissive document would then reuse that errored resource. Could we guard with `if (fresh && !fresh->ErrorOccurred())` before writing to `fetched_`? Might be wrong here, happy to be corrected.
if (CachedResource(url)) {
return std::nullopt;
}
if (resource->ErrorOccurred()) {
return std::nullopt;
}
if (ResourceHasBeenEmulatedLoadStartedForInspector(url)) {nit: this swaps the `ErrorOccurred` / "already-emulated" check order from the old code, makes sense since the emulated path now calls `CanRequest`. Maybe a one-liner comment like `// Bail on error before the emulated path to avoid a needless CanRequest` will be nice?
Dileep Maurya (Gerrit)
Dileep Maurya added 15 comments![]( "Open in Gerrit")
Consider wrapping this inside backticks.
```suggestion
`http/tests/security/contentSecurityPolicy/font-src/`
```
Done
// A StyleSheetContents (and the CSSFontFaceSrcValues it owns) is shared
// across documents in the same renderer process via the MemoryCache.
// Re-run the current document's security checks (CSP, etc.) before
// surfacing a previously-fetched FontResource so this document is not
// handed a resource that its own policies would have blocked.NIT: We can remove the HOW part from above comment.
Done.
// Fall through to a fresh fetch; do not overwrite `fetched_` so that
// other documents sharing this value still get a cache hit.Move it after line 165.
Done. After restoring the original `if/else` structure, the fall-through comment now sits directly above the `return *fetch_from_network(`) in the blocked branch, where the no-overwrite behavior actually happens.
// FIXME: CSSFontFaceSrcValue::Fetch is invoked when @font-face rule
// is processed by StyleResolver / StyleEngine.Is it fixed ? if not please add this comment.
Done. Restored the FIXME above the cached-path branch.
const CSSUrlData& url_data = src_value_->UrlData();Can we add this in else block ?
Also since this code is not changed can you format is so that only the diff is visible while review.
Done. Restored the original `if (!fetched_) { fresh } else { cached }` structure. Extracted the fresh-fetch body into a local `fetch_from_network` lambda so the blocked-CSP path can reuse it without duplicating code.
FontResource* fresh = FontResource::Fetch(params, context->Fetcher(), client);When `RestoreCachedResourceIfNeeded()` returns a blocked reason (reporting the violation), the fall-through to `FontResource::Fetch()` will re-enter the path
`RequestResource()` -> `PrepareResourceRequestForCacheAccess()` -> `CanRequest(..., kReport)`, which fires a second `securitypolicyviolation` event for the same resource. (Please correct me if I am wrong here)
Could we either suppress reporting on the fallback fetch (e.g. via `params.SetSpeculativePreload()` or a dedicated suppress flag), or skip the fetch entirely and return an error `FontResource` directly?
Confirmed and fixed. `RestoreCachedResourceIfNeeded()` is now called with `ReportingDisposition::kSuppressReporting`, so only the fall-back `FontResource::Fetch()` reports. Verified by `result.violationCount == 1` even though the strict iframe calls `document.fonts.load()` twice.
FontResource* fresh = FontResource::Fetch(params, context->Fetcher(), client);If the strict-CSP document loads this stylesheet first (before any permissive one), `fetched_` is null, `!fetched_` is true, `fetched_` gets set to the blocked/errored `FontResource`. A later permissive document would then reuse that errored resource. Could we guard with `if (fresh && !fresh->ErrorOccurred())` before writing to `fetched_`? Might be wrong here, happy to be corrected.
Good catch - added `if (!fresh->ErrorOccurred()) fetched_ = fresh;`. New regression test `shared-stylesheet-cache-bypass-strict-first.html` covers exactly this ordering.
nit: Add DCHECK(fresh); before return *fresh; to encode the non-null invariant locally.
Done
Divyansh Mangalreturn *fetched_
I think when CSP is set (or condition at L:216 return false) shouldn't a fresh reload of the resource should be returned (IIUC thats the security bug)? that is isn't the current code already captures it correctly?
Yes, that's exactly what happens, but I agree the previous structure obscured it. After the restructure, the blocked-CSP path explicitly does return `*fetch_from_network()`, which issues a fresh fetch that the loader's `CanRequest` will properly block for this document.
StyleImage* CSSImageValue::CacheImage(
const Document& document,
CrossOriginAttributeValue cross_origin,
const float override_image_resolution) {
if (!cached_image_) {
const CSSUrlData& url_data = UrlData();
if (url_data.ResolvedUrl().empty()) {
url_data.ReResolveUrl(document);
}
FetchParameters params = PrepareFetch(document, cross_origin);
ImageResourceContent* image_content =
document.GetStyleEngine().CacheImageContent(params);
cached_image_ = MakeGarbageCollected<StyleFetchedImage>(
image_content, *url_data.MakeResolvedIfDanglingMarkup(document),
document,
params.Url(), override_image_resolution);
}
return cached_image_.Get();`CacheImage()` returns `cached_image_` without any per-document CSP check when it's already populated. This means `cross-fade()` and `image-set()`, which always go through `StyleImageLoader::Load()` → `CacheImage()`, can still serve a cached image from a permissive-CSP document to a strict-CSP document, bypassing the new enforcement.
Consider adding a CSP revalidation here when `cached_image_` already exists, similar to how `CachedStyleImage()` now calls `RestoreCachedResourceIfNeeded()`. Something like:
```
if (cached_image_) {
auto blocked = RestoreCachedResourceIfNeeded(document);
if (blocked) return nullptr;
return cached_image_.Get();
}
```
Done. Added the symmetric CSP check at the top of `CacheImage()`.
Note: I return a freshly-fetched (loader-blocked) `StyleFetchedImage` instead of `nullptr`, because callers like `EnsureSVGResource()` and `LoadMaskSource()'s` `To<StyleFetchedImage>(...)` non-null cast crash on `null`. Functionally equivalent, no bytes leak and the loader still fires the CSP report.
// The cached image is shared via a memory-cached StyleSheetContents
// across documents with different CSPs; reject it for this document
// without clearing the shared cache.NIT : IMO we do not need it. It is evident from the like 235.
Done.
if (CachedResource(url)) {
return std::nullopt;
}
if (resource->ErrorOccurred()) {
return std::nullopt;
}
if (ResourceHasBeenEmulatedLoadStartedForInspector(url)) {nit: this swaps the `ErrorOccurred` / "already-emulated" check order from the old code, makes sense since the emulated path now calls `CanRequest`. Maybe a one-liner comment like `// Bail on error before the emulated path to avoid a needless CanRequest` will be nice?
Done
// We already fired DevTools / violation-report events on this fetcher for
// this resource. Re-run CanRequest with reporting suppressed so callers
// surfacing a cached resource still get the current verdict without
// duplicating reports.This function here need not understand what has happened before it.
```suggestion
// Re-run CanRequest with reporting suppressed so callers surfacing a
// cached resource still get the current verdict without duplicating
// reports.
```
Done
return Context().CanRequest(resource->GetType(), last_resource_request,
last_resource_request.Url(), silent_options,
ReportingDisposition::kSuppressReporting,
last_resource_request.GetRedirectInfo());Re-running `CanRequest(..., kSuppressReporting)` on the "already-emulated" path happens on every style recalc for elements using @font-face. For pages with many cached fonts, this adds per-recalc overhead.
Not blocking, but worth considering whether the result could be memoized per-fetcher if this shows up in profiles. (Maybe a TODO can be added?)
Done. added TODO(crbug.com/504107363) noting we should memoize per-fetcher if profiles flag it.
Should we extend coverage by considering below cases too?
1. Consider adding a test for `img-src` / `background-image` via a shared stylesheet to cover the `CachedStyleImage()` returning `nullptr` path.
2. Consider triggering the font lookup twice in the strict iframe to exercise the "already-emulated" path and verify no duplicate violation event fires.
3. Optional: add a third iframe (permissive CSP) after the strict one, to prove that blocking in one document doesn't poison the shared cache for later allowed documents.
Thanks for your comment here. All three case added:
1. `img-src/shared-stylesheet-image-cache-bypass.html`: `img-src/background-image` via shared stylesheet.
2. `shared-stylesheet-cache-bypass-strict.html` now loads the font twice; main test asserts violationCount == 1.
3. `shared-stylesheet-cache-bypass-strict-first.html`: strict iframe first, permissive second; asserts the permissive iframe still loads the font (no cache poisoning).
Related details
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dileep Maurya (Gerrit)
Dileep Maurya voted Commit-Queue+1![]( "Open in Gerrit")
- Divyansh Mangal
- Gaurav Kumar
- Kevin Babbitt
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Babbitt (Gerrit)
Kevin Babbitt added 3 comments![]( "Open in Gerrit")
General shape seems okay to me, but I think someone with Fetch expertise should also have a look. yoavweiss@ would that be you?
if (cached_image_) {
// Suppress the emulated report: on block, the fall-back fetch below fires
// its own CSP violation report via the loader's CanRequest.
if (RestoreCachedResourceIfNeeded(
document, ReportingDisposition::kSuppressReporting)) {
// Fall through to a fresh fetch without overwriting `cached_image_` so
// other documents that legally share this value still get a cache hit.
} else {
return cached_image_.Get();
}
}Suggest simplifying this to:
```suggestion
if (cached_image_) {
// Suppress the emulated report: on block, the fall-back fetch below fires
// its own CSP violation report via the loader's CanRequest.
if (!RestoreCachedResourceIfNeeded(
document, ReportingDisposition::kSuppressReporting)) {
return cached_image_.Get();
}
// Otherwise, continue to a fresh fetch without overwriting `cached_image_`
// so other documents that legally share this value still get a cache hit.
}
```
// TODO(crbug.com/504107363): Runs on every style recalc for cached
// fonts/images; consider memoizing per-fetcher if it shows up in profiles.
ResourceRequest last_resource_request(resource->LastResourceRequest());Can you clarify this? Does "Runs on every style recalc" mean...
... runs once per document-wide style recalc pass?
... runs once per style recalc for an element that references the resource?
... something else?
Style recalc is a hot path, so if we think this is going to be slow, I think it's worth considering whether we should jump straight to memoization. Do we have any benchmarking to understand the cost?
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 2 comments![]( "Open in Gerrit")
FontResource* fresh =The logic flow here is very confusing. Can you stick to the original form of setting `fetched_` and returning it, while adding the new CSP logic?
DCHECK(fresh);`CHECK` instead of `DCHECK`?
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
Dileep Maurya (Gerrit)
Dileep Maurya added 4 comments![]( "Open in Gerrit")
The logic flow here is very confusing. Can you stick to the original form of setting `fetched_` and returning it, while adding the new CSP logic?
Done
`CHECK` instead of `DCHECK`?
Done
if (cached_image_) {
// Suppress the emulated report: on block, the fall-back fetch below fires
// its own CSP violation report via the loader's CanRequest.
if (RestoreCachedResourceIfNeeded(
document, ReportingDisposition::kSuppressReporting)) {
// Fall through to a fresh fetch without overwriting `cached_image_` so
// other documents that legally share this value still get a cache hit.
} else {
return cached_image_.Get();
}
}Suggest simplifying this to:
```suggestion
if (cached_image_) {
// Suppress the emulated report: on block, the fall-back fetch below fires
// its own CSP violation report via the loader's CanRequest.
if (!RestoreCachedResourceIfNeeded(
document, ReportingDisposition::kSuppressReporting)) {
return cached_image_.Get();
}
// Otherwise, continue to a fresh fetch without overwriting `cached_image_`
// so other documents that legally share this value still get a cache hit.
}
```
Done
// TODO(crbug.com/504107363): Runs on every style recalc for cached
// fonts/images; consider memoizing per-fetcher if it shows up in profiles.
ResourceRequest last_resource_request(resource->LastResourceRequest());Can you clarify this? Does "Runs on every style recalc" mean...
... runs once per document-wide style recalc pass?
... runs once per style recalc for an element that references the resource?
... something else?Style recalc is a hot path, so if we think this is going to be slow, I think it's worth considering whether we should jump straight to memoization. Do we have any benchmarking to understand the cost?
1. "Runs on every style recalc" frequency:
- Font path (`CSSFontFaceSrcValue::Fetch`): once per FontFace init. Callers are `FontFace::InitCSSFontFace` and `FrameSerializer::RetrieveResourcesForCSSValue`. Not per style recalc.
- Image path (`CSSImageValue::RestoreCachedResourceIfNeeded`): once per (element, image-using property) per recalc that resolves a non-pending `CSSImageValue`. Reached via `StyleImageLoader::Load`, `LoadMaskSource`, and directly from `CachedStyleImage`.
2. Benchmarks (two data points, Win11 x64):
- Infrastructure-only microbench (`MockFetchContext, 100k iters`): ~187 ns / call for the cached branch.
- Real-world cost (WPT `shared-stylesheet-cache-bypass.html`, real CSP): ~234 µs / call (dominated by `Context().CanRequest(...)'s` policy walk).
- So the per-recalc image-path cost in the cross-doc cache scenario is roughly `N × M × 234 µs` (N image-using properties × M elements). 10 elements would consume ~2.3 ms of frame budget; 100 elements would overflow it.
As per analysis locally, memoization was needed - added the memoization in this CL.
- Combined with the earlier WPT-measured `~234 µs per real-CSP CanRequest`, steady-state cost for cross-document cache hits collapses from `N × M × 234 µs` to `N × M × ~106 ns`, with one ~234 µs catch-up per (URL, CSP-version) change.
Related details
- Divyansh Mangal
- Gaurav Kumar
- Kevin Babbitt
- Vinay Singh
- Virali Purbey
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dileep Maurya (Gerrit)
Dileep Maurya added 1 comment![]( "Open in Gerrit")
Kevin Babbitt (Gerrit)
Kevin Babbitt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kevin Babbitt
| Code-Review | +1 |
1 comment
const String cache_key = MemoryCache::RemoveFragmentIdentifierIfNeeded(url);It's strange that we added a test for this code but the coverage result is showing it as "not covered." Though the entire change is showing 0% coverage so maybe it's an issue with the bot. Could you please double-check to confirm that this code is actually being exercised by the test?
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Dileep Maurya (Gerrit)
Dileep Maurya added 1 comment![]( "Open in Gerrit")
const String cache_key = MemoryCache::RemoveFragmentIdentifierIfNeeded(url);It's strange that we added a test for this code but the coverage result is showing it as "not covered." Though the entire change is showing 0% coverage so maybe it's an issue with the bot. Could you please double-check to confirm that this code is actually being exercised by the test?
Thanks for the review Kevin. I ran `ResourceFetcherTestBase.EmulateLoadStartedForInspectorMemoizesUntilCspChanges` test and verified it locally. The memoization code get executed as expected. The Gerrit Coverage tab reads 0% what it looks to be `*-code-coverage` tryjobs failing for infrastructure reasons unrelated to this CL.
Related details
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Dileep Maurya (Gerrit)
Dileep Maurya added 1 comment![]( "Open in Gerrit")
Dileep Maurya (Gerrit)
Dileep Maurya added 1 comment![]( "Open in Gerrit")
yoav...@chromium.org gentle ping on this. Please take a look when you have a chance. Thanks!
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
} else if (RestoreCachedResourceIfNeeded(It's confusing that this returns a value only on failure. Can you consider renaming this method to make that obvious, or take that value into a variable that makes that clearer?
Related details
- Dileep Maurya
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Dileep Maurya (Gerrit)
Dileep Maurya voted and added 1 comment![]( "Open in Gerrit")
Votes added by Dileep Maurya
| Commit-Queue | +1 |
1 comment
It's confusing that this returns a value only on failure. Can you consider renaming this method to make that obvious, or take that value into a variable that makes that clearer?
Thanks for pointing this out. I've updated the method name to make it obvious, PTAL!
Related details
- Divyansh Mangal
- Gaurav Kumar
- Vinay Singh
- Virali Purbey
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |