From 9e39b3b02fa18ba43b04eb3872ef4fdaadbbb658 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emilio=20Cobos=20=C3=81lvarez?= Date: Sun, 11 Aug 2024 15:59:48 +0000 Subject: [PATCH] Bug 1912115 - HTMLImageElement shouldn't be considered broken once a load task is ongoing. r=smaug Clean-up some legacy nsImageLoadingContent state tracking while at it: * mLoading was not used to set ElementState anymore, and was only checked on the "loading" attribute handling, in what seems to be a poor man's way of preventing lazy loading to start once an image load is ongoing? However that should already be covered by the image load not being sync and checking mLazyLoad itself. * The depth tracking was unneeded, image notifications have been all async for ages. Move some bools into nsImageLoadingContent to pack it better. Differential Revision: https://phabricator.services.mozilla.com/D218971 --- dom/base/nsImageLoadingContent.cpp | 77 +++++-------------- dom/base/nsImageLoadingContent.h | 55 +++---------- dom/html/HTMLImageElement.cpp | 53 ++++++++----- dom/html/HTMLImageElement.h | 10 +-- .../not-broken-while-load-task-scheduled.html | 31 ++++++++ 5 files changed, 97 insertions(+), 129 deletions(-) create mode 100644 testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-the-image-data/not-broken-while-load-task-scheduled.html diff --git a/dom/base/nsImageLoadingContent.cpp b/dom/base/nsImageLoadingContent.cpp index 10a0c35d854f..ba6f6300901e 100644 --- a/dom/base/nsImageLoadingContent.cpp +++ b/dom/base/nsImageLoadingContent.cpp @@ -104,14 +104,13 @@ nsImageLoadingContent::nsImageLoadingContent() mOutstandingDecodePromises(0), mRequestGeneration(0), mLoadingEnabled(true), - mLoading(false), mUseUrgentStartForChannel(false), mLazyLoading(false), - mStateChangerDepth(0), + mHasPendingLoadTask(false), + mSyncDecodingHint(false), + mInDocResponsiveContent(false), mCurrentRequestRegistered(false), - mPendingRequestRegistered(false), - mIsStartingImageLoad(false), - mSyncDecodingHint(false) { + mPendingRequestRegistered(false) { if (!nsContentUtils::GetImgLoaderForChannel(nullptr, nullptr)) { mLoadingEnabled = false; } @@ -174,12 +173,6 @@ void nsImageLoadingContent::Notify(imgIRequest* aRequest, int32_t aType, } } - if (aType == imgINotificationObserver::SIZE_AVAILABLE) { - // Have to check for state changes here, since we might have been in - // the LOADING state before. - UpdateImageState(true); - } - if (aType == imgINotificationObserver::LOAD_COMPLETE) { uint32_t reqStatus; aRequest->GetImageStatus(&reqStatus); @@ -216,7 +209,6 @@ void nsImageLoadingContent::Notify(imgIRequest* aRequest, int32_t aType, if (container) { container->PropagateUseCounters(GetOurOwnerDoc()); } - UpdateImageState(true); } } @@ -238,9 +230,6 @@ void nsImageLoadingContent::OnLoadComplete(imgIRequest* aRequest, return; } - // Our state may change. Watch it. - AutoStateChanger changer(this, true); - // If the pending request is loaded, switch to it. if (aRequest == mPendingRequest) { MakePendingRequestCurrent(); @@ -260,6 +249,7 @@ void nsImageLoadingContent::OnLoadComplete(imgIRequest* aRequest, MaybeResolveDecodePromises(); LargestContentfulPaint::MaybeProcessImageForElementTiming(mCurrentRequest, element); + UpdateImageState(true); } void nsImageLoadingContent::OnUnlockedDraw() { @@ -952,8 +942,7 @@ nsImageLoadingContent::LoadImageWithChannel(nsIChannel* aChannel, // XXX what about shouldProcess? // Our state might change. Watch it. - AutoStateChanger changer(this, true); - + auto updateStateOnExit = MakeScopeExit([&] { UpdateImageState(true); }); // Do the load. nsCOMPtr uri; aChannel->GetOriginalURI(getter_AddRefs(uri)); @@ -1026,11 +1015,6 @@ nsresult nsImageLoadingContent::LoadImage(nsIURI* aNewURI, bool aForce, nsLoadFlags aLoadFlags, Document* aDocument, nsIPrincipal* aTriggeringPrincipal) { - MOZ_ASSERT(!mIsStartingImageLoad, "some evil code is reentering LoadImage."); - if (mIsStartingImageLoad) { - return NS_OK; - } - // Pending load/error events need to be canceled in some situations. This // is not documented in the spec, but can cause site compat problems if not // done. See bug 1309461 and https://github.com/whatwg/html/issues/1872. @@ -1065,9 +1049,6 @@ nsresult nsImageLoadingContent::LoadImage(nsIURI* aNewURI, bool aForce, } } - AutoRestore guard(mIsStartingImageLoad); - mIsStartingImageLoad = true; - // Data documents, or documents from DOMParser shouldn't perform image // loading. // @@ -1099,7 +1080,7 @@ nsresult nsImageLoadingContent::LoadImage(nsIURI* aNewURI, bool aForce, } // From this point on, our image state could change. Watch it. - AutoStateChanger changer(this, aNotify); + auto updateStateOnExit = MakeScopeExit([&] { UpdateImageState(aNotify); }); // Sanity check. // @@ -1333,49 +1314,29 @@ CSSIntSize nsImageLoadingContent::GetWidthHeightForImage() { } void nsImageLoadingContent::UpdateImageState(bool aNotify) { - if (mStateChangerDepth > 0) { - // Ignore this call; we'll update our state when the outermost state changer - // is destroyed. Need this to work around the fact that some ImageLib - // stuff is actually sync and hence we can get OnStopDecode called while - // we're still under LoadImage, and OnStopDecode doesn't know anything about - // aNotify. - // XXX - This machinery should be removed after bug 521604. - return; - } - Element* thisElement = AsContent()->AsElement(); - - mLoading = false; - - Element::AutoStateChangeNotifier notifier(*thisElement, aNotify); - thisElement->RemoveStatesSilently(ElementState::BROKEN); - - // If we were blocked, we're broken, so are we if we don't have an image - // request at all or the image has errored. - if (!mCurrentRequest) { - if (!mLazyLoading) { - // In case of non-lazy loading, no current request means error, since we - // weren't disabled or suppressed - thisElement->AddStatesSilently(ElementState::BROKEN); - RejectDecodePromises(NS_ERROR_DOM_IMAGE_BROKEN); + const bool isBroken = [&] { + if (mLazyLoading || mHasPendingLoadTask) { + return false; + } + if (!mCurrentRequest) { + return true; } - } else { uint32_t currentLoadStatus; nsresult rv = mCurrentRequest->GetImageStatus(¤tLoadStatus); - if (NS_FAILED(rv) || (currentLoadStatus & imgIRequest::STATUS_ERROR)) { - thisElement->AddStatesSilently(ElementState::BROKEN); - RejectDecodePromises(NS_ERROR_DOM_IMAGE_BROKEN); - } else if (!(currentLoadStatus & imgIRequest::STATUS_SIZE_AVAILABLE)) { - mLoading = true; - } + return NS_FAILED(rv) || currentLoadStatus & imgIRequest::STATUS_ERROR; + }(); + thisElement->SetStates(ElementState::BROKEN, isBroken, aNotify); + if (isBroken) { + RejectDecodePromises(NS_ERROR_DOM_IMAGE_BROKEN); } } void nsImageLoadingContent::CancelImageRequests(bool aNotify) { RejectDecodePromises(NS_ERROR_DOM_IMAGE_INVALID_REQUEST); - AutoStateChanger changer(this, aNotify); ClearPendingRequest(NS_BINDING_ABORTED, Some(OnNonvisible::DiscardImages)); ClearCurrentRequest(NS_BINDING_ABORTED, Some(OnNonvisible::DiscardImages)); + UpdateImageState(aNotify); } Document* nsImageLoadingContent::GetOurOwnerDoc() { diff --git a/dom/base/nsImageLoadingContent.h b/dom/base/nsImageLoadingContent.h index 80d93d0aae0a..99cfc28538a3 100644 --- a/dom/base/nsImageLoadingContent.h +++ b/dom/base/nsImageLoadingContent.h @@ -330,25 +330,6 @@ class nsImageLoadingContent : public nsIImageLoadingContent { ~ScriptedImageObserver(); }; - /** - * Struct to report state changes - */ - struct AutoStateChanger { - AutoStateChanger(nsImageLoadingContent* aImageContent, bool aNotify) - : mImageContent(aImageContent), mNotify(aNotify) { - mImageContent->mStateChangerDepth++; - } - ~AutoStateChanger() { - mImageContent->mStateChangerDepth--; - mImageContent->UpdateImageState(mNotify); - } - - nsImageLoadingContent* mImageContent; - bool mNotify; - }; - - friend struct AutoStateChanger; - /** * Method to fire an event once we know what's going on with the image load. * @@ -550,15 +531,8 @@ class nsImageLoadingContent : public nsIImageLoadingContent { */ uint32_t mRequestGeneration; - bool mLoadingEnabled : 1; - protected: - /** - * The state we had the last time we checked whether we needed to notify the - * document of a state change. These are maintained by UpdateImageState. - */ - bool mLoading : 1; - + bool mLoadingEnabled : 1; /** * Flag to indicate whether the channel should be mark as urgent-start. * It should be set in *Element and passed to nsContentUtils::LoadImage. @@ -570,29 +544,20 @@ class nsImageLoadingContent : public nsIImageLoadingContent { // Represents the image is deferred loading until this element gets visible. bool mLazyLoading : 1; - private: - /* The number of nested AutoStateChangers currently tracking our state. */ - uint8_t mStateChangerDepth; + // Whether we have a pending load task scheduled (HTMLImageElement only). + bool mHasPendingLoadTask : 1; + // If true, force frames to synchronously decode images on draw. + bool mSyncDecodingHint : 1; + + // Whether we're in the doc responsive content set (HTMLImageElement only). + bool mInDocResponsiveContent : 1; + + private: // Flags to indicate whether each of the current and pending requests are // registered with the refresh driver. bool mCurrentRequestRegistered; bool mPendingRequestRegistered; - - // TODO: - // Bug 1353685: Should ServiceWorker call SetBlockedRequest? - // - // This member is used in SetBlockedRequest, if it's true, then this call is - // triggered from LoadImage. - // If this is false, it means this call is from other places like - // ServiceWorker, then we will ignore call to SetBlockedRequest for now. - // - // Also we use this variable to check if some evil code is reentering - // LoadImage. - bool mIsStartingImageLoad; - - // If true, force frames to synchronously decode images on draw. - bool mSyncDecodingHint; }; #endif // nsImageLoadingContent_h__ diff --git a/dom/html/HTMLImageElement.cpp b/dom/html/HTMLImageElement.cpp index 20b61c5f5b7c..2a42b20e7a41 100644 --- a/dom/html/HTMLImageElement.cpp +++ b/dom/html/HTMLImageElement.cpp @@ -92,7 +92,7 @@ class ImageLoadTask final : public MicroTaskRunnable { void Run(AutoSlowOperation& aAso) override { if (mElement->mPendingImageLoadTask == this) { JSCallingLocation::AutoFallback fallback(&mCallingLocation); - mElement->mPendingImageLoadTask = nullptr; + mElement->ClearImageLoadTask(); mElement->mUseUrgentStartForChannel = mUseUrgentStartForChannel; mElement->LoadSelectedImage(mAlwaysLoad); } @@ -327,7 +327,7 @@ void HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName, } bool forceReload = false; - if (aName == nsGkAtoms::loading && !mLoading) { + if (aName == nsGkAtoms::loading) { if (aValue && Loading(aValue->GetEnumValue()) == Loading::Lazy) { SetLazyLoading(); } else if (aOldValue && @@ -387,7 +387,7 @@ void HTMLImageElement::AfterSetAttr(int32_t aNameSpaceID, nsAtom* aName, // get bound to a tree. if (forceReload) { mUseUrgentStartForChannel = UserActivation::IsHandlingUserInput(); - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, aNotify); } return nsGenericHTMLElement::AfterSetAttr( @@ -427,7 +427,7 @@ void HTMLImageElement::AfterMaybeChangeAttr( mResponsiveSelector->SetDefaultSource(mSrcURI, mSrcTriggeringPrincipal); } mUseUrgentStartForChannel = UserActivation::IsHandlingUserInput(); - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, aNotify); } void HTMLImageElement::GetEventTargetParent(EventChainPreVisitor& aVisitor) { @@ -486,7 +486,7 @@ nsresult HTMLImageElement::BindToTree(BindContext& aContext, nsINode& aParent) { mInDocResponsiveContent = true; } mUseUrgentStartForChannel = UserActivation::IsHandlingUserInput(); - UpdateSourceSyncAndQueueImageTask(false); + UpdateSourceSyncAndQueueImageTask(false, /* aNotify = */ false); } return NS_OK; } @@ -570,7 +570,7 @@ void HTMLImageElement::NodeInfoChanged(Document* aOldDoc) { // per spec, // https://html.spec.whatwg.org/#reacting-to-dom-mutations, and // https://html.spec.whatwg.org/#reacting-to-environment-changes. - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, /* aNotify = */ false); } // static @@ -647,7 +647,7 @@ nsresult HTMLImageElement::CopyInnerTo(HTMLImageElement* aDest) { // doing the image load because we passed in false for aNotify. But we // really do want it to do the load, so set it up to happen once the cloning // reaches a stable state. - aDest->UpdateSourceSyncAndQueueImageTask(false); + aDest->UpdateSourceSyncAndQueueImageTask(false, /* aNotify = */ false); return NS_OK; } @@ -700,9 +700,14 @@ void HTMLImageElement::ClearForm(bool aRemoveFromForm) { mForm = nullptr; } +void HTMLImageElement::ClearImageLoadTask() { + mPendingImageLoadTask = nullptr; + mHasPendingLoadTask = false; +} + // Roughly corresponds to https://html.spec.whatwg.org/#update-the-image-data void HTMLImageElement::UpdateSourceSyncAndQueueImageTask( - bool aAlwaysLoad, const HTMLSourceElement* aSkippedSource) { + bool aAlwaysLoad, bool aNotify, const HTMLSourceElement* aSkippedSource) { // Per spec, when updating the image data or reacting to environment // changes, we always run the full selection (including selecting the source // element and the best fit image from srcset) even if it doesn't directly @@ -751,17 +756,17 @@ void HTMLImageElement::UpdateSourceSyncAndQueueImageTask( // possible instead. This prevents unsound state changes from frame // construction and such. nsContentUtils::AddScriptRunner( - NewRunnableMethod( + NewRunnableMethod( "HTMLImageElement::UpdateSourceSyncAndQueueImageTask", this, &HTMLImageElement::UpdateSourceSyncAndQueueImageTask, aAlwaysLoad, - nullptr)); + /* aNotify = */ true, nullptr)); return; } if (mLazyLoading && mSrcURI) { StopLazyLoading(StartLoad::No); } - mPendingImageLoadTask = nullptr; + ClearImageLoadTask(); LoadSelectedImage(alwaysLoad); return; } @@ -776,6 +781,9 @@ void HTMLImageElement::UpdateSourceSyncAndQueueImageTask( RefPtr task = new ImageLoadTask(this, alwaysLoad, mUseUrgentStartForChannel); mPendingImageLoadTask = task; + mHasPendingLoadTask = true; + // We might have just become non-broken. + UpdateImageState(aNotify); // The task checks this to determine if it was the last queued event, and so // earlier tasks are implicitly canceled. CycleCollectedJSContext::Get()->DispatchToMicroTask(task.forget()); @@ -842,6 +850,9 @@ void HTMLImageElement::LoadSelectedImage(bool aAlwaysLoad) { // to nsImageFrame::NotifyNewCurrentRequest, which takes care of that for // us. SetDensity(currentDensity); + // If we're (re-)loading a broken image, we might have just become broken + // again. + UpdateImageState(true); return; } @@ -891,7 +902,7 @@ void HTMLImageElement::PictureSourceSrcsetChanged(nsIContent* aSourceNode, // This always triggers the image update steps per the spec, even if we are // not using this source. - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, aNotify); } void HTMLImageElement::PictureSourceSizesChanged(nsIContent* aSourceNode, @@ -911,7 +922,7 @@ void HTMLImageElement::PictureSourceSizesChanged(nsIContent* aSourceNode, // This always triggers the image update steps per the spec, even if // we are not using this source. - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, aNotify); } void HTMLImageElement::PictureSourceMediaOrTypeChanged(nsIContent* aSourceNode, @@ -921,7 +932,7 @@ void HTMLImageElement::PictureSourceMediaOrTypeChanged(nsIContent* aSourceNode, // This always triggers the image update steps per the spec, even if // we are not switching to/from this source - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, aNotify); } void HTMLImageElement::PictureSourceDimensionChanged( @@ -943,14 +954,14 @@ void HTMLImageElement::PictureSourceAdded(bool aNotify, MOZ_ASSERT(!aSourceNode || IsPreviousSibling(aSourceNode, this), "Should not be getting notifications for non-previous-siblings"); - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, aNotify); } void HTMLImageElement::PictureSourceRemoved(bool aNotify, HTMLSourceElement* aSourceNode) { MOZ_ASSERT(!aSourceNode || IsPreviousSibling(aSourceNode, this), "Should not be getting notifications for non-previous-siblings"); - UpdateSourceSyncAndQueueImageTask(true, aSourceNode); + UpdateSourceSyncAndQueueImageTask(true, aNotify, aSourceNode); } bool HTMLImageElement::UpdateResponsiveSource( @@ -1163,9 +1174,9 @@ bool HTMLImageElement::SelectSourceForTagWithAttrs( } void HTMLImageElement::DestroyContent() { - // Clear mPendingImageLoadTask to avoid running LoadSelectedImage() after - // getting destroyed. - mPendingImageLoadTask = nullptr; + // Clear the load task to avoid running LoadSelectedImage() after getting + // destroyed. + ClearImageLoadTask(); mResponsiveSelector = nullptr; @@ -1174,7 +1185,7 @@ void HTMLImageElement::DestroyContent() { } void HTMLImageElement::MediaFeatureValuesChanged() { - UpdateSourceSyncAndQueueImageTask(false); + UpdateSourceSyncAndQueueImageTask(false, /* aNotify = */ true); } bool HTMLImageElement::ShouldLoadImage() const { @@ -1211,7 +1222,7 @@ void HTMLImageElement::StopLazyLoading(StartLoad aStartLoad) { } if (aStartLoad == StartLoad::Yes) { - UpdateSourceSyncAndQueueImageTask(true); + UpdateSourceSyncAndQueueImageTask(true, /* aNotify = */ true); } } diff --git a/dom/html/HTMLImageElement.h b/dom/html/HTMLImageElement.h index 9bfe434e9b6b..128d1793e132 100644 --- a/dom/html/HTMLImageElement.h +++ b/dom/html/HTMLImageElement.h @@ -267,7 +267,11 @@ class HTMLImageElement final : public nsGenericHTMLElement, // Update the responsive source synchronously and queues a task to run // LoadSelectedImage pending stable state. void UpdateSourceSyncAndQueueImageTask( - bool aAlwaysLoad, const HTMLSourceElement* aSkippedSource = nullptr); + bool aAlwaysLoad, bool aNotify, + const HTMLSourceElement* aSkippedSource = nullptr); + + // Clears the current image load task. + void ClearImageLoadTask(); // True if we have a srcset attribute or a parent, regardless of if // any valid responsive sources were parsed from either. @@ -396,9 +400,6 @@ class HTMLImageElement final : public nsGenericHTMLElement, void SetResponsiveSelector(RefPtr&& aSource); void SetDensity(double aDensity); - // Queue an image load task (via microtask). - void QueueImageLoadTask(bool aAlwaysLoad); - RefPtr mPendingImageLoadTask; nsCOMPtr mSrcURI; nsCOMPtr mSrcTriggeringPrincipal; @@ -408,7 +409,6 @@ class HTMLImageElement final : public nsGenericHTMLElement, nsCOMPtr mLastSelectedSource; // Last pixel density that was selected. double mCurrentDensity = 1.0; - bool mInDocResponsiveContent = false; }; } // namespace dom diff --git a/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-the-image-data/not-broken-while-load-task-scheduled.html b/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-the-image-data/not-broken-while-load-task-scheduled.html new file mode 100644 index 000000000000..82b860a4700c --- /dev/null +++ b/testing/web-platform/tests/html/semantics/embedded-content/the-img-element/update-the-image-data/not-broken-while-load-task-scheduled.html @@ -0,0 +1,31 @@ + +Image shouldn't be broken while load task is scheduled. + + + +