Bug 1679204 - Consider to add signal to addEventListener, r=edgar

This passes the tests which are in https://github.com/web-platform-tests/wpt/pull/26472

Because of complications in #include handling, AbortFollower needs to be in a different
header file than AbortSignal, yet AbortSignalImpl needs to be available when AbortFollower is used.
Another option would have been to make DOMEventTargetHelper.h a bit different and uninline some hot methods
there or move them to another file, but that would have been equally bad and Abort* is used way less often.
AbortFollower and AbortSignalImpl are thus just moved to a new header.

Memory management is such that Listener in EventListenerManager owns the possible ListenerSignalFollower
instance which follows the relevant signal. In order to be able remove event listener,
ListenerSignalFollower has many similar fields as Listener.
ListenerSignalFollower can't easily have just a pointer to Listener* since Listener isn't stored as a pointer
in EventListenerManager.
ListenerSignalFollower is cycle collectable so that Listener->ListenerSignalFollower can be traversed/unlinked
and also strong pointers in ListenerSignalFollower itself can be traversed/unlinked.

There is an XXX in the .webidl, since nullability of signal is unclear in the spec pr.
Whether or not it ends up being nullable shouldn't change the actual C++ implementation.

Differential Revision: https://phabricator.services.mozilla.com/D97938

dom/abort/AbortFollower.h

@@ -0,0 +1,83 @@
+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim: set ts=8 sts=2 et sw=2 tw=80: */
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#ifndef mozilla_dom_AbortFollower_h
+#define mozilla_dom_AbortFollower_h
+
+#include "nsISupportsImpl.h"
+#include "nsTObserverArray.h"
+
+namespace mozilla {
+namespace dom {
+
+class AbortSignal;
+class AbortSignalImpl;
+
+// This class must be implemented by objects who want to follow an
+// AbortSignalImpl.
+class AbortFollower : public nsISupports {
+ public:
+  virtual void RunAbortAlgorithm() = 0;
+
+  void Follow(AbortSignalImpl* aSignal);
+
+  void Unfollow();
+
+  bool IsFollowing() const;
+
+  AbortSignalImpl* Signal() const { return mFollowingSignal; }
+
+ protected:
+  // Subclasses of this class must call these Traverse and Unlink functions
+  // during corresponding cycle collection operations.
+  static void Traverse(AbortFollower* aFollower,
+                       nsCycleCollectionTraversalCallback& cb);
+
+  static void Unlink(AbortFollower* aFollower) { aFollower->Unfollow(); }
+
+  virtual ~AbortFollower();
+
+  friend class AbortSignalImpl;
+
+  RefPtr<AbortSignalImpl> mFollowingSignal;
+};
+
+class AbortSignalImpl : public nsISupports {
+ public:
+  explicit AbortSignalImpl(bool aAborted);
+
+  bool Aborted() const;
+
+  virtual void SignalAbort();
+
+ protected:
+  // Subclasses of this class must call these Traverse and Unlink functions
+  // during corresponding cycle collection operations.
+  static void Traverse(AbortSignalImpl* aSignal,
+                       nsCycleCollectionTraversalCallback& cb);
+
+  static void Unlink(AbortSignalImpl* aSignal) {
+    // To be filled in shortly.
+  }
+
+  virtual ~AbortSignalImpl() = default;
+
+ private:
+  friend class AbortFollower;
+
+  // Raw pointers.  |AbortFollower::Follow| adds to this array, and
+  // |AbortFollower::Unfollow| (also callbed by the destructor) will remove
+  // from this array.  Finally, calling |SignalAbort()| will (after running all
+  // abort algorithms) empty this and make all contained followers |Unfollow()|.
+  nsTObserverArray<AbortFollower*> mFollowers;
+
+  bool mAborted;
+};
+
+}  // namespace dom
+}  // namespace mozilla
+
+#endif  // mozilla_dom_AbortFollower_h

dom/abort/AbortSignal.h

@@ -7,77 +7,12 @@
 #ifndef mozilla_dom_AbortSignal_h
 #define mozilla_dom_AbortSignal_h
 
+#include "mozilla/dom/AbortFollower.h"
 #include "mozilla/DOMEventTargetHelper.h"
-#include "nsISupportsImpl.h"
-#include "nsTObserverArray.h"
 
 namespace mozilla {
 namespace dom {
 
-class AbortSignal;
-class AbortSignalImpl;
-
-// This class must be implemented by objects who want to follow an
-// AbortSignalImpl.
-class AbortFollower : public nsISupports {
- public:
-  virtual void RunAbortAlgorithm() = 0;
-
-  void Follow(AbortSignalImpl* aSignal);
-
-  void Unfollow();
-
-  bool IsFollowing() const;
-
-  AbortSignalImpl* Signal() const { return mFollowingSignal; }
-
- protected:
-  // Subclasses of this class must call these Traverse and Unlink functions
-  // during corresponding cycle collection operations.
-  static void Traverse(AbortFollower* aFollower,
-                       nsCycleCollectionTraversalCallback& cb);
-
-  static void Unlink(AbortFollower* aFollower) { aFollower->Unfollow(); }
-
-  virtual ~AbortFollower();
-
-  friend class AbortSignalImpl;
-
-  RefPtr<AbortSignalImpl> mFollowingSignal;
-};
-
-class AbortSignalImpl : public nsISupports {
- public:
-  explicit AbortSignalImpl(bool aAborted);
-
-  bool Aborted() const;
-
-  virtual void SignalAbort();
-
- protected:
-  // Subclasses of this class must call these Traverse and Unlink functions
-  // during corresponding cycle collection operations.
-  static void Traverse(AbortSignalImpl* aSignal,
-                       nsCycleCollectionTraversalCallback& cb);
-
-  static void Unlink(AbortSignalImpl* aSignal) {
-    // To be filled in shortly.
-  }
-
-  virtual ~AbortSignalImpl() = default;
-
- private:
-  friend class AbortFollower;
-
-  // Raw pointers.  |AbortFollower::Follow| adds to this array, and
-  // |AbortFollower::Unfollow| (also callbed by the destructor) will remove
-  // from this array.  Finally, calling |SignalAbort()| will (after running all
-  // abort algorithms) empty this and make all contained followers |Unfollow()|.
-  nsTObserverArray<AbortFollower*> mFollowers;
-
-  bool mAborted;
-};
-
 // AbortSignal the spec concept includes the concept of a child signal
 // "following" a parent signal -- internally, adding abort steps to the parent
 // signal that will then signal abort on the child signal -- to propagate

dom/abort/moz.build

@@ -11,6 +11,7 @@ TEST_DIRS += ["tests"]
 
 EXPORTS.mozilla.dom += [
     "AbortController.h",
+    "AbortFollower.h",
     "AbortSignal.h",
 ]
 

dom/events/DOMEventTargetHelper.cpp

@@ -11,6 +11,7 @@
 #include "mozilla/DOMEventTargetHelper.h"
 #include "mozilla/EventDispatcher.h"
 #include "mozilla/EventListenerManager.h"
+#include "mozilla/EventListenerManager.h"
 #include "mozilla/Likely.h"
 #include "MainThreadUtils.h"
 

dom/events/EventListenerManager.cpp

@@ -19,6 +19,7 @@
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/PresShell.h"
+#include "mozilla/dom/AbortSignal.h"
 #include "mozilla/dom/BindingUtils.h"
 #include "mozilla/dom/EventCallbackDebuggerNotification.h"
 #include "mozilla/dom/Element.h"
@@ -172,6 +173,9 @@ inline void ImplCycleCollectionTraverse(
     CycleCollectionNoteChild(aCallback, aField.mListener.GetISupports(), aName,
                              aFlags);
   }
+
+  CycleCollectionNoteChild(aCallback, aField.mSignalFollower.get(),
+                           "mSignalFollower", aFlags);
 }
 
 NS_IMPL_CYCLE_COLLECTION_CLASS(EventListenerManager)
@@ -205,7 +209,7 @@ EventListenerManager::GetTargetAsInnerWindow() const {
 void EventListenerManager::AddEventListenerInternal(
     EventListenerHolder aListenerHolder, EventMessage aEventMessage,
     nsAtom* aTypeAtom, const EventListenerFlags& aFlags, bool aHandler,
-    bool aAllEvents) {
+    bool aAllEvents, AbortSignal* aSignal) {
   MOZ_ASSERT((aEventMessage && aTypeAtom) || aAllEvents,  // all-events listener
              "Missing type");
 
@@ -213,6 +217,10 @@ void EventListenerManager::AddEventListenerInternal(
     return;
   }
 
+  if (aSignal && aSignal->Aborted()) {
+    return;
+  }
+
   // Since there is no public API to call us with an EventListenerHolder, we
   // know that there's an EventListenerHolder on the stack holding a strong ref
   // to the listener.
@@ -255,6 +263,11 @@ void EventListenerManager::AddEventListenerInternal(
   }
   listener->mListener = std::move(aListenerHolder);
 
+  if (aSignal) {
+    listener->mSignalFollower = new ListenerSignalFollower(this, listener);
+    listener->mSignalFollower->Follow(aSignal);
+  }
+
   if (aFlags.mInSystemGroup) {
     mMayHaveSystemGroupListeners = true;
   }
@@ -687,7 +700,8 @@ void EventListenerManager::MaybeMarkPassive(EventMessage aMessage,
 
 void EventListenerManager::AddEventListenerByType(
     EventListenerHolder aListenerHolder, const nsAString& aType,
-    const EventListenerFlags& aFlags, const Optional<bool>& aPassive) {
+    const EventListenerFlags& aFlags, const Optional<bool>& aPassive,
+    AbortSignal* aSignal) {
   RefPtr<nsAtom> atom;
   EventMessage message =
       GetEventMessageAndAtomForListener(aType, getter_AddRefs(atom));
@@ -699,7 +713,8 @@ void EventListenerManager::AddEventListenerByType(
     MaybeMarkPassive(message, flags);
   }
 
-  AddEventListenerInternal(std::move(aListenerHolder), message, atom, flags);
+  AddEventListenerInternal(std::move(aListenerHolder), message, atom, flags,
+                           false, false, aSignal);
 }
 
 void EventListenerManager::RemoveEventListenerByType(
@@ -1370,6 +1385,7 @@ void EventListenerManager::AddEventListener(
     bool aWantsUntrusted) {
   EventListenerFlags flags;
   Optional<bool> passive;
+  AbortSignal* signal = nullptr;
   if (aOptions.IsBoolean()) {
     flags.mCapture = aOptions.GetAsBoolean();
   } else {
@@ -1380,10 +1396,15 @@ void EventListenerManager::AddEventListener(
     if (options.mPassive.WasPassed()) {
       passive.Construct(options.mPassive.Value());
     }
+
+    if (options.mSignal.WasPassed()) {
+      signal = options.mSignal.Value();
+    }
   }
+
   flags.mAllowUntrustedEvents = aWantsUntrusted;
   return AddEventListenerByType(std::move(aListenerHolder), aType, flags,
-                                passive);
+                                passive, signal);
 }
 
 void EventListenerManager::RemoveEventListener(
@@ -1789,4 +1810,47 @@ EventListenerManager::GetScriptGlobalAndDocument(Document** aDoc) {
   return global.forget();
 }
 
+EventListenerManager::ListenerSignalFollower::ListenerSignalFollower(
+    EventListenerManager* aListenerManager,
+    EventListenerManager::Listener* aListener)
+    : dom::AbortFollower(),
+      mListenerManager(aListenerManager),
+      mListener(aListener->mListener.Clone()),
+      mTypeAtom(aListener->mTypeAtom),
+      mEventMessage(aListener->mEventMessage),
+      mAllEvents(aListener->mAllEvents),
+      mFlags(aListener->mFlags){};
+
+NS_IMPL_CYCLE_COLLECTION_CLASS(EventListenerManager::ListenerSignalFollower)
+
+NS_IMPL_CYCLE_COLLECTING_ADDREF(EventListenerManager::ListenerSignalFollower)
+NS_IMPL_CYCLE_COLLECTING_RELEASE(EventListenerManager::ListenerSignalFollower)
+
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(
+    EventListenerManager::ListenerSignalFollower)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mListener)
+  AbortFollower::Traverse(static_cast<AbortFollower*>(tmp), cb);
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(
+    EventListenerManager::ListenerSignalFollower)
+  NS_IMPL_CYCLE_COLLECTION_UNLINK(mListener)
+  AbortFollower::Unlink(static_cast<AbortFollower*>(tmp));
+  tmp->mListenerManager = nullptr;
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+
+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(
+    EventListenerManager::ListenerSignalFollower)
+  NS_INTERFACE_MAP_ENTRY(nsISupports)
+NS_INTERFACE_MAP_END
+
+void EventListenerManager::ListenerSignalFollower::RunAbortAlgorithm() {
+  if (mListenerManager) {
+    RefPtr<EventListenerManager> elm = mListenerManager;
+    mListenerManager = nullptr;
+    elm->RemoveEventListenerInternal(std::move(mListener), mEventMessage,
+                                     mTypeAtom, mFlags, mAllEvents);
+  }
+}
+
 }  // namespace mozilla

dom/events/EventListenerManager.h

@@ -8,6 +8,7 @@
 #define mozilla_EventListenerManager_h_
 
 #include "mozilla/BasicEvents.h"
+#include "mozilla/dom/AbortFollower.h"
 #include "mozilla/dom/EventListenerBinding.h"
 #include "mozilla/JSEventHandler.h"
 #include "mozilla/MemoryReporting.h"
@@ -28,6 +29,7 @@ namespace mozilla {
 
 class ELMCreationDetector;
 class EventListenerManager;
+class ListenerSignalFollower;
 
 namespace dom {
 class Event;
@@ -169,7 +171,36 @@ class EventListenerManager final : public EventListenerManagerBase {
   ~EventListenerManager();
 
  public:
+  struct Listener;
+  class ListenerSignalFollower : public dom::AbortFollower {
+   public:
+    explicit ListenerSignalFollower(EventListenerManager* aListenerManager,
+                                    Listener* aListener);
+
+    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
+    NS_DECL_CYCLE_COLLECTION_CLASS(ListenerSignalFollower)
+
+    void RunAbortAlgorithm() override;
+
+    void Disconnect() {
+      mListenerManager = nullptr;
+      mListener.Reset();
+      Unfollow();
+    }
+
+   protected:
+    ~ListenerSignalFollower() = default;
+
+    EventListenerManager* mListenerManager;
+    EventListenerHolder mListener;
+    RefPtr<nsAtom> mTypeAtom;
+    EventMessage mEventMessage;
+    bool mAllEvents;
+    EventListenerFlags mFlags;
+  };
+
   struct Listener {
+    RefPtr<ListenerSignalFollower> mSignalFollower;
     EventListenerHolder mListener;
     RefPtr<nsAtom> mTypeAtom;
     EventMessage mEventMessage;
@@ -208,7 +239,8 @@ class EventListenerManager final : public EventListenerManagerBase {
           mIsChrome(false) {}
 
     Listener(Listener&& aOther)
-        : mListener(std::move(aOther.mListener)),
+        : mSignalFollower(std::move(aOther.mSignalFollower)),
+          mListener(std::move(aOther.mListener)),
           mTypeAtom(std::move(aOther.mTypeAtom)),
           mEventMessage(aOther.mEventMessage),
           mListenerType(aOther.mListenerType),
@@ -229,6 +261,9 @@ class EventListenerManager final : public EventListenerManagerBase {
         static_cast<JSEventHandler*>(mListener.GetXPCOMCallback())
             ->Disconnect();
       }
+      if (mSignalFollower) {
+        mSignalFollower->Disconnect();
+      }
     }
 
     MOZ_ALWAYS_INLINE bool IsListening(const WidgetEvent* aEvent) const {
@@ -292,7 +327,8 @@ class EventListenerManager final : public EventListenerManagerBase {
   void AddEventListenerByType(
       EventListenerHolder aListener, const nsAString& type,
       const EventListenerFlags& aFlags,
-      const dom::Optional<bool>& aPassive = dom::Optional<bool>());
+      const dom::Optional<bool>& aPassive = dom::Optional<bool>(),
+      dom::AbortSignal* aSignal = nullptr);
   void RemoveEventListenerByType(nsIDOMEventListener* aListener,
                                  const nsAString& type,
                                  const EventListenerFlags& aFlags) {
@@ -598,7 +634,8 @@ class EventListenerManager final : public EventListenerManagerBase {
   void AddEventListenerInternal(EventListenerHolder aListener,
                                 EventMessage aEventMessage, nsAtom* aTypeAtom,
                                 const EventListenerFlags& aFlags,
-                                bool aHandler = false, bool aAllEvents = false);
+                                bool aHandler = false, bool aAllEvents = false,
+                                dom::AbortSignal* aSignal = nullptr);
   void RemoveEventListenerInternal(EventListenerHolder aListener,
                                    EventMessage aEventMessage,
                                    nsAtom* aUserType,

dom/webidl/EventTarget.webidl

@@ -21,6 +21,7 @@ dictionary EventListenerOptions {
 dictionary AddEventListenerOptions : EventListenerOptions {
   boolean passive;
   boolean once = false;
+  AbortSignal? signal; //XXX Spec PR is unclear whether this should be nullable.
   [ChromeOnly]
   boolean wantUntrusted;
 };

testing/web-platform/meta/dom/events/AddEventListenerOptions-signal.any.js.ini

@@ -1,34 +0,0 @@
-[AddEventListenerOptions-signal.any.html]
-  [Passing an AbortSignal to addEventListener works with the once flag]
-    expected: FAIL
-
-  [Passing an AbortSignal to addEventListener works with the capture flag]
-    expected: FAIL
-
-  [Aborting from a listener does not call future listeners]
-    expected: FAIL
-
-  [Passing an AbortSignal to multiple listeners]
-    expected: FAIL
-
-  [Passing an AbortSignal to addEventListener options should allow removing a listener]
-    expected: FAIL
-
-
-[AddEventListenerOptions-signal.any.worker.html]
-  expected: ERROR
-  [Passing an AbortSignal to addEventListener works with the once flag]
-    expected: FAIL
-
-  [Passing an AbortSignal to addEventListener works with the capture flag]
-    expected: FAIL
-
-  [Aborting from a listener does not call future listeners]
-    expected: FAIL
-
-  [Passing an AbortSignal to multiple listeners]
-    expected: FAIL
-
-  [Passing an AbortSignal to addEventListener options should allow removing a listener]
-    expected: FAIL
-