diff --git a/image/ProgressTracker.cpp b/image/ProgressTracker.cpp index 0805d22e92eb..d34e96d6ec5a 100644 --- a/image/ProgressTracker.cpp +++ b/image/ProgressTracker.cpp @@ -171,6 +171,11 @@ ProgressTracker::Notify(IProgressObserver* aObserver) { MOZ_ASSERT(NS_IsMainThread()); + if (aObserver->NotificationsDeferred()) { + // There is a pending notification, or the observer isn't ready yet. + return; + } + if (MOZ_LOG_TEST(gImgLog, LogLevel::Debug)) { RefPtr image = GetImage(); if (image && image->GetURI()) { @@ -241,6 +246,11 @@ ProgressTracker::NotifyCurrentState(IProgressObserver* aObserver) { MOZ_ASSERT(NS_IsMainThread()); + if (aObserver->NotificationsDeferred()) { + // There is a pending notification, or the observer isn't ready yet. + return; + } + if (MOZ_LOG_TEST(gImgLog, LogLevel::Debug)) { RefPtr image = GetImage(); nsAutoCString spec; diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp index d766a6706a63..4632974ef609 100644 --- a/image/imgLoader.cpp +++ b/image/imgLoader.cpp @@ -1768,7 +1768,7 @@ imgLoader::ValidateRequestWithNewChannel(imgRequest* request, // In the mean time, we must defer notifications because we are added to // the imgRequest's proxy list, and we can get extra notifications // resulting from methods such as StartDecoding(). See bug 579122. - proxy->SetNotificationsDeferred(true); + proxy->MarkValidating(); // Attach the proxy without notifying request->GetValidator()->AddProxy(proxy); @@ -1834,7 +1834,7 @@ imgLoader::ValidateRequestWithNewChannel(imgRequest* request, // In the mean time, we must defer notifications because we are added to // the imgRequest's proxy list, and we can get extra notifications // resulting from methods such as StartDecoding(). See bug 579122. - req->SetNotificationsDeferred(true); + req->MarkValidating(); // Add the proxy without notifying hvc->AddProxy(req); @@ -2936,13 +2936,45 @@ imgCacheValidator::AddProxy(imgRequestProxy* aProxy) // the network. aProxy->AddToLoadGroup(); - mProxies.AppendObject(aProxy); + mProxies.AppendElement(aProxy); } void imgCacheValidator::RemoveProxy(imgRequestProxy* aProxy) { - mProxies.RemoveObject(aProxy); + mProxies.RemoveElement(aProxy); +} + +void +imgCacheValidator::UpdateProxies() +{ + // We have finished validating the request, so we can safely take ownership + // of the proxy list. imgRequestProxy::SyncNotifyListener can mutate the list + // if imgRequestProxy::CancelAndForgetObserver is called by its owner. Note + // that any potential notifications should still be suppressed in + // imgRequestProxy::ChangeOwner because we haven't cleared the validating + // flag yet, and thus they will remain deferred. + AutoTArray, 4> proxies(Move(mProxies)); + + for (auto& proxy : proxies) { + // First update the state of all proxies before notifying any of them + // to ensure a consistent state (e.g. in case the notification causes + // other proxies to be touched indirectly.) + MOZ_ASSERT(proxy->IsValidating()); + MOZ_ASSERT(proxy->NotificationsDeferred(), + "Proxies waiting on cache validation should be " + "deferring notifications!"); + if (mNewRequest) { + proxy->ChangeOwner(mNewRequest); + } + proxy->ClearValidating(); + } + + for (auto& proxy : proxies) { + // Notify synchronously, because we're already in OnStartRequest, an + // asynchronously-called function. + proxy->SyncNotifyListener(); + } } /** nsIRequestObserver methods **/ @@ -2982,25 +3014,11 @@ imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt) } if (isFromCache && sameURI) { - uint32_t count = mProxies.Count(); - for (int32_t i = count-1; i>=0; i--) { - imgRequestProxy* proxy = static_cast(mProxies[i]); - - // Proxies waiting on cache validation should be deferring - // notifications. Undefer them. - MOZ_ASSERT(proxy->NotificationsDeferred(), - "Proxies waiting on cache validation should be " - "deferring notifications!"); - proxy->SetNotificationsDeferred(false); - - // Notify synchronously, because we're already in OnStartRequest, an - // asynchronously-called function. - proxy->SyncNotifyListener(); - } - // We don't need to load this any more. aRequest->Cancel(NS_BINDING_ABORTED); + // Clear the validator before updating the proxies. The notifications may + // clone an existing request, and its state could be inconsistent. mRequest->SetLoadId(context); mRequest->SetValidator(nullptr); @@ -3009,6 +3027,7 @@ imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt) mNewRequest = nullptr; mNewEntry = nullptr; + UpdateProxies(); return NS_OK; } } @@ -3035,6 +3054,8 @@ imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt) // Doom the old request's cache entry mRequest->RemoveFromCache(); + // Clear the validator before updating the proxies. The notifications may + // clone an existing request, and its state could be inconsistent. mRequest->SetValidator(nullptr); mRequest = nullptr; @@ -3055,17 +3076,7 @@ imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt) // changes the caching behaviour for imgRequests. mImgLoader->PutIntoCache(mNewRequest->CacheKey(), mNewEntry); - uint32_t count = mProxies.Count(); - for (int32_t i = count-1; i>=0; i--) { - imgRequestProxy* proxy = static_cast(mProxies[i]); - proxy->ChangeOwner(mNewRequest); - - // Notify synchronously, because we're already in OnStartRequest, an - // asynchronously-called function. - proxy->SetNotificationsDeferred(false); - proxy->SyncNotifyListener(); - } - + UpdateProxies(); mNewRequest = nullptr; mNewEntry = nullptr; diff --git a/image/imgLoader.h b/image/imgLoader.h index 24130b25e249..b0b4d84ae46f 100644 --- a/image/imgLoader.h +++ b/image/imgLoader.h @@ -568,6 +568,7 @@ public: NS_DECL_NSIASYNCVERIFYREDIRECTCALLBACK private: + void UpdateProxies(); virtual ~imgCacheValidator(); nsCOMPtr mDestListener; @@ -576,7 +577,7 @@ private: nsCOMPtr mRedirectChannel; RefPtr mRequest; - nsCOMArray mProxies; + AutoTArray, 4> mProxies; RefPtr mNewRequest; RefPtr mNewEntry; diff --git a/image/imgRequestProxy.cpp b/image/imgRequestProxy.cpp index 6edda1d18582..f0c278955872 100644 --- a/image/imgRequestProxy.cpp +++ b/image/imgRequestProxy.cpp @@ -121,6 +121,7 @@ imgRequestProxy::imgRequestProxy() : mListenerIsStrongRef(false), mDecodeRequested(false), mDeferNotifications(false), + mValidating(false), mHadListener(false), mHadDispatch(false) { @@ -163,15 +164,13 @@ imgRequestProxy::~imgRequestProxy() // above assert. NullOutListener(); - if (GetOwner()) { - /* Call RemoveProxy with a successful status. This will keep the - channel, if still downloading data, from being canceled if 'this' is - the last observer. This allows the image to continue to download and - be cached even if no one is using it currently. - */ - mCanceled = true; - GetOwner()->RemoveProxy(this, NS_OK); - } + /* Call RemoveProxy with a successful status. This will keep the + channel, if still downloading data, from being canceled if 'this' is + the last observer. This allows the image to continue to download and + be cached even if no one is using it currently. + */ + mCanceled = true; + RemoveFromOwner(NS_OK); RemoveFromLoadGroup(); LOG_FUNC(gImgLog, "imgRequestProxy::~imgRequestProxy"); @@ -237,6 +236,7 @@ imgRequestProxy::ChangeOwner(imgRequest* aNewOwner) GetOwner()->RemoveProxy(this, NS_OK); mBehaviour->SetOwner(aNewOwner); + MOZ_ASSERT(!GetValidator(), "New owner cannot be validating!"); // If we were locked, apply the locks here for (uint32_t i = 0; i < oldLockCount; i++) { @@ -251,14 +251,29 @@ imgRequestProxy::ChangeOwner(imgRequest* aNewOwner) } AddToOwner(nullptr); + return NS_OK; +} + +void +imgRequestProxy::MarkValidating() +{ + MOZ_ASSERT(GetValidator()); + mValidating = true; +} + +void +imgRequestProxy::ClearValidating() +{ + MOZ_ASSERT(mValidating); + MOZ_ASSERT(!GetValidator()); + mValidating = false; // If we'd previously requested a synchronous decode, request a decode on the // new image. if (mDecodeRequested) { + mDecodeRequested = false; StartDecoding(imgIContainer::FLAG_NONE); } - - return NS_OK; } bool @@ -351,11 +366,28 @@ imgRequestProxy::AddToOwner(nsIDocument* aLoadingDocument) mEventTarget = do_GetMainThread(); } - if (!GetOwner()) { + imgRequest* owner = GetOwner(); + if (!owner) { return; } - GetOwner()->AddProxy(this); + owner->AddProxy(this); +} + +void +imgRequestProxy::RemoveFromOwner(nsresult aStatus) +{ + imgRequest* owner = GetOwner(); + if (owner) { + if (mValidating) { + imgCacheValidator* validator = owner->GetValidator(); + MOZ_ASSERT(validator); + validator->RemoveProxy(this); + mValidating = false; + } + + owner->RemoveProxy(this, aStatus); + } } void @@ -494,10 +526,7 @@ imgRequestProxy::Cancel(nsresult status) void imgRequestProxy::DoCancel(nsresult status) { - if (GetOwner()) { - GetOwner()->RemoveProxy(this, status); - } - + RemoveFromOwner(status); RemoveFromLoadGroup(); NullOutListener(); } @@ -519,17 +548,7 @@ imgRequestProxy::CancelAndForgetObserver(nsresult aStatus) mCanceled = true; mForceDispatchLoadGroup = true; - - imgRequest* owner = GetOwner(); - if (owner) { - imgCacheValidator* validator = owner->GetValidator(); - if (validator) { - validator->RemoveProxy(this); - } - - owner->RemoveProxy(this, aStatus); - } - + RemoveFromOwner(aStatus); RemoveFromLoadGroup(); mForceDispatchLoadGroup = false; @@ -859,15 +878,16 @@ imgRequestProxy::PerformClone(imgINotificationObserver* aObserver, // surprised. NS_ADDREF(*aClone = clone); - if (GetOwner() && GetOwner()->GetValidator()) { + imgCacheValidator* validator = GetValidator(); + if (validator) { // Note that if we have a validator, we don't want to issue notifications at // here because we want to defer until that completes. AddProxy will add us // to the load group; we cannot avoid that in this case, because we don't // know when the validation will complete, and if it will cause us to // discard our cached state anyways. We are probably already blocked by the // original LoadImage(WithChannel) request in any event. - clone->SetNotificationsDeferred(true); - GetOwner()->GetValidator()->AddProxy(clone); + clone->MarkValidating(); + validator->AddProxy(clone); } else { // We only want to add the request to the load group of the owning document // if it is still in progress. Some callers cannot handle a supurious load @@ -1270,6 +1290,16 @@ imgRequestProxy::GetOwner() const return mBehaviour->GetOwner(); } +imgCacheValidator* +imgRequestProxy::GetValidator() const +{ + imgRequest* owner = GetOwner(); + if (!owner) { + return nullptr; + } + return owner->GetValidator(); +} + ////////////////// imgRequestProxyStatic methods class StaticBehaviour : public ProxyBehaviour diff --git a/image/imgRequestProxy.h b/image/imgRequestProxy.h index 5c0d342d1387..287a72239c2b 100644 --- a/image/imgRequestProxy.h +++ b/image/imgRequestProxy.h @@ -30,6 +30,7 @@ {0x8f, 0x65, 0x9c, 0x46, 0x2e, 0xe2, 0xbc, 0x95} \ } +class imgCacheValidator; class imgINotificationObserver; class imgStatusNotifyRunnable; class ProxyBehaviour; @@ -111,15 +112,21 @@ public: virtual void SetHasImage() override; // Whether we want notifications from ProgressTracker to be deferred until - // an event it has scheduled has been fired. + // an event it has scheduled has been fired and/or validation is complete. virtual bool NotificationsDeferred() const override { - return mDeferNotifications; + return IsValidating() || mDeferNotifications; } virtual void SetNotificationsDeferred(bool aDeferNotifications) override { mDeferNotifications = aDeferNotifications; } + bool IsValidating() const + { + return mValidating; + } + void MarkValidating(); + void ClearValidating(); bool IsOnEventTarget() const; already_AddRefed GetEventTarget() const override; @@ -194,6 +201,7 @@ protected: already_AddRefed GetImage() const; bool HasImage() const; imgRequest* GetOwner() const; + imgCacheValidator* GetValidator() const; nsresult PerformClone(imgINotificationObserver* aObserver, nsIDocument* aLoadingDocument, @@ -212,6 +220,7 @@ private: friend class imgCacheValidator; void AddToOwner(nsIDocument* aLoadingDocument); + void RemoveFromOwner(nsresult aStatus); nsresult DispatchWithTargetIfAvailable(already_AddRefed aEvent); void DispatchWithTarget(already_AddRefed aEvent); @@ -242,6 +251,7 @@ private: // Whether we want to defer our notifications by the non-virtual Observer // interfaces as image loads proceed. bool mDeferNotifications : 1; + bool mValidating : 1; bool mHadListener : 1; bool mHadDispatch : 1; };