From 41c3fd6f74cbc61ce8aea86ead54ecd369cbd34c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Tue, 22 Jul 2025 21:50:49 +0000 Subject: [PATCH] Bug 1978217 - Don't share sheets using relative URIs when it's not safe to do so. r=firefox-style-system-reviewers,layout-reviewers,dshin a=dsmith Track whether an inline sheet might have uris that depend on the base, and avoid caching them if appropriate. Differential Revision: https://phabricator.services.mozilla.com/D258292 --- layout/style/Loader.cpp | 58 ++++++++++++++++++-- layout/style/Loader.h | 3 +- layout/style/StyleSheet.cpp | 14 +++++ layout/style/StyleSheet.h | 4 ++ servo/components/style/gecko/url.rs | 61 ++++++++++++++++++++++ servo/components/style/use_counters/mod.rs | 4 ++ servo/ports/geckolib/cbindgen.toml | 1 + 7 files changed, 140 insertions(+), 5 deletions(-) diff --git a/layout/style/Loader.cpp b/layout/style/Loader.cpp index 993eba086505..52e1e7ed4599 100644 --- a/layout/style/Loader.cpp +++ b/layout/style/Loader.cpp @@ -1648,23 +1648,72 @@ void Loader::MarkLoadTreeFailed(SheetLoadData& aLoadData, } while (data); } +static bool URIsEqual(nsIURI* aA, nsIURI* aB) { + if (aA == aB) { + return true; + } + if (!aA || !aB) { + return false; + } + bool equal = false; + return NS_SUCCEEDED(aA->Equals(aB, &equal)) && equal; +} + +// The intention is that this would return true for inputs like +// (https://example.com/a, https://example.com/b) +// but not for: +// (https://example.com/a, https://example.com/b/c) +// where "regular" relative URIs would resolve differently. +static bool BaseURIsArePathCompatible(nsIURI* aA, nsIURI* aB) { + if (!aA || !aB) { + return false; + } + constexpr auto kDummyPath = "foo.css"_ns; + nsAutoCString resultA; + nsAutoCString resultB; + aA->Resolve(kDummyPath, resultA); + aB->Resolve(kDummyPath, resultB); + return resultA == resultB; +} + +static bool CanReuseInlineSheet(StyleSheet* aSheet, nsIURI* aNewBaseURI) { + nsIURI* oldBase = aSheet->GetBaseURI(); + if (URIsEqual(oldBase, aNewBaseURI)) { + return true; + } + switch (aSheet->OriginalContentsBaseUriDependency()) { + case StyleLikelyBaseUriDependency::No: + break; + case StyleLikelyBaseUriDependency::Path: + if (BaseURIsArePathCompatible(oldBase, aNewBaseURI)) { + break; + } + [[fallthrough]]; + case StyleLikelyBaseUriDependency::Full: + LOG((" Can't reuse due to base URI dependency")); + return false; + } + return true; +} + RefPtr Loader::LookupInlineSheetInCache( - const nsAString& aBuffer, nsIPrincipal* aSheetPrincipal) { + const nsAString& aBuffer, nsIPrincipal* aSheetPrincipal, nsIURI* aBaseURI) { MOZ_ASSERT(mSheets, "Document associated loader should have sheet cache"); auto result = mSheets->LookupInline(LoaderPrincipal(), aBuffer); if (!result) { return nullptr; } - StyleSheet* sheet = result.Data(); MOZ_ASSERT(!sheet->HasModifiedRules(), "How did we end up with a dirty sheet?"); - if (NS_WARN_IF(!sheet->Principal()->Equals(aSheetPrincipal))) { // If the sheet is going to have different access rights, don't return it // from the cache. XXX can this happen now that we eagerly clone? return nullptr; } + if (!CanReuseInlineSheet(sheet, aBaseURI)) { + return nullptr; + } return sheet->Clone(nullptr, nullptr); } @@ -1732,7 +1781,8 @@ Result Loader::LoadInlineStyle( return LoaderPrincipal(); }(); - RefPtr sheet = LookupInlineSheetInCache(aBuffer, sheetPrincipal); + RefPtr sheet = + LookupInlineSheetInCache(aBuffer, sheetPrincipal, baseURI); const bool isSheetFromCache = !!sheet; if (!isSheetFromCache) { sheet = MakeRefPtr(eAuthorSheetFeatures, aInfo.mCORSMode, diff --git a/layout/style/Loader.h b/layout/style/Loader.h index b3071be6a46a..b273486d21c9 100644 --- a/layout/style/Loader.h +++ b/layout/style/Loader.h @@ -567,7 +567,8 @@ class Loader final { CORSMode aCORSMode, const nsAString& aNonce, const nsAString& aIntegrity, uint64_t aEarlyHintPreloaderId, dom::FetchPriority aFetchPriority); - RefPtr LookupInlineSheetInCache(const nsAString&, nsIPrincipal*); + RefPtr LookupInlineSheetInCache(const nsAString&, nsIPrincipal*, + nsIURI* aBaseURI); // Synchronously notify of a cached load data. void NotifyOfCachedLoad(RefPtr); diff --git a/layout/style/StyleSheet.cpp b/layout/style/StyleSheet.cpp index 7b8ee0057716..f5d4ee84c54b 100644 --- a/layout/style/StyleSheet.cpp +++ b/layout/style/StyleSheet.cpp @@ -1254,6 +1254,20 @@ void StyleSheet::FinishAsyncParse( UnblockParsePromise(); } +StyleLikelyBaseUriDependency StyleSheet::OriginalContentsBaseUriDependency() + const { + const auto* counters = UseCounters(); + if (Servo_IsCustomUseCounterRecorded( + counters, StyleCustomUseCounter::MaybeHasFullBaseUriDependency)) { + return StyleLikelyBaseUriDependency::Full; + } + if (Servo_IsCustomUseCounterRecorded( + counters, StyleCustomUseCounter::MaybeHasPathBaseUriDependency)) { + return StyleLikelyBaseUriDependency::Path; + } + return StyleLikelyBaseUriDependency::No; +} + const StyleUseCounters* StyleSheet::UseCounters() const { return Servo_StyleSheet_UseCounters(RawContents()); } diff --git a/layout/style/StyleSheet.h b/layout/style/StyleSheet.h index 12d1a8cc2300..117d5aaf7822 100644 --- a/layout/style/StyleSheet.h +++ b/layout/style/StyleSheet.h @@ -41,6 +41,7 @@ using StyleSheetParsePromise = MozPromise; enum class StyleRuleChangeKind : uint32_t; +enum class StyleLikelyBaseUriDependency : uint8_t; struct StyleRuleChange { StyleRuleChange() = delete; @@ -159,6 +160,9 @@ class StyleSheet final : public nsICSSLoaderObserver, public nsWrapperCache { const StyleUseCounters* UseCounters() const; void PropagateUseCountersTo(dom::Document*) const; + // Whether our original contents may be using relative URIs. + StyleLikelyBaseUriDependency OriginalContentsBaseUriDependency() const; + URLExtraData* URLData() const { return Inner().mURLData; } // nsICSSLoaderObserver interface diff --git a/servo/components/style/gecko/url.rs b/servo/components/style/gecko/url.rs index 26c148a0d10b..09f0342a17fd 100644 --- a/servo/components/style/gecko/url.rs +++ b/servo/components/style/gecko/url.rs @@ -59,6 +59,52 @@ impl PartialEq for CssUrlData { } } +/// How an URI might depend on our base URI. +/// +/// TODO(emilio): See if necko can provide this, but for our case local refs or empty URIs are +/// totally fine even though they wouldn't be in general... +#[repr(u8)] +pub enum LikelyBaseUriDependency { + /// No dependency on the base URI (either absolute uri, or relative URI). + No, + /// We might depend on our path depth. E.g. `https://example.com/foo` and + /// `https://example.com/bar` both resolve a relative URI like `baz.css` as + /// `https://example.com/baz.css`. + Path, + /// We might depend on our full URI. This is the case for query strings (and, in the general + /// case, local refs and empty URIs, but that's not the case for CSS as described below. + Full, +} + +fn likely_base_uri_dependency(specified: &str) -> LikelyBaseUriDependency { + if specified.is_empty() { + // In CSS the empty URL is special / invalid. + // https://drafts.csswg.org/css-values-4/#url-empty + return LikelyBaseUriDependency::No; + } + if specified.starts_with('#') || specified.starts_with('/') { + // Local refs and absolute paths are fair game. + return LikelyBaseUriDependency::No; + } + const COMMON_PROTOCOLS: [&str; 3] = [ + "http:", + "https:", + "data:", + ]; + for protocol in COMMON_PROTOCOLS { + if specified.starts_with(protocol) { + // Common absolute URIs. + return LikelyBaseUriDependency::No; + } + } + if specified.starts_with('?') { + // Query string resolves differently for any two different base URIs + return LikelyBaseUriDependency::Full; + } + // Might be a relative URI, play it safe. + LikelyBaseUriDependency::Path +} + impl CssUrl { /// Parse a URL with a particular CORS mode. pub fn parse_with_cors_mode<'i, 't>( @@ -76,6 +122,21 @@ impl CssUrl { /// Parse a URL from a string value that is a valid CSS token for a URL. pub fn parse_from_string(url: String, context: &ParserContext, cors_mode: CorsMode) -> Self { + use crate::use_counters::CustomUseCounter; + if let Some(counters) = context.use_counters { + if !counters.custom.recorded(CustomUseCounter::MaybeHasFullBaseUriDependency) { + match likely_base_uri_dependency(&url) { + LikelyBaseUriDependency::No => {}, + LikelyBaseUriDependency::Path => { + counters.custom.record(CustomUseCounter::MaybeHasPathBaseUriDependency); + }, + LikelyBaseUriDependency::Full => { + counters.custom.record(CustomUseCounter::MaybeHasPathBaseUriDependency); + counters.custom.record(CustomUseCounter::MaybeHasFullBaseUriDependency); + }, + } + } + } CssUrl(Arc::new(CssUrlData { serialization: url.into(), extra_data: context.url_data.clone(), diff --git a/servo/components/style/use_counters/mod.rs b/servo/components/style/use_counters/mod.rs index 2f2293acf9ac..f4bc8d897110 100644 --- a/servo/components/style/use_counters/mod.rs +++ b/servo/components/style/use_counters/mod.rs @@ -30,6 +30,10 @@ pub struct NonCustomPropertyUseCounters { #[derive(Copy, Clone, Debug)] #[repr(u32)] pub enum CustomUseCounter { + /// Whether we are likely to be using relative URIs that depend on our path depth. + MaybeHasPathBaseUriDependency = 0, + /// Whether we are likely to be using relative URIs that depend on our full URI. + MaybeHasFullBaseUriDependency, /// Dummy value, used for indexing purposes. Last, } diff --git a/servo/ports/geckolib/cbindgen.toml b/servo/ports/geckolib/cbindgen.toml index fbf7cdc679b3..4dd6de09bb38 100644 --- a/servo/ports/geckolib/cbindgen.toml +++ b/servo/ports/geckolib/cbindgen.toml @@ -175,6 +175,7 @@ include = [ "Overflow", "LengthPercentage", "LetterSpacing", + "LikelyBaseUriDependency", "NonNegativeLengthPercentage", "LengthPercentageOrAuto", "LineHeight",