Merge pull request #1653 from cloudflare/revert-1648-jsnell/fixup-gct…

…race-basics

Revert "Fixup gc tracing in EventTarget"

src/workerd/api/basics.c++

@@ -33,17 +33,17 @@ bool isSpecialEventType(kj::StringPtr type) {
 
 EventTarget::NativeHandler::NativeHandler(
     jsg::Lock& js,
-    jsg::Ref<EventTarget> target,
+    EventTarget& target,
     kj::String type,
     jsg::Function<Signature> func,
     bool once)
     : type(kj::mv(type)),
       state(State {
-        .target = kj::mv(target),
+        .target = target,
         .func = kj::mv(func),
       }),
       once(once) {
-  KJ_ASSERT_NONNULL(state).target->addNativeListener(js, *this);
+  target.addNativeListener(js, *this);
 }
 
 EventTarget::NativeHandler::~NativeHandler() noexcept(false) { detach(); }
@@ -52,8 +52,7 @@ void EventTarget::NativeHandler::operator()(jsg::Lock& js, jsg::Ref<Event> event
   KJ_IF_SOME(s, state) {
     if (once) {
       auto fn = kj::mv(s.func);
-      auto target = kj::mv(s.target);
-      state = kj::none;
+      detach();
       fn(js, kj::mv(event));
       // Note that the function may have detached itself and caused the NativeHandler
       // to be destroyed. Let's be careful not to touch it after this point.
@@ -71,23 +70,22 @@ uint EventTarget::NativeHandler::hashCode() const {
 void EventTarget::NativeHandler::visitForGc(jsg::GcVisitor& visitor) {
   KJ_IF_SOME(s, state) {
     visitor.visit(s.func);
-    visitor.visit(s.target);
   }
 }
 
 void EventTarget::NativeHandler::detach() {
   KJ_IF_SOME(s, state) {
-    s.target->removeNativeListener(*this);
+    s.target.removeNativeListener(*this);
     state = kj::none;
   }
 }
 
-kj::Own<EventTarget::NativeHandler> EventTarget::newNativeHandler(
+kj::Own<void> EventTarget::newNativeHandler(
     jsg::Lock& js,
     kj::String type,
     jsg::Function<void(jsg::Ref<Event>)> func,
     bool once) {
-  return kj::heap<EventTarget::NativeHandler>(js, JSG_THIS, kj::mv(type), kj::mv(func), once);
+  return kj::heap<EventTarget::NativeHandler>(js, *this, kj::mv(type), kj::mv(func), once);
 }
 
 const EventTarget::EventHandler::Handler& EventTarget::EventHandlerHashCallbacks::keyForRow(
@@ -192,7 +190,18 @@ jsg::Ref<EventTarget> EventTarget::constructor() {
   return jsg::alloc<EventTarget>();
 }
 
-EventTarget::~EventTarget() noexcept(false) {}
+EventTarget::~EventTarget() noexcept(false) {
+  for (auto& entry : typeMap) {
+    for (auto& handler : entry.value.handlers) {
+      KJ_IF_SOME(native, handler.handler.tryGet<EventHandler::NativeHandlerRef>()) {
+        // Note: Can't call `detach()` here because it would loop back and call
+        // `removeNativeListener()` on us, invalidating the `typeMap` iterator. We'll directly
+        // null out the state.
+        native.handler.state = kj::none;
+      }
+    }
+  }
+}
 
 size_t EventTarget::getHandlerCount(kj::StringPtr type) const {
   KJ_IF_SOME(handlerSet, typeMap.find(type)) {
@@ -267,8 +276,7 @@ void EventTarget::addEventListener(jsg::Lock& js, kj::String type,
         removeEventListener(js, kj::mv(type), kj::mv(handler), kj::none);
       });
 
-      return kj::heap<NativeHandler>(js, signal.addRef(),
-          kj::str("abort"), kj::mv(func), true);
+      return signal->newNativeHandler(js, kj::str("abort"), kj::mv(func), true);
     });
 
     EventHandler eventHandler {
@@ -658,7 +666,25 @@ void EventTarget::visitForGc(jsg::GcVisitor& visitor) {
           visitor.visit(js);
         }
         KJ_CASE_ONEOF(native, EventHandler::NativeHandlerRef) {
-          // This is just a ref to the native handler, no need to visit.
+          // Note that even though `native.handler` is a non-owned reference, we still need to
+          // visit it. This is because we are the ones that will invoke the handles contained
+          // in the native handler if it ever fires. The actual owner of the C++ NativeHandler
+          // object doesn't ever access the JS objects it contains; the ownership relationship
+          // exists only for RAII reasons, so that the NativeHandler is automatically unregistered
+          // if the owner is destroyed.
+          //
+          // You might say: "Well, it's fine if the owner is responsible for visiting it, because
+          // if the owner is no longer reachable then it will be destroyed and it will unregister
+          // itself from here!" That doesn't quite work: V8's GC doesn't necessarily destroy
+          // objects immediately when they become unreachable. However, it is no longer safe to
+          // access an object once it is unreachable. Therefore, if we left it to the
+          // NativeHandler's owner to visit the object, it's possible that the object becomes
+          // poison some time before it is actually unregistered.
+          //
+          // Put another way, this is a very weird case where the C++ ownership and the JavaScript
+          // ownership are different. We need GC visitation to follow the JavaScript ownership
+          // graph.
+          visitor.visit(native.handler);
         }
       }
     }

src/workerd/api/basics.h

@@ -231,36 +231,6 @@ class CustomEvent: public Event {
 // An implementation of the Web Platform Standard EventTarget API
 class EventTarget: public jsg::Object {
 public:
-
-  // RAII-style listener that can be attached to an EventTarget.
-  class NativeHandler {
-  public:
-    using Signature = void(jsg::Ref<Event>);
-    NativeHandler(jsg::Lock& js, jsg::Ref<EventTarget> target, kj::String type,
-        jsg::Function<Signature> func, bool once = false);
-    ~NativeHandler() noexcept(false);
-    KJ_DISALLOW_COPY_AND_MOVE(NativeHandler);
-
-    void operator()(jsg::Lock&js, jsg::Ref<Event> event);
-
-    uint hashCode() const;
-
-    void visitForGc(jsg::GcVisitor& visitor);
-  private:
-    void detach();
-
-    kj::String type;
-    struct State {
-      jsg::Ref<EventTarget> target;
-      jsg::Function<Signature> func;
-    };
-
-    kj::Maybe<State> state;
-    bool once;
-
-    friend class EventTarget;
-  };
-
   ~EventTarget() noexcept(false);
 
   size_t getHandlerCount(kj::StringPtr type) const;
@@ -335,33 +305,73 @@ class EventTarget: public jsg::Object {
   static jsg::Ref<EventTarget> constructor();
 
   // Registers a lambda that will be called when the given event type is emitted.
-  // The handler will be registered for as long as the returned kj::Own<NativeHandler>
+  // The handler will be registered for as long as the returned kj::Own<void>
   // handle is held. If the EventTarget is destroyed while the native handler handle
   // is held, it will be automatically detached.
-  kj::Own<NativeHandler> newNativeHandler(jsg::Lock& js,
+  //
+  // The caller must not do anything with the returned Own<void> except drop it. This is why it
+  // is Own<void> and not Own<NativeHandler>.
+  kj::Own<void> newNativeHandler(jsg::Lock& js,
       kj::String type,
       jsg::Function<void(jsg::Ref<Event>)> func,
       bool once = false);
 
   void visitForMemoryInfo(jsg::MemoryTracker& tracker) const;
 
 private:
+  // RAII-style listener that can be attached to an EventTarget.
+  class NativeHandler {
+  public:
+    using Signature = void(jsg::Ref<Event>);
+    NativeHandler(jsg::Lock& js, EventTarget& target, kj::String type,
+        jsg::Function<Signature> func, bool once = false);
+    ~NativeHandler() noexcept(false);
+    KJ_DISALLOW_COPY_AND_MOVE(NativeHandler);
+
+    void operator()(jsg::Lock&js, jsg::Ref<Event> event);
+
+    uint hashCode() const;
+
+    void visitForGc(jsg::GcVisitor& visitor);
+  private:
+    void detach();
+
+    kj::String type;
+    struct State {
+      // target's destructor will null out `state`, so this is OK to be a bare reference.
+      EventTarget& target;
+
+      jsg::Function<Signature> func;
+    };
+
+    kj::Maybe<State> state;
+    bool once;
+
+    friend class EventTarget;
+  };
+
   void addNativeListener(jsg::Lock& js, NativeHandler& handler);
   bool removeNativeListener(NativeHandler& handler);
 
   struct EventHandler {
     struct JavaScriptHandler {
       jsg::HashableV8Ref<v8::Object> identity;
       HandlerFunction callback;
-      // If the event handler is registered with an AbortSignal, then the abortHandler
-      // is set and will ensure that the handler is removed correctly.
-      kj::Maybe<kj::Own<NativeHandler>> abortHandler;
+
+      // If the event handler is registered with an AbortSignal, then the abortHandler points
+      // at the NativeHandler representing that registration, so that if this object is GC'd before
+      // the AbortSignal is signaleled, we unregister ourselves from listening on it. Note that
+      // this is Own<void> for the same reason newNativeHandler() returns Own<void>: We are not
+      // supposed to do anything with this except drop it.
+      kj::Maybe<kj::Own<void>> abortHandler;
 
       void visitForGc(jsg::GcVisitor& visitor) {
         visitor.visit(identity, callback);
-        KJ_IF_SOME(handler, abortHandler) {
-          handler->visitForGc(visitor);
-        }
+
+        // Note that we intentionally do NOT visit `abortHandler`. This is because the JS handles
+        // held by `abortHandler` are not ever accessed by this path. Instead, they are accessed
+        // by the AbortSignal, if and when it fires. So it is the AbortSignal's responsibility to
+        // visit the NativeHandler's content.
       }
 
       kj::StringPtr jsgGetMemoryName() const { return "JavaScriptHandler"_kjc; }
@@ -594,8 +604,6 @@ class Scheduler final: public jsg::Object {
 private:
 };
 
-KJ_DECLARE_NON_POLYMORPHIC(workerd::api::EventTarget::NativeHandler);
-
 #define EW_BASICS_ISOLATE_TYPES                \
     api::Event,                                \
     api::Event::Init,                          \