From a0321e7db6e2d23ecbc8aedc9b0a84474591750e Mon Sep 17 00:00:00 2001 From: Andrew Osmond Date: Wed, 19 Jul 2017 14:15:11 -0400 Subject: [PATCH] Bug 1359833 - Part 6. nsImageLoadingContent should not associate scripted and XPCOM observers with the same document. r=tnikkel --- dom/base/nsIImageLoadingContent.idl | 14 +- dom/base/nsImageLoadingContent.cpp | 199 +++++++++++++++++++++++++++- dom/base/nsImageLoadingContent.h | 47 ++++++- 3 files changed, 256 insertions(+), 4 deletions(-) diff --git a/dom/base/nsIImageLoadingContent.idl b/dom/base/nsIImageLoadingContent.idl index eacc4ac3a922..26b553db0195 100644 --- a/dom/base/nsIImageLoadingContent.idl +++ b/dom/base/nsIImageLoadingContent.idl @@ -81,13 +81,25 @@ interface nsIImageLoadingContent : imgINotificationObserver * * @throws NS_ERROR_OUT_OF_MEMORY */ - void addObserver(in imgINotificationObserver aObserver); + [notxpcom] nsresult addNativeObserver(in imgINotificationObserver aObserver); /** * Used to unregister an image decoder observer. * * @param aObserver the observer to unregister */ + [notxpcom] nsresult removeNativeObserver(in imgINotificationObserver aObserver); + + /** + * Same as addNativeObserver but intended for scripted observers or observers + * from another or without a document. + */ + void addObserver(in imgINotificationObserver aObserver); + + /** + * Same as removeNativeObserver but intended for scripted observers or + * observers from another or without a document. + */ void removeObserver(in imgINotificationObserver aObserver); /** diff --git a/dom/base/nsImageLoadingContent.cpp b/dom/base/nsImageLoadingContent.cpp index fa703b0116ff..33549b8718c2 100644 --- a/dom/base/nsImageLoadingContent.cpp +++ b/dom/base/nsImageLoadingContent.cpp @@ -123,6 +123,8 @@ nsImageLoadingContent::~nsImageLoadingContent() "DestroyImageLoadingContent not called"); NS_ASSERTION(!mObserverList.mObserver && !mObserverList.mNext, "Observers still registered?"); + NS_ASSERTION(mScriptedObservers.IsEmpty(), + "Scripted observers still registered?"); } /* @@ -387,7 +389,7 @@ ReplayImageStatus(imgIRequest* aRequest, imgINotificationObserver* aObserver) } NS_IMETHODIMP -nsImageLoadingContent::AddObserver(imgINotificationObserver* aObserver) +nsImageLoadingContent::AddNativeObserver(imgINotificationObserver* aObserver) { NS_ENSURE_ARG_POINTER(aObserver); @@ -416,7 +418,7 @@ nsImageLoadingContent::AddObserver(imgINotificationObserver* aObserver) } NS_IMETHODIMP -nsImageLoadingContent::RemoveObserver(imgINotificationObserver* aObserver) +nsImageLoadingContent::RemoveNativeObserver(imgINotificationObserver* aObserver) { NS_ENSURE_ARG_POINTER(aObserver); @@ -449,6 +451,157 @@ nsImageLoadingContent::RemoveObserver(imgINotificationObserver* aObserver) return NS_OK; } +NS_IMETHODIMP +nsImageLoadingContent::AddObserver(imgINotificationObserver* aObserver) +{ + NS_ENSURE_ARG_POINTER(aObserver); + + nsresult rv = NS_OK; + RefPtr currentReq; + if (mCurrentRequest) { + // Scripted observers may not belong to the same document as us, so when we + // create the imgRequestProxy, we shouldn't use any. This allows the request + // to dispatch notifications from the correct scheduler group. + rv = mCurrentRequest->Clone(aObserver, nullptr, getter_AddRefs(currentReq)); + if (NS_FAILED(rv)) { + return rv; + } + } + + RefPtr pendingReq; + if (mPendingRequest) { + // See above for why we don't use the loading document. + rv = mPendingRequest->Clone(aObserver, nullptr, getter_AddRefs(pendingReq)); + if (NS_FAILED(rv)) { + mCurrentRequest->CancelAndForgetObserver(NS_BINDING_ABORTED); + return rv; + } + } + + mScriptedObservers.AppendElement( + new ScriptedImageObserver(aObserver, Move(currentReq), Move(pendingReq))); + return rv; +} + +NS_IMETHODIMP +nsImageLoadingContent::RemoveObserver(imgINotificationObserver* aObserver) +{ + NS_ENSURE_ARG_POINTER(aObserver); + + if (NS_WARN_IF(mScriptedObservers.IsEmpty())) { + return NS_OK; + } + + RefPtr observer; + auto i = mScriptedObservers.Length(); + do { + --i; + if (mScriptedObservers[i]->mObserver == aObserver) { + observer = Move(mScriptedObservers[i]); + mScriptedObservers.RemoveElementAt(i); + break; + } + } while(i > 0); + + if (NS_WARN_IF(!observer)) { + return NS_OK; + } + + // If the cancel causes a mutation, it will be harmless, because we have + // already removed the observer from the list. + observer->CancelRequests(); + return NS_OK; +} + +void +nsImageLoadingContent::ClearScriptedRequests(int32_t aRequestType, nsresult aReason) +{ + if (MOZ_LIKELY(mScriptedObservers.IsEmpty())) { + return; + } + + nsTArray> observers(mScriptedObservers); + auto i = observers.Length(); + do { + --i; + + RefPtr req; + switch (aRequestType) { + case CURRENT_REQUEST: + req = Move(observers[i]->mCurrentRequest); + break; + case PENDING_REQUEST: + req = Move(observers[i]->mPendingRequest); + break; + default: + NS_ERROR("Unknown request type"); + return; + } + + if (req) { + req->CancelAndForgetObserver(aReason); + } + } while (i > 0); +} + +void +nsImageLoadingContent::CloneScriptedRequests(imgRequestProxy* aRequest) +{ + MOZ_ASSERT(aRequest); + + if (MOZ_LIKELY(mScriptedObservers.IsEmpty())) { + return; + } + + bool current; + if (aRequest == mCurrentRequest) { + current = true; + } else if (aRequest == mPendingRequest) { + current = false; + } else { + MOZ_ASSERT_UNREACHABLE("Unknown request type"); + return; + } + + nsTArray> observers(mScriptedObservers); + auto i = observers.Length(); + do { + --i; + + ScriptedImageObserver* observer = observers[i]; + RefPtr& req = current ? observer->mCurrentRequest + : observer->mPendingRequest; + if (NS_WARN_IF(req)) { + MOZ_ASSERT_UNREACHABLE("Should have cancelled original request"); + req->CancelAndForgetObserver(NS_BINDING_ABORTED); + req = nullptr; + } + + nsresult rv = aRequest->Clone(observer->mObserver, nullptr, getter_AddRefs(req)); + Unused << NS_WARN_IF(NS_FAILED(rv)); + } while (i > 0); +} + +void +nsImageLoadingContent::MakePendingScriptedRequestsCurrent() +{ + if (MOZ_LIKELY(mScriptedObservers.IsEmpty())) { + return; + } + + nsTArray> observers(mScriptedObservers); + auto i = observers.Length(); + do { + --i; + + ScriptedImageObserver* observer = observers[i]; + if (observer->mCurrentRequest) { + observer->mCurrentRequest->CancelAndForgetObserver(NS_BINDING_ABORTED); + } + observer->mCurrentRequest = Move(observer->mPendingRequest); + } while (i > 0); +} + already_AddRefed nsImageLoadingContent::GetRequest(int32_t aRequestType, ErrorResult& aError) @@ -633,6 +786,7 @@ nsImageLoadingContent::LoadImageWithChannel(nsIChannel* aChannel, nsresult rv = loader-> LoadImageWithChannel(aChannel, this, doc, aListener, getter_AddRefs(req)); if (NS_SUCCEEDED(rv)) { + CloneScriptedRequests(req); TrackImage(req); ResetAnimationIfNeeded(); return NS_OK; @@ -910,6 +1064,7 @@ nsImageLoadingContent::LoadImage(nsIURI* aNewURI, aDocument->ForgetImagePreload(aNewURI); if (NS_SUCCEEDED(rv)) { + CloneScriptedRequests(req); TrackImage(req); ResetAnimationIfNeeded(); @@ -1085,6 +1240,7 @@ nsImageLoadingContent::UseAsPrimaryRequest(imgRequestProxy* aRequest, RefPtr& req = PrepareNextRequest(aImageLoadType); nsresult rv = aRequest->SyncClone(this, GetOurOwnerDoc(), getter_AddRefs(req)); if (NS_SUCCEEDED(rv)) { + CloneScriptedRequests(req); TrackImage(req); } else { MOZ_ASSERT(!req, "Shouldn't have non-null request here"); @@ -1334,6 +1490,7 @@ nsImageLoadingContent::MakePendingRequestCurrent() : eImageLoadType_Normal; PrepareCurrentRequest(loadType) = mPendingRequest; + MakePendingScriptedRequestsCurrent(); mPendingRequest = nullptr; mCurrentRequestFlags = mPendingRequestFlags; mPendingRequestFlags = 0; @@ -1363,6 +1520,7 @@ nsImageLoadingContent::ClearCurrentRequest(nsresult aReason, // Clean up the request. UntrackImage(mCurrentRequest, aNonvisibleAction); + ClearScriptedRequests(CURRENT_REQUEST, aReason); mCurrentRequest->CancelAndForgetObserver(aReason); mCurrentRequest = nullptr; mCurrentRequestFlags = 0; @@ -1381,6 +1539,7 @@ nsImageLoadingContent::ClearPendingRequest(nsresult aReason, &mPendingRequestRegistered); UntrackImage(mPendingRequest, aNonvisibleAction); + ClearScriptedRequests(PENDING_REQUEST, aReason); mPendingRequest->CancelAndForgetObserver(aReason); mPendingRequest = nullptr; mPendingRequestFlags = 0; @@ -1568,8 +1727,12 @@ nsImageLoadingContent::UntrackImage(imgIRequest* aImage, void nsImageLoadingContent::CreateStaticImageClone(nsImageLoadingContent* aDest) const { + aDest->ClearScriptedRequests(CURRENT_REQUEST, NS_BINDING_ABORTED); aDest->mCurrentRequest = nsContentUtils::GetStaticRequest(aDest->GetOurOwnerDoc(), mCurrentRequest); + if (aDest->mCurrentRequest) { + aDest->CloneScriptedRequests(aDest->mCurrentRequest); + } aDest->TrackImage(aDest->mCurrentRequest); aDest->mForcedImageState = mForcedImageState; aDest->mImageBlockingStatus = mImageBlockingStatus; @@ -1601,6 +1764,38 @@ nsImageLoadingContent::ImageObserver::~ImageObserver() NS_CONTENT_DELETE_LIST_MEMBER(ImageObserver, this, mNext); } +nsImageLoadingContent::ScriptedImageObserver::ScriptedImageObserver(imgINotificationObserver* aObserver, + RefPtr&& aCurrentRequest, + RefPtr&& aPendingRequest) + : mObserver(aObserver) + , mCurrentRequest(aCurrentRequest) + , mPendingRequest(aPendingRequest) +{ } + +nsImageLoadingContent::ScriptedImageObserver::~ScriptedImageObserver() +{ + // We should have cancelled any requests before getting released. + DebugOnly cancel = CancelRequests(); + MOZ_ASSERT(!cancel, "Still have requests in ~ScriptedImageObserver!"); +} + +bool +nsImageLoadingContent::ScriptedImageObserver::CancelRequests() +{ + bool cancelled = false; + if (mCurrentRequest) { + mCurrentRequest->CancelAndForgetObserver(NS_BINDING_ABORTED); + mCurrentRequest = nullptr; + cancelled = true; + } + if (mPendingRequest) { + mPendingRequest->CancelAndForgetObserver(NS_BINDING_ABORTED); + mPendingRequest = nullptr; + cancelled = true; + } + return cancelled; +} + // Only HTMLInputElement.h overrides this for tags // all other subclasses use this one, i.e. ignore referrer attributes mozilla::net::ReferrerPolicy diff --git a/dom/base/nsImageLoadingContent.h b/dom/base/nsImageLoadingContent.h index 3a9ae2f1ea8a..db110e2dee95 100644 --- a/dom/base/nsImageLoadingContent.h +++ b/dom/base/nsImageLoadingContent.h @@ -225,7 +225,7 @@ protected: private: /** - * Struct used to manage the image observers. + * Struct used to manage the native image observers. */ struct ImageObserver { explicit ImageObserver(imgINotificationObserver* aObserver); @@ -235,6 +235,26 @@ private: ImageObserver* mNext; }; + /** + * Struct used to manage the scripted/XPCOM image observers. + */ + class ScriptedImageObserver final { + public: + NS_INLINE_DECL_REFCOUNTING(ScriptedImageObserver) + + ScriptedImageObserver(imgINotificationObserver* aObserver, + RefPtr&& aCurrentRequest, + RefPtr&& aPendingRequest); + bool CancelRequests(); + + nsCOMPtr mObserver; + RefPtr mCurrentRequest; + RefPtr mPendingRequest; + + private: + ~ScriptedImageObserver(); + }; + /** * Struct to report state changes */ @@ -402,6 +422,24 @@ protected: nsCOMPtr mCurrentURI; private: + /** + * Clones the given "current" or "pending" request for each scripted observer. + */ + void CloneScriptedRequests(imgRequestProxy* aRequest); + + /** + * Cancels and nulls-out the "current" or "pending" requests if they exist + * for each scripted observer. + */ + void ClearScriptedRequests(int32_t aRequestType, nsresult aReason); + + /** + * Moves the "pending" request into the "current" request for each scripted + * observer. If there is an existing "current" request, it will cancel it + * first. + */ + void MakePendingScriptedRequestsCurrent(); + /** * Typically we will have only one observer (our frame in the screen * prescontext), so we want to only make space for one and to @@ -412,6 +450,13 @@ private: */ ImageObserver mObserverList; + /** + * Typically we will have no scripted observers, as this is only used by + * chrome, legacy extensions, and some mochitests. An empty array reserves + * minimal memory. + */ + nsTArray> mScriptedObservers; + /** * When mIsImageStateForced is true, this holds the ImageState that we'll * return in ImageState().