Merge pull request #1028 from openkraken/fix/event_target

Fix: event target string property leak.

bridge/CMakeLists.txt

@@ -185,6 +185,7 @@ if ($ENV{KRAKEN_JS_ENGINE} MATCHES "quickjs")
     bindings/qjs/garbage_collected.h
     bindings/qjs/executing_context.cc
     bindings/qjs/executing_context.h
+    bindings/qjs/heap_hashmap.h
     bindings/qjs/native_value.cc
     bindings/qjs/native_value.h
     bindings/qjs/host_object.h

bridge/bindings/qjs/bom/timer.cc

@@ -39,6 +39,10 @@ void DOMTimer::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const {
   JS_MarkValue(rt, m_callback, mark_func);
 }
 
+void DOMTimer::dispose() const {
+  JS_FreeValueRT(m_runtime, m_callback);
+}
+
 int32_t DOMTimer::timerId() {
   return m_timerId;
 }

bridge/bindings/qjs/bom/timer.h

@@ -26,6 +26,7 @@ class DOMTimer : public GarbageCollected<DOMTimer> {
   [[nodiscard]] FORCE_INLINE const char* getHumanReadableName() const override { return "DOMTimer"; }
 
   void trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const override;
+  void dispose() const override;
 
  private:
   int32_t m_timerId{-1};

bridge/bindings/qjs/dom/element.cc

@@ -132,6 +132,11 @@ void ElementAttributes::dispose() const {
     JS_FreeValueRT(m_runtime, attr.second);
   }
 }
+void ElementAttributes::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const {
+  for (auto& attr : m_attributes) {
+    JS_MarkValue(rt, attr.second, mark_func);
+  }
+}
 
 JSValue Element::instanceConstructor(JSContext* ctx, JSValue func_obj, JSValue this_val, int argc, JSValue* argv) {
   if (argc == 0)

bridge/bindings/qjs/dom/element.h

@@ -55,6 +55,7 @@ class ElementAttributes : public GarbageCollected<ElementAttributes> {
   FORCE_INLINE const char* getHumanReadableName() const override { return "ElementAttributes"; }
 
   void dispose() const override;
+  void trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const override;
 
   JSValue getAttribute(const std::string& name);
   JSValue setAttribute(const std::string& name, JSValue value);

bridge/bindings/qjs/dom/event_listener_map.cc

@@ -86,4 +86,12 @@ void EventListenerMap::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func)
   }
 }
 
+EventListenerMap::~EventListenerMap() {
+  for (const auto& entry : m_entries) {
+    for (const auto& vector : entry.second) {
+      JS_FreeValueRT(m_runtime, vector);
+    }
+  }
+}
+
 }  // namespace kraken::binding::qjs

bridge/bindings/qjs/dom/event_listener_map.h

@@ -16,6 +16,9 @@ using EventListenerVector = std::vector<JSValue>;
 
 class EventListenerMap final {
  public:
+  EventListenerMap(JSContext* ctx) : m_runtime(JS_GetRuntime(ctx)){};
+  ~EventListenerMap();
+
   [[nodiscard]] bool empty() const { return m_entries.empty(); }
   [[nodiscard]] bool contains(JSAtom eventType) const;
   void clear();
@@ -32,6 +35,8 @@ class EventListenerMap final {
   //  - An EventTarget rarely has event listeners for many event types, and
   //    vector is faster in such cases.
   std::vector<std::pair<JSAtom, EventListenerVector>> m_entries;
+
+  JSRuntime* m_runtime;
 };
 
 }  // namespace kraken::binding::qjs

bridge/bindings/qjs/dom/event_target.cc

@@ -69,7 +69,7 @@ JSValue EventTarget::addEventListener(JSContext* ctx, JSValue this_val, int argc
   JSAtom eventType = JS_ValueToAtom(ctx, eventTypeValue);
 
   // Dart needs to be notified for the first registration event.
-  if (!eventTargetInstance->m_eventListenerMap.contains(eventType) || eventTargetInstance->m_eventHandlerMap.count(eventType) > 0) {
+  if (!eventTargetInstance->m_eventListenerMap.contains(eventType) || eventTargetInstance->m_eventHandlerMap.contains(eventType)) {
     int32_t contextId = eventTargetInstance->prototype()->contextId();
 
     NativeString args_01{};
@@ -113,7 +113,7 @@ JSValue EventTarget::removeEventListener(JSContext* ctx, JSValue this_val, int a
     JS_FreeValue(ctx, callback);
   }
 
-  if (eventHandlers.empty() && eventTargetInstance->m_eventHandlerMap.count(eventType) > 0) {
+  if (eventHandlers.empty() && eventTargetInstance->m_eventHandlerMap.contains(eventType)) {
     // Dart needs to be notified for handles is empty.
     int32_t contextId = eventTargetInstance->prototype()->contextId();
 
@@ -209,12 +209,12 @@ bool EventTargetInstance::internalDispatchEvent(EventInstance* eventInstance) {
   }
 
   // Dispatch event listener white by 'on' prefix property.
-  if (m_eventHandlerMap.count(eventType) > 0) {
+  if (m_eventHandlerMap.contains(eventType)) {
     // Let special error event handling be true if event is an ErrorEvent.
     bool specialErrorEventHanding = eventTypeStr == "error";
 
     if (specialErrorEventHanding) {
-      auto _dispatchErrorEvent = [&eventInstance, this, eventTypeStr](JSValue& handler) {
+      auto _dispatchErrorEvent = [&eventInstance, this, eventTypeStr](JSValue handler) {
         JSValue error = JS_GetPropertyStr(m_ctx, eventInstance->jsObject, "error");
         JSValue messageValue = JS_GetPropertyStr(m_ctx, error, "message");
         JSValue lineNumberValue = JS_GetPropertyStr(m_ctx, error, "lineNumber");
@@ -231,9 +231,9 @@ bool EventTargetInstance::internalDispatchEvent(EventInstance* eventInstance) {
         JS_FreeValue(m_ctx, lineNumberValue);
         JS_FreeValue(m_ctx, columnValue);
       };
-      _dispatchErrorEvent(m_eventHandlerMap[eventType]);
+      _dispatchErrorEvent(m_eventHandlerMap.getProperty(eventType));
     } else {
-      _dispatchEvent(m_eventHandlerMap[eventType]);
+      _dispatchEvent(m_eventHandlerMap.getProperty(eventType));
     }
   }
 
@@ -283,7 +283,7 @@ int EventTargetInstance::hasProperty(JSContext* ctx, JSValue obj, JSAtom atom) {
     return !JS_IsNull(eventTarget->getAttributesEventHandler(p));
   }
 
-  return eventTarget->m_properties.count(atom) > 0;
+  return eventTarget->m_properties.contains(atom);
 }
 
 JSValue EventTargetInstance::getProperty(JSContext* ctx, JSValue obj, JSAtom atom, JSValue receiver) {
@@ -305,8 +305,8 @@ JSValue EventTargetInstance::getProperty(JSContext* ctx, JSValue obj, JSAtom ato
     return eventTarget->getAttributesEventHandler(p);
   }
 
-  if (eventTarget->m_properties.count(atom) > 0) {
-    return JS_DupValue(ctx, eventTarget->m_properties[atom]);
+  if (eventTarget->m_properties.contains(atom)) {
+    return JS_DupValue(ctx, eventTarget->m_properties.getProperty(atom));
   }
 
   // For plugin elements, try to auto generate properties and functions from dart response.
@@ -355,7 +355,13 @@ int EventTargetInstance::setProperty(JSContext* ctx, JSValue obj, JSAtom atom, J
   if (!p->is_wide_char && p->len > 2 && p->u.str8[0] == 'o' && p->u.str8[1] == 'n') {
     eventTarget->setAttributesEventHandler(p, value);
   } else {
-    eventTarget->m_properties[atom] = JS_DupValue(ctx, value);
+    // GC can't track the value if key had been override.
+    // Should free the value if exist on m_properties.
+    if (eventTarget->m_properties.contains(atom)) {
+      JS_FreeValue(eventTarget->m_ctx, eventTarget->m_properties.getProperty(atom));
+    }
+
+    eventTarget->m_properties.setProperty(atom, JS_DupValue(ctx, value));
     if (isJavaScriptExtensionElementInstance(eventTarget->context(), eventTarget->jsObject) && !p->is_wide_char && p->u.str8[0] != '_') {
       std::unique_ptr<NativeString> args_01 = atomToNativeString(ctx, atom);
       std::unique_ptr<NativeString> args_02 = jsValueToNativeString(ctx, value);
@@ -394,8 +400,8 @@ void EventTargetInstance::setAttributesEventHandler(JSString* p, JSValue value)
   JSAtom atom = JS_NewAtom(m_ctx, eventType);
 
   // EventHandler are no long visible by GC. Should free it manually.
-  if (m_eventHandlerMap.count(atom) > 0) {
-    JS_FreeValue(m_ctx, m_eventHandlerMap[atom]);
+  if (m_eventHandlerMap.contains(atom)) {
+    JS_FreeValue(m_ctx, m_eventHandlerMap.getProperty(atom));
   }
 
   // When evaluate scripts like 'element.onclick = null', we needs to remove the event handlers callbacks
@@ -406,7 +412,7 @@ void EventTargetInstance::setAttributesEventHandler(JSString* p, JSValue value)
   }
 
   JSValue newCallback = JS_DupValue(m_ctx, value);
-  m_eventHandlerMap[atom] = newCallback;
+  m_eventHandlerMap.setProperty(atom, newCallback);
 
   if (JS_IsFunction(m_ctx, value) && m_eventListenerMap.empty()) {
     int32_t contextId = m_context->getContextId();
@@ -422,11 +428,14 @@ JSValue EventTargetInstance::getAttributesEventHandler(JSString* p) {
   char eventType[p->len + 1 - 2];
   memcpy(eventType, &p->u.str8[2], p->len + 1 - 2);
   JSAtom atom = JS_NewAtom(m_ctx, eventType);
-  if (m_eventHandlerMap.count(atom) == 0) {
+  if (!m_eventHandlerMap.contains(atom)) {
+    JS_FreeAtom(m_ctx, atom);
     return JS_NULL;
   }
 
-  return JS_DupValue(m_ctx, m_eventHandlerMap[atom]);
+  JSValue handler = JS_DupValue(m_ctx, m_eventHandlerMap.getProperty(atom));
+  JS_FreeAtom(m_ctx, atom);
+  return handler;
 }
 
 void EventTargetInstance::finalize(JSRuntime* rt, JSValue val) {
@@ -444,23 +453,18 @@ JSValue EventTargetInstance::getNativeProperty(const char* prop) {
 // JSValues are stored in this class are no visible to QuickJS GC.
 // We needs to gc which JSValues are still holding.
 void EventTargetInstance::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) {
-  // Trace m_eventHandlers
+  // Trace m_eventListeners.
   m_eventListenerMap.trace(rt, JS_UNDEFINED, mark_func);
 
-  for (auto& entry : m_eventHandlerMap) {
-    JS_MarkValue(rt, entry.second, mark_func);
-  }
+  // Trace m_eventHandlers.
+  m_eventHandlerMap.trace(rt, JS_UNDEFINED, mark_func);
 
-  for (auto& entry : m_properties) {
-    JS_MarkValue(rt, entry.second, mark_func);
-  }
+  // Trace properties.
+  m_properties.trace(rt, JS_UNDEFINED, mark_func);
 }
 
 void EventTargetInstance::copyNodeProperties(EventTargetInstance* newNode, EventTargetInstance* referenceNode) {
-  JSContext* ctx = referenceNode->m_ctx;
-  for (auto& entry : referenceNode->m_properties) {
-    newNode->m_properties[entry.first] = JS_DupValue(ctx, entry.second);
-  }
+  referenceNode->m_properties.copyWith(&newNode->m_properties);
 }
 
 void NativeEventTarget::dispatchEventImpl(NativeEventTarget* nativeEventTarget, NativeString* nativeEventType, void* rawEvent, int32_t isCustomEvent) {

bridge/bindings/qjs/dom/event_target.h

@@ -8,6 +8,7 @@
 
 #include "bindings/qjs/dom/event.h"
 #include "bindings/qjs/executing_context.h"
+#include "bindings/qjs/heap_hashmap.h"
 #include "bindings/qjs/host_class.h"
 #include "bindings/qjs/host_object.h"
 #include "bindings/qjs/native_value.h"
@@ -68,6 +69,16 @@ struct NativeEventTarget {
 #endif
 };
 
+class EventTargetProperties : public HeapHashMap<JSAtom> {
+ public:
+  EventTargetProperties(JSContext* ctx) : HeapHashMap<JSAtom>(ctx){};
+};
+
+class EventHandlerMap : public HeapHashMap<JSAtom> {
+ public:
+  EventHandlerMap(JSContext* ctx) : HeapHashMap<JSAtom>(ctx){};
+};
+
 class EventTargetInstance : public Instance {
  public:
   EventTargetInstance() = delete;
@@ -89,16 +100,16 @@ class EventTargetInstance : public Instance {
   int32_t m_eventTargetId;
   // EventListener handlers registered with addEventListener API.
   // https://dom.spec.whatwg.org/#concept-event-listener
-  EventListenerMap m_eventListenerMap;
+  EventListenerMap m_eventListenerMap{m_ctx};
 
   // EventListener handlers registered with DOM attributes API.
   // https://html.spec.whatwg.org/C/#event-handler-attributes
-  std::unordered_map<JSAtom, JSValue> m_eventHandlerMap;
+  EventHandlerMap m_eventHandlerMap{m_ctx};
 
   // When javascript code set a property on EventTarget instance, EventTarget::setProperty callback will be called when
   // property are not defined by Object.defineProperty or setProperty.
   // We store there values in here.
-  std::unordered_map<JSAtom, JSValue> m_properties;
+  EventTargetProperties m_properties{m_ctx};
 
   void trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) override;
   static void copyNodeProperties(EventTargetInstance* newNode, EventTargetInstance* referenceNode);

bridge/bindings/qjs/dom/event_target_test.cc

@@ -176,3 +176,11 @@ console.log(s.addEventListener, s.removeEventListener)
   EXPECT_EQ(errorCalled, false);
   EXPECT_EQ(logCalled, true);
 }
+
+TEST(EventTarget, wontLeakWithStringProperty) {
+  auto bridge = TEST_init();
+  std::string code =
+      "var img = new Image();\n"
+      "img.any = '1234'";
+  bridge->evaluateScript(code.c_str(), code.size(), "internal://", 0);
+}

bridge/bindings/qjs/dom/frame_request_callback_collection.cc

@@ -35,6 +35,10 @@ void FrameCallback::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) co
   JS_MarkValue(rt, m_callback, mark_func);
 }
 
+void FrameCallback::dispose() const {
+  JS_FreeValueRT(m_runtime, m_callback);
+}
+
 void FrameRequestCallbackCollection::registerFrameCallback(uint32_t callbackId, FrameCallback* frameCallback) {
   m_frameCallbacks[callbackId] = frameCallback;
 }

bridge/bindings/qjs/dom/frame_request_callback_collection.h

@@ -23,6 +23,7 @@ class FrameCallback : public GarbageCollected<FrameCallback> {
   [[nodiscard]] FORCE_INLINE const char* getHumanReadableName() const override { return "FrameCallback"; }
 
   void trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const override;
+  void dispose() const override;
 
  private:
   JSValue m_callback{JS_NULL};

bridge/bindings/qjs/dom/script_animation_controller.cc

@@ -19,6 +19,7 @@ void ScriptAnimationController::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* m
   auto* controller = static_cast<ScriptAnimationController*>(JS_GetOpaque(val, ScriptAnimationController::classId));
   controller->m_frameRequestCallbackCollection.trace(rt, JS_UNDEFINED, mark_func);
 }
+void ScriptAnimationController::dispose() const {}
 
 ScriptAnimationController* ScriptAnimationController::initialize(JSContext* ctx, JSClassID* classId) {
   return GarbageCollected::initialize(ctx, classId);

bridge/bindings/qjs/dom/script_animation_controller.h

@@ -24,6 +24,7 @@ class ScriptAnimationController : public GarbageCollected<ScriptAnimationControl
   [[nodiscard]] FORCE_INLINE const char* getHumanReadableName() const override { return "ScriptAnimationController"; }
 
   void trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const override;
+  void dispose() const override;
 
  private:
   FrameRequestCallbackCollection m_frameRequestCallbackCollection;

bridge/bindings/qjs/executing_context.cc

@@ -37,6 +37,7 @@ void ExecutionContextGCTracker::trace(JSRuntime* rt, JSValue val, JS_MarkFunc* m
   auto* context = static_cast<ExecutionContext*>(JS_GetContextOpaque(m_ctx));
   context->trace(rt, context->global(), mark_func);
 }
+void ExecutionContextGCTracker::dispose() const {}
 
 JSClassID ExecutionContextGCTracker::contextGcTrackerClassId{0};
 

bridge/bindings/qjs/executing_context.h

@@ -58,6 +58,7 @@ class ExecutionContextGCTracker : public GarbageCollected<ExecutionContextGCTrac
   static JSClassID contextGcTrackerClassId;
 
   void trace(JSRuntime* rt, JSValue val, JS_MarkFunc* mark_func) const override;
+  void dispose() const override;
 
  private:
 };

bridge/bindings/qjs/garbage_collected.h

@@ -48,13 +48,13 @@ class GarbageCollected {
    * This Trace method must be override by objects inheriting from
    * GarbageCollected.
    */
-  virtual void trace(JSRuntime* rt, JSValueConst val, JS_MarkFunc* mark_func) const {};
+  virtual void trace(JSRuntime* rt, JSValueConst val, JS_MarkFunc* mark_func) const = 0;
 
   /**
    * Called before underline JavaScript object been collected by GC.
    * Note: JS_FreeValue and JS_FreeAtom is not available, use JS_FreeValueRT and JS_FreeAtomRT instead.
    */
-  virtual void dispose() const {};
+  virtual void dispose() const = 0;
 
   /**
    * Specifies a name for the garbage-collected object. Such names will never