REGRESSION (252960@main, WPT resync): [ macOS Debug ] imported/w3c/we…

…b-platform-tests/html/semantics/document-metadata/the-style-element/style-load-after-mutate.html is a flaky failure

https://bugs.webkit.org/show_bug.cgi?id=244014
rdar://98757197

Reviewed by Ryosuke Niwa.

The test in question starts out with an empty <style> and then adds an @import
to it. However, the URL of the @import points to a missing resource. As a
result, we first schedule the dispatch of a load event, then an error event.
The test makes sure that a load event is fired and times out if none gets fired.

The issue was that HTMLStyleElement was using the same EventSender for the load
and error events. However, EventSender was storing senders in its queue, not
events. The state about whether to send a load or an error event was stored on
the HTMLStyleElement instance instead.

This meant that we could schedule a load event and then an error event right
after one another. The request to send an error event would override the state
on the HTMLStyleElement and we would end up sending 2 error events, instead of
a load event followed by an error event.

To address the issue, I updated EventSender to store the event type in its
queue, in addition to the sender. We would have used two separate EventSenders
like HTMLLinkElement was doing. However, I worry that ordering between load
and error events may be incorrect if we use separate queues.

* LayoutTests/platform/mac/TestExpectations:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::implicitClose):
* Source/WebCore/dom/EventSender.h:
(WebCore::EventSender::hasPendingEvents const):
(WebCore::Counter>::EventSender):
(WebCore::Counter>::dispatchEventSoon):
(WebCore::Counter>::cancelEvent):
(WebCore::Counter>::dispatchPendingEvents):
(WebCore::EventSender::eventType const): Deleted.
* Source/WebCore/html/HTMLImageElement.cpp:
(WebCore::HTMLImageElement::didAttachRenderers):
* Source/WebCore/html/HTMLLinkElement.cpp:
(WebCore::linkLoadEventSender):
(WebCore::HTMLLinkElement::~HTMLLinkElement):
(WebCore::HTMLLinkElement::linkLoaded):
(WebCore::HTMLLinkElement::linkLoadingErrored):
(WebCore::HTMLLinkElement::dispatchPendingEvent):
(WebCore::HTMLLinkElement::notifyLoadedSheetAndAllCriticalSubresources):
(WebCore::linkErrorEventSender): Deleted.
* Source/WebCore/html/HTMLLinkElement.h:
* Source/WebCore/html/HTMLStyleElement.cpp:
(WebCore::styleLoadEventSender):
(WebCore::HTMLStyleElement::dispatchPendingEvent):
(WebCore::HTMLStyleElement::notifyLoadedSheetAndAllCriticalSubresources):
* Source/WebCore/html/HTMLStyleElement.h:
* Source/WebCore/html/ImageInputType.cpp:
(WebCore::ImageInputType::attach):
* Source/WebCore/loader/ImageLoader.cpp:
(WebCore::loadEventSender):
(WebCore::ImageLoader::ImageLoader):
(WebCore::ImageLoader::~ImageLoader):
(WebCore::ImageLoader::clearImageWithoutConsideringPendingLoadEvent):
(WebCore::ImageLoader::updateFromElement):
(WebCore::ImageLoader::didUpdateCachedImage):
(WebCore::ImageLoader::notifyFinished):
(WebCore::ImageLoader::dispatchPendingEvent):
(WebCore::ImageLoader::dispatchPendingErrorEvent):
(WebCore::beforeLoadEventSender): Deleted.
(WebCore::errorEventSender): Deleted.
(WebCore::ImageLoader::dispatchPendingBeforeLoadEvent): Deleted.
(WebCore::ImageLoader::dispatchPendingBeforeLoadEvents): Deleted.
(WebCore::ImageLoader::dispatchPendingErrorEvents): Deleted.
* Source/WebCore/loader/ImageLoader.h:
(WebCore::ImageLoader::hasPendingBeforeLoadEvent const): Deleted.
* Source/WebCore/svg/animation/SVGSMILElement.cpp:
(WebCore::smilEventSender):
(WebCore::SVGSMILElement::~SVGSMILElement):
(WebCore::SVGSMILElement::progress):
(WebCore::SVGSMILElement::dispatchPendingEvent):
(WebCore::smilBeginEventSender): Deleted.
(WebCore::smilEndEventSender): Deleted.
* Source/WebCore/svg/animation/SVGSMILElement.h:
* Source/WebCore/xml/parser/XMLDocumentParser.cpp:
(WebCore::XMLDocumentParser::append):

Canonical link: https://commits.webkit.org/256535@main

LayoutTests/platform/mac/TestExpectations

@@ -2344,8 +2344,6 @@ webkit.org/b/246356 imported/w3c/web-platform-tests/html/semantics/embedded-cont
 # The following test only fail on Mac EWS bots.
 webkit.org/b/244012 imported/w3c/web-platform-tests/css/css-transforms/transform-3d-rotateY-stair-below-001.xht [ Pass ImageOnlyFailure ]
 
-webkit.org/b/244014 [ Debug ] imported/w3c/web-platform-tests/html/semantics/document-metadata/the-style-element/style-load-after-mutate.html  [ Pass Failure ]
-
 # These tests are flaky due to "Blocked access to external URL" messages because we don't support WPT domains.
 imported/w3c/web-platform-tests/feature-policy/feature-policy-frame-policy-allowed-for-all.https.sub.html [ DumpJSConsoleLogInStdErr Failure Pass ]
 imported/w3c/web-platform-tests/feature-policy/feature-policy-frame-policy-allowed-for-self.https.sub.html [ DumpJSConsoleLogInStdErr Failure Pass ]

Source/WebCore/dom/Document.cpp

@@ -3282,9 +3282,7 @@ void Document::implicitClose()
         // For now, only do this when there is a Frame, otherwise this could cause JS reentrancy
         // below SVG font parsing, for example. <https://webkit.org/b/136269>
         if (auto* currentPage = page()) {
-            ImageLoader::dispatchPendingBeforeLoadEvents(currentPage);
             ImageLoader::dispatchPendingLoadEvents(currentPage);
-            ImageLoader::dispatchPendingErrorEvents(currentPage);
             HTMLLinkElement::dispatchPendingLoadEvents(currentPage);
             HTMLStyleElement::dispatchPendingLoadEvents(currentPage);
         }

Source/WebCore/dom/EventSender.h

@@ -34,60 +34,77 @@ namespace WebCore {
 class Page;
 class WeakPtrImplWithEventTargetData;
 
-template<typename T, typename Counter> class EventSender {
+template<typename T, typename WeakPtrImpl> class EventSender {
     WTF_MAKE_NONCOPYABLE(EventSender); WTF_MAKE_FAST_ALLOCATED;
 public:
-    explicit EventSender(const AtomString& eventType);
+    EventSender();
 
-    const AtomString& eventType() const { return m_eventType; }
-    void dispatchEventSoon(T&);
+    void dispatchEventSoon(T&, const AtomString& eventType);
     void cancelEvent(T&);
+    void cancelEvent(T&, const AtomString& eventType);
     void dispatchPendingEvents(Page*);
 
 #if ASSERT_ENABLED
     bool hasPendingEvents(T& sender) const
     {
-        return m_dispatchSoonList.find(&sender) != notFound || m_dispatchingList.find(&sender) != notFound;
+        return m_dispatchSoonList.containsIf([&](auto& task) { return task.sender == &sender; })
+            || m_dispatchingList.containsIf([&](auto& task) { return task.sender == &sender; });
     }
 #endif
 
 private:
     void timerFired() { dispatchPendingEvents(nullptr); }
 
-    AtomString m_eventType;
     Timer m_timer;
-    Vector<WeakPtr<T, Counter>> m_dispatchSoonList;
-    Vector<WeakPtr<T, Counter>> m_dispatchingList;
+    struct DispatchTask {
+        WeakPtr<T, WeakPtrImpl> sender;
+        AtomString eventType;
+    };
+    Vector<DispatchTask> m_dispatchSoonList;
+    Vector<DispatchTask> m_dispatchingList;
 };
 
-template<typename T, typename Counter> EventSender<T, Counter>::EventSender(const AtomString& eventType)
-    : m_eventType(eventType)
-    , m_timer(*this, &EventSender::timerFired)
+template<typename T, typename WeakPtrImpl> EventSender<T, WeakPtrImpl>::EventSender()
+    : m_timer(*this, &EventSender::timerFired)
 {
 }
 
-template<typename T, typename Counter> void EventSender<T, Counter>::dispatchEventSoon(T& sender)
+template<typename T, typename WeakPtrImpl> void EventSender<T, WeakPtrImpl>::dispatchEventSoon(T& sender, const AtomString& eventType)
 {
-    m_dispatchSoonList.append(sender);
+    m_dispatchSoonList.append(DispatchTask { sender, eventType });
     if (!m_timer.isActive())
         m_timer.startOneShot(0_s);
 }
 
-template<typename T, typename Counter> void EventSender<T, Counter>::cancelEvent(T& sender)
+template<typename T, typename WeakPtrImpl> void EventSender<T, WeakPtrImpl>::cancelEvent(T& senderToCancel)
 {
     // Remove instances of this sender from both lists.
     // Use loops because we allow multiple instances to get into the lists.
-    for (auto& event : m_dispatchSoonList) {
-        if (event == &sender)
-            event = nullptr;
+    for (auto& [sender, eventType] : m_dispatchSoonList) {
+        if (sender == &senderToCancel)
+            sender = nullptr;
     }
-    for (auto& event : m_dispatchingList) {
-        if (event == &sender)
-            event = nullptr;
+    for (auto& [sender, eventType] : m_dispatchingList) {
+        if (sender == &senderToCancel)
+            sender = nullptr;
     }
 }
 
-template<typename T, typename Counter> void EventSender<T, Counter>::dispatchPendingEvents(Page* page)
+template<typename T, typename WeakPtrImpl> void EventSender<T, WeakPtrImpl>::cancelEvent(T& senderToCancel, const AtomString& eventTypeToCancel)
+{
+    // Remove instances of this sender from both lists.
+    // Use loops because we allow multiple instances to get into the lists.
+    for (auto& [sender, eventType] : m_dispatchSoonList) {
+        if (sender == &senderToCancel && eventType == eventTypeToCancel)
+            sender = nullptr;
+    }
+    for (auto& [sender, eventType] : m_dispatchingList) {
+        if (sender == &senderToCancel && eventType == eventTypeToCancel)
+            sender = nullptr;
+    }
+}
+
+template<typename T, typename WeakPtrImpl> void EventSender<T, WeakPtrImpl>::dispatchPendingEvents(Page* page)
 {
     // Need to avoid re-entering this function; if new dispatches are
     // scheduled before the parent finishes processing the list, they
@@ -100,14 +117,14 @@ template<typename T, typename Counter> void EventSender<T, Counter>::dispatchPen
     m_dispatchSoonList.checkConsistency();
 
     m_dispatchingList = std::exchange(m_dispatchSoonList, { });
-    for (auto& event : m_dispatchingList) {
-        if (auto sender = event.get()) {
-            event = nullptr;
-            if (!page || sender->document().page() == page)
-                sender->dispatchPendingEvent(this);
-            else
-                dispatchEventSoon(*sender);
-        }
+    for (auto& [weakSender, eventType] : m_dispatchingList) {
+        if (!weakSender)
+            continue;
+        auto sender = std::exchange(weakSender, nullptr);
+        if (!page || sender->document().page() == page)
+            sender->dispatchPendingEvent(this, eventType);
+        else
+            dispatchEventSoon(*sender, eventType);
     }
     m_dispatchingList.clear();
 }

Source/WebCore/html/HTMLLinkElement.cpp

@@ -77,16 +77,10 @@ using namespace HTMLNames;
 
 static LinkEventSender& linkLoadEventSender()
 {
-    static NeverDestroyed<LinkEventSender> sharedLoadEventSender(eventNames().loadEvent);
+    static NeverDestroyed<LinkEventSender> sharedLoadEventSender;
     return sharedLoadEventSender;
 }
 
-static LinkEventSender& linkErrorEventSender()
-{
-    static NeverDestroyed<LinkEventSender> sharedErrorEventSender(eventNames().errorEvent);
-    return sharedErrorEventSender;
-}
-
 inline HTMLLinkElement::HTMLLinkElement(const QualifiedName& tagName, Document& document, bool createdByParser)
     : HTMLElement(tagName, document)
     , m_linkLoader(*this)
@@ -118,7 +112,6 @@ HTMLLinkElement::~HTMLLinkElement()
         m_styleScope->removeStyleSheetCandidateNode(*this);
 
     linkLoadEventSender().cancelEvent(*this);
-    linkErrorEventSender().cancelEvent(*this);
 }
 
 void HTMLLinkElement::setDisabledState(bool disabled)
@@ -529,13 +522,13 @@ void HTMLLinkElement::linkLoaded()
 {
     m_loadedResource = true;
     if (!m_relAttribute.isLinkPrefetch || m_allowPrefetchLoadAndErrorForTesting)
-        linkLoadEventSender().dispatchEventSoon(*this);
+        linkLoadEventSender().dispatchEventSoon(*this, eventNames().loadEvent);
 }
 
 void HTMLLinkElement::linkLoadingErrored()
 {
     if (!m_relAttribute.isLinkPrefetch || m_allowPrefetchLoadAndErrorForTesting)
-        linkErrorEventSender().dispatchEventSoon(*this);
+        linkLoadEventSender().dispatchEventSoon(*this, eventNames().errorEvent);
 }
 
 bool HTMLLinkElement::sheetLoaded()
@@ -552,13 +545,10 @@ void HTMLLinkElement::dispatchPendingLoadEvents(Page* page)
     linkLoadEventSender().dispatchPendingEvents(page);
 }
 
-void HTMLLinkElement::dispatchPendingEvent(LinkEventSender* eventSender)
+void HTMLLinkElement::dispatchPendingEvent(LinkEventSender* eventSender, const AtomString& eventType)
 {
-    ASSERT_UNUSED(eventSender, eventSender == &linkLoadEventSender() || eventSender == &linkErrorEventSender());
-    if (m_loadedResource)
-        dispatchEvent(Event::create(eventNames().loadEvent, Event::CanBubble::No, Event::IsCancelable::No));
-    else
-        dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
+    ASSERT_UNUSED(eventSender, eventSender == &linkLoadEventSender());
+    dispatchEvent(Event::create(eventType, Event::CanBubble::No, Event::IsCancelable::No));
 }
 
 DOMTokenList& HTMLLinkElement::relList()
@@ -573,7 +563,7 @@ DOMTokenList& HTMLLinkElement::relList()
 void HTMLLinkElement::notifyLoadedSheetAndAllCriticalSubresources(bool errorOccurred)
 {
     m_loadedResource = !errorOccurred;
-    linkLoadEventSender().dispatchEventSoon(*this);
+    linkLoadEventSender().dispatchEventSoon(*this, m_loadedResource ? eventNames().loadEvent : eventNames().errorEvent);
 }
 
 void HTMLLinkElement::startLoadingDynamicSheet()

Source/WebCore/html/HTMLLinkElement.h

@@ -76,7 +76,7 @@ class HTMLLinkElement final : public HTMLElement, public CachedStyleSheetClient,
     WEBCORE_EXPORT void setAs(const AtomString&);
     WEBCORE_EXPORT String as() const;
 
-    void dispatchPendingEvent(LinkEventSender*);
+    void dispatchPendingEvent(LinkEventSender*, const AtomString& eventType);
     static void dispatchPendingLoadEvents(Page*);
 
     WEBCORE_EXPORT DOMTokenList& relList();

Source/WebCore/html/HTMLStyleElement.cpp

@@ -47,7 +47,7 @@ using namespace HTMLNames;
 
 static StyleEventSender& styleLoadEventSender()
 {
-    static NeverDestroyed<StyleEventSender> sharedLoadEventSender(eventNames().loadEvent);
+    static NeverDestroyed<StyleEventSender> sharedLoadEventSender;
     return sharedLoadEventSender;
 }
 
@@ -128,19 +128,16 @@ void HTMLStyleElement::dispatchPendingLoadEvents(Page* page)
     styleLoadEventSender().dispatchPendingEvents(page);
 }
 
-void HTMLStyleElement::dispatchPendingEvent(StyleEventSender* eventSender)
+void HTMLStyleElement::dispatchPendingEvent(StyleEventSender* eventSender, const AtomString& eventType)
 {
     ASSERT_UNUSED(eventSender, eventSender == &styleLoadEventSender());
-    if (m_loadedSheet)
-        dispatchEvent(Event::create(eventNames().loadEvent, Event::CanBubble::No, Event::IsCancelable::No));
-    else
-        dispatchEvent(Event::create(eventNames().errorEvent, Event::CanBubble::No, Event::IsCancelable::No));
+    dispatchEvent(Event::create(eventType, Event::CanBubble::No, Event::IsCancelable::No));
 }
 
 void HTMLStyleElement::notifyLoadedSheetAndAllCriticalSubresources(bool errorOccurred)
 {
     m_loadedSheet = !errorOccurred;
-    styleLoadEventSender().dispatchEventSoon(*this);
+    styleLoadEventSender().dispatchEventSoon(*this, m_loadedSheet ? eventNames().loadEvent : eventNames().errorEvent);
 }
 
 void HTMLStyleElement::addSubresourceAttributeURLs(ListHashSet<URL>& urls) const

Source/WebCore/html/HTMLStyleElement.h

@@ -47,7 +47,7 @@ class HTMLStyleElement final : public HTMLElement {
     WEBCORE_EXPORT bool disabled() const;
     WEBCORE_EXPORT void setDisabled(bool);
 
-    void dispatchPendingEvent(StyleEventSender*);
+    void dispatchPendingEvent(StyleEventSender*, const AtomString& eventType);
     static void dispatchPendingLoadEvents(Page*);
 
     void finishParsingChildren() final;