Implement AbortSignal.protototype.onabort (#1848)

src/workerd/api/basics.c++

@@ -496,6 +496,33 @@ AbortSignal::AbortSignal(kj::Maybe<kj::Exception> exception,
     flag(flag),
     reason(kj::mv(maybeReason)) {}
 
+kj::Maybe<jsg::JsValue> AbortSignal::getOnAbort(jsg::Lock& js) {
+  return onAbortHandler.map([&](jsg::JsRef<jsg::JsValue>& ref)
+      -> jsg::JsValue { return ref.getHandle(js); });
+}
+
+void AbortSignal::setOnAbort(jsg::Lock& js, jsg::Optional<jsg::JsValue> handler) {
+  // We only want to accept the handler if it's a valid handler... For anything
+  // else, set it to null.
+  KJ_IF_SOME(h, handler) {
+    if (h.isFunction() || h.isObject()) {
+      onAbortHandler = jsg::JsRef(js, h);
+      return;
+    } else {
+      // TODO(soon): Per the spec we are supposed o set the handler to null if it is not
+      // a function or an object. However, there's an ever so slight change that would
+      // be breaking. So let's go ahead and set the value in this case and log a warning.
+      // If we do not see any instances of the warning in logs, we can remove this and
+      // go with the default behavior.
+      LOG_WARNING_PERIODICALLY(
+          "NOSENTRY AbortSignal::setOnAbort set to non-function/non-object value");
+      onAbortHandler = jsg::JsRef(js, h);
+      return;
+    }
+  }
+  onAbortHandler = kj::none;
+}
+
 jsg::JsValue AbortSignal::getReason(jsg::Lock& js) {
   KJ_IF_SOME(r, reason) {
     return r.getHandle(js);
@@ -612,7 +639,7 @@ jsg::Ref<AbortSignal> AbortSignal::any(
 }
 
 void AbortSignal::visitForGc(jsg::GcVisitor& visitor) {
-  visitor.visit(reason);
+  visitor.visit(reason, onAbortHandler);
 }
 
 RefcountedCanceler& AbortSignal::getCanceler() {

src/workerd/api/basics.h

@@ -493,6 +493,13 @@ class AbortSignal final: public EventTarget {
       kj::Array<jsg::Ref<AbortSignal>> signals,
       const jsg::TypeHandler<EventTarget::HandlerFunction>& handler);
 
+  // While AbortSignal extends EventTarget, and our EventTarget implementation will
+  // automatically support onabort being set as an own property, the spec defines
+  // onabort as a prototype property on the AbortSignal prototype. Therefore, we
+  // need to explicitly set it as a prototype property here.
+  kj::Maybe<jsg::JsValue> getOnAbort(jsg::Lock& js);
+  void setOnAbort(jsg::Lock& js, jsg::Optional<jsg::JsValue> handler);
+
   JSG_RESOURCE_TYPE(AbortSignal, CompatibilityFlags::Reader flags) {
     JSG_INHERIT(EventTarget);
     JSG_STATIC_METHOD(abort);
@@ -505,6 +512,7 @@ class AbortSignal final: public EventTarget {
       JSG_READONLY_INSTANCE_PROPERTY(aborted, getAborted);
       JSG_READONLY_INSTANCE_PROPERTY(reason, getReason);
     }
+    JSG_PROTOTYPE_PROPERTY(onabort, getOnAbort, setOnAbort);
     JSG_METHOD(throwIfAborted);
   }
 
@@ -543,6 +551,7 @@ class AbortSignal final: public EventTarget {
   IoOwn<RefcountedCanceler> canceler;
   Flag flag;
   kj::Maybe<jsg::JsRef<jsg::JsValue>> reason;
+  kj::Maybe<jsg::JsRef<jsg::JsValue>> onAbortHandler;
 
   void visitForGc(jsg::GcVisitor& visitor);
 

src/workerd/api/http-test.js

@@ -129,7 +129,7 @@ export const inspect = {
   keepalive: false,
   integrity: '',
   cf: undefined,
-  signal: AbortSignal { reason: undefined, aborted: false },
+  signal: AbortSignal { onabort: null, reason: undefined, aborted: false },
   fetcher: null,
   redirect: 'follow',
   headers: Headers(1) { 'content-type' => 'text/plain', [immutable]: false },

src/workerd/api/tests/abortsignal-test.js

@@ -163,3 +163,36 @@ export const anyAbort3 = {
     strictEqual(any.reason, 123);
   }
 };
+
+export const onabortPrototypeProperty = {
+  test() {
+    const ac = new AbortController();
+    ok('onabort' in AbortSignal.prototype);
+    strictEqual(ac.signal.onabort, null);
+    delete ac.signal.onabort;
+    ok('onabort' in AbortSignal.prototype);
+    strictEqual(ac.signal.onabort, null);
+    let called = false;
+    ac.signal.onabort = () => {
+      called = true;
+    };
+    ac.abort();
+    ok(called);
+
+    // Setting the value to something other than a function or object
+    // should cause the value to become null.
+    [123, null, 'foo'].forEach((v) => {
+      ac.signal.onabort = () => {};
+      ac.signal.onabort = v;
+      // TODO(soon): For now, we are relaxing this check and will log a warning
+      // if the value is not a function or object. If we get no hits on that warning,
+      // we can return to checking for null here.
+      //strictEqual(ac.signal.onabort, null);
+      strictEqual(ac.signal.onabort, v);
+    });
+
+    const handler = {};
+    ac.signal.onabort = handler;
+    strictEqual(ac.signal.onabort, handler);
+  }
+};