From 7d479f6a4a76c9f3f9ce92a12799f68f96088367 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Thu, 25 May 2023 22:25:31 +0000 Subject: [PATCH] Bug 1832528 - Make cors mode part of the image cache key. r=tnikkel So that we can hold entries loaded with different CORS modes, such as masks and backgrounds. Differential Revision: https://phabricator.services.mozilla.com/D177759 --- dom/base/CORSMode.h | 3 ++ dom/base/Document.cpp | 8 ++--- dom/base/nsContentUtils.cpp | 24 ++++++--------- dom/base/nsContentUtils.h | 7 ++--- image/ImageCacheKey.cpp | 16 +++++++--- image/ImageCacheKey.h | 11 +++++-- image/imgLoader.cpp | 58 ++++++++++++++++++++----------------- 7 files changed, 70 insertions(+), 57 deletions(-) diff --git a/dom/base/CORSMode.h b/dom/base/CORSMode.h index 707d54a0f944..25ad0437ba00 100644 --- a/dom/base/CORSMode.h +++ b/dom/base/CORSMode.h @@ -30,6 +30,9 @@ enum CORSMode : uint8_t { CORS_USE_CREDENTIALS }; +constexpr auto kFirstCORSMode = CORS_NONE; +constexpr auto kLastCORSMode = CORS_USE_CREDENTIALS; + } // namespace mozilla #endif /* CORSMode_h_ */ diff --git a/dom/base/Document.cpp b/dom/base/Document.cpp index f01b2c9c74b8..ee78d4a38075 100644 --- a/dom/base/Document.cpp +++ b/dom/base/Document.cpp @@ -12379,12 +12379,12 @@ void Document::MaybePreLoadImage(nsIURI* aUri, ReferrerPolicyEnum aReferrerPolicy, bool aIsImgSet, bool aLinkPreload, const TimeStamp& aInitTimestamp) { + const CORSMode corsMode = dom::Element::StringToCORSMode(aCrossOriginAttr); if (aLinkPreload) { // Check if the image was already preloaded in this document to avoid // duplicate preloading. - PreloadHashKey key = PreloadHashKey::CreateAsImage( - aUri, NodePrincipal(), - dom::Element::StringToCORSMode(aCrossOriginAttr)); + PreloadHashKey key = + PreloadHashKey::CreateAsImage(aUri, NodePrincipal(), corsMode); if (!mPreloadService.PreloadExists(key)) { PreLoadImage(aUri, aCrossOriginAttr, aReferrerPolicy, aIsImgSet, aLinkPreload, 0); @@ -12395,7 +12395,7 @@ void Document::MaybePreLoadImage(nsIURI* aUri, // Early exit if the img is already present in the img-cache // which indicates that the "real" load has already started and // that we shouldn't preload it. - if (nsContentUtils::IsImageInCache(aUri, this)) { + if (nsContentUtils::IsImageAvailable(aUri, NodePrincipal(), corsMode, this)) { return; } diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index ee9908ba3c58..65538f193c91 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -3844,19 +3844,6 @@ imgLoader* nsContentUtils::GetImgLoaderForChannel(nsIChannel* aChannel, : imgLoader::NormalLoader(); } -// static -bool nsContentUtils::IsImageInCache(nsIURI* aURI, Document* aDocument) { - imgILoader* loader = GetImgLoaderForDocument(aDocument); - nsCOMPtr cache = do_QueryInterface(loader); - - // If something unexpected happened we return false, otherwise if props - // is set, the image is cached and we return true - nsCOMPtr props; - nsresult rv = - cache->FindEntryProperties(aURI, aDocument, getter_AddRefs(props)); - return (NS_SUCCEEDED(rv) && props); -} - // static int32_t nsContentUtils::CORSModeToLoadImageFlags(mozilla::CORSMode aMode) { switch (aMode) { @@ -10026,8 +10013,15 @@ bool nsContentUtils::IsImageAvailable(nsIContent* aLoadingNode, nsIURI* aURI, MOZ_ASSERT(triggeringPrincipal); Document* doc = aLoadingNode->OwnerDoc(); - imgLoader* imgLoader = GetImgLoaderForDocument(doc); - return imgLoader->IsImageAvailable(aURI, triggeringPrincipal, aCORSMode, doc); + return IsImageAvailable(aURI, triggeringPrincipal, aCORSMode, doc); +} + +bool nsContentUtils::IsImageAvailable(nsIURI* aURI, + nsIPrincipal* aTriggeringPrincipal, + CORSMode aCORSMode, Document* aDoc) { + imgLoader* imgLoader = GetImgLoaderForDocument(aDoc); + return imgLoader->IsImageAvailable(aURI, aTriggeringPrincipal, aCORSMode, + aDoc); } /* static */ diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h index 4e7c24594382..929224c2a55e 100644 --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ -1051,11 +1051,6 @@ class nsContentUtils { static imgLoader* GetImgLoaderForChannel(nsIChannel* aChannel, Document* aContext); - /** - * Returns whether the given URI is in the image cache. - */ - static bool IsImageInCache(nsIURI* aURI, Document* aDocument); - /** * Method to get an imgIContainer from an image loading content * @@ -3099,6 +3094,8 @@ class nsContentUtils { static bool IsImageAvailable(nsIContent*, nsIURI*, nsIPrincipal* aDefaultTriggeringPrincipal, mozilla::CORSMode); + static bool IsImageAvailable(nsIURI*, nsIPrincipal* aTriggeringPrincipal, + mozilla::CORSMode, Document*); /** * Returns the content policy type that should be used for loading images diff --git a/image/ImageCacheKey.cpp b/image/ImageCacheKey.cpp index 141985bf05e5..7a5f78b70fd3 100644 --- a/image/ImageCacheKey.cpp +++ b/image/ImageCacheKey.cpp @@ -29,26 +29,30 @@ using namespace dom; namespace image { -ImageCacheKey::ImageCacheKey(nsIURI* aURI, const OriginAttributes& aAttrs, +ImageCacheKey::ImageCacheKey(nsIURI* aURI, CORSMode aCORSMode, + const OriginAttributes& aAttrs, Document* aDocument) : mURI(aURI), mOriginAttributes(aAttrs), mControlledDocument(GetSpecialCaseDocumentToken(aDocument)), - mIsolationKey(GetIsolationKey(aDocument, aURI)) {} + mIsolationKey(GetIsolationKey(aDocument, aURI)), + mCORSMode(aCORSMode) {} ImageCacheKey::ImageCacheKey(const ImageCacheKey& aOther) : mURI(aOther.mURI), mOriginAttributes(aOther.mOriginAttributes), mControlledDocument(aOther.mControlledDocument), mIsolationKey(aOther.mIsolationKey), - mHash(aOther.mHash) {} + mHash(aOther.mHash), + mCORSMode(aOther.mCORSMode) {} ImageCacheKey::ImageCacheKey(ImageCacheKey&& aOther) : mURI(std::move(aOther.mURI)), mOriginAttributes(aOther.mOriginAttributes), mControlledDocument(aOther.mControlledDocument), mIsolationKey(aOther.mIsolationKey), - mHash(aOther.mHash) {} + mHash(aOther.mHash), + mCORSMode(aOther.mCORSMode) {} bool ImageCacheKey::operator==(const ImageCacheKey& aOther) const { // Don't share the image cache between a controlled document and anything @@ -67,6 +71,10 @@ bool ImageCacheKey::operator==(const ImageCacheKey& aOther) const { return false; } + if (mCORSMode != aOther.mCORSMode) { + return false; + } + // For non-blob URIs, compare the URIs. bool equals = false; nsresult rv = mURI->Equals(aOther.mURI, &equals); diff --git a/image/ImageCacheKey.h b/image/ImageCacheKey.h index 7567bd923849..2be4493081bd 100644 --- a/image/ImageCacheKey.h +++ b/image/ImageCacheKey.h @@ -18,6 +18,9 @@ class nsIURI; namespace mozilla { + +enum CORSMode : uint8_t; + namespace image { /** @@ -30,8 +33,7 @@ namespace image { */ class ImageCacheKey final { public: - ImageCacheKey(nsIURI* aURI, const OriginAttributes& aAttrs, - dom::Document* aDocument); + ImageCacheKey(nsIURI*, CORSMode, const OriginAttributes&, dom::Document*); ImageCacheKey(const ImageCacheKey& aOther); ImageCacheKey(ImageCacheKey&& aOther); @@ -47,6 +49,8 @@ class ImageCacheKey final { /// A weak pointer to the URI. nsIURI* URI() const { return mURI; } + CORSMode GetCORSMode() const { return mCORSMode; } + const OriginAttributes& OriginAttributesRef() const { return mOriginAttributes; } @@ -70,10 +74,11 @@ class ImageCacheKey final { void EnsureHash() const; nsCOMPtr mURI; - OriginAttributes mOriginAttributes; + const OriginAttributes mOriginAttributes; void* mControlledDocument; nsCString mIsolationKey; mutable Maybe mHash; + const CORSMode mCORSMode; }; } // namespace image diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp index 37b41c68de73..dcfb031789ed 100644 --- a/image/imgLoader.cpp +++ b/image/imgLoader.cpp @@ -1502,23 +1502,24 @@ nsresult imgLoader::RemoveEntriesInternal(nsIPrincipal* aPrincipal, return NS_OK; } +constexpr auto AllCORSModes() { + return MakeInclusiveEnumeratedRange(kFirstCORSMode, kLastCORSMode); +} + NS_IMETHODIMP imgLoader::RemoveEntry(nsIURI* aURI, Document* aDoc) { - if (aURI) { - OriginAttributes attrs; - if (aDoc) { - nsCOMPtr principal = aDoc->NodePrincipal(); - if (principal) { - attrs = principal->OriginAttributesRef(); - } - } - - ImageCacheKey key(aURI, attrs, aDoc); - if (RemoveFromCache(key)) { - return NS_OK; - } + if (!aURI) { + return NS_OK; } - return NS_ERROR_NOT_AVAILABLE; + OriginAttributes attrs; + if (aDoc) { + attrs = aDoc->NodePrincipal()->OriginAttributesRef(); + } + for (auto corsMode : AllCORSModes()) { + ImageCacheKey key(aURI, corsMode, attrs, aDoc); + RemoveFromCache(key); + } + return NS_OK; } NS_IMETHODIMP @@ -1534,20 +1535,22 @@ imgLoader::FindEntryProperties(nsIURI* uri, Document* aDoc, } } - ImageCacheKey key(uri, attrs, aDoc); - RefPtr entry; - if (mCache.Get(key, getter_AddRefs(entry)) && entry) { + for (auto corsMode : AllCORSModes()) { + ImageCacheKey key(uri, corsMode, attrs, aDoc); + RefPtr entry; + if (!mCache.Get(key, getter_AddRefs(entry)) || !entry) { + continue; + } if (mCacheTracker && entry->HasNoProxies()) { mCacheTracker->MarkUsed(entry); } - RefPtr request = entry->GetRequest(); if (request) { nsCOMPtr properties = request->Properties(); properties.forget(_retval); + return NS_OK; } } - return NS_OK; } @@ -2228,8 +2231,8 @@ static void MakeRequestStaticIfNeeded( bool imgLoader::IsImageAvailable(nsIURI* aURI, nsIPrincipal* aTriggeringPrincipal, CORSMode aCORSMode, Document* aDocument) { - ImageCacheKey key(aURI, aTriggeringPrincipal->OriginAttributesRef(), - aDocument); + ImageCacheKey key(aURI, aCORSMode, + aTriggeringPrincipal->OriginAttributesRef(), aDocument); RefPtr entry; if (!mCache.Get(key, getter_AddRefs(entry)) || !entry) { return false; @@ -2406,7 +2409,7 @@ nsresult imgLoader::LoadImage( if (aTriggeringPrincipal) { attrs = aTriggeringPrincipal->OriginAttributesRef(); } - ImageCacheKey key(aURI, attrs, aLoadingDocument); + ImageCacheKey key(aURI, corsmode, attrs, aLoadingDocument); if (mCache.Get(key, getter_AddRefs(entry)) && entry) { bool newChannelCreated = false; if (ValidateEntry(entry, aURI, aInitialDocumentURI, aReferrerInfo, @@ -2643,7 +2646,9 @@ nsresult imgLoader::LoadImageWithChannel(nsIChannel* channel, OriginAttributes attrs = loadInfo->GetOriginAttributes(); - ImageCacheKey key(uri, attrs, aLoadingDocument); + // TODO: Get a meaningful cors mode from the caller probably? + const auto corsMode = CORS_NONE; + ImageCacheKey key(uri, corsMode, attrs, aLoadingDocument); nsLoadFlags requestFlags = nsIRequest::LOAD_NORMAL; channel->GetLoadFlags(&requestFlags); @@ -2676,7 +2681,7 @@ nsresult imgLoader::LoadImageWithChannel(nsIChannel* channel, if (ValidateEntry(entry, uri, nullptr, nullptr, nullptr, aObserver, aLoadingDocument, requestFlags, policyType, false, - nullptr, nullptr, nullptr, CORS_NONE, false, 0)) { + nullptr, nullptr, nullptr, corsMode, false, 0)) { request = entry->GetRequest(); } else { nsCOMPtr cacheChan(do_QueryInterface(channel)); @@ -2753,7 +2758,8 @@ nsresult imgLoader::LoadImageWithChannel(nsIChannel* channel, // constructed above with the *current URI* and not the *original URI*. I'm // pretty sure this is a bug, and it's preventing us from ever getting a // cache hit in LoadImageWithChannel when redirects are involved. - ImageCacheKey originalURIKey(originalURI, attrs, aLoadingDocument); + ImageCacheKey originalURIKey(originalURI, corsMode, attrs, + aLoadingDocument); // Default to doing a principal check because we don't know who // started that load and whether their principal ended up being @@ -2772,7 +2778,7 @@ nsresult imgLoader::LoadImageWithChannel(nsIChannel* channel, // can set aHadInsecureRedirect to false here. rv = request->Init(originalURI, uri, /* aHadInsecureRedirect = */ false, channel, channel, entry, aLoadingDocument, nullptr, - CORS_NONE, nullptr); + corsMode, nullptr); NS_ENSURE_SUCCESS(rv, rv); RefPtr pl =