Fix event handler ObjectEnvironment instantiation

This works around a bug (nodejs/node#30985) which would prevent eventually migrating the global object to using a Proxy. It also prevents a global variable named "element" or "formOwner" from breaking the inner with statements.
jsdom · Mar 28, 2021 · d5cfd69 · d5cfd69
1 parent 93e3d4a
commit d5cfd69
Show file tree

Hide file tree

Showing 2 changed files with 64 additions and 25 deletions.
diff --git a/lib/jsdom/living/helpers/create-event-accessor.js b/lib/jsdom/living/helpers/create-event-accessor.js
@@ -115,45 +115,49 @@ exports.getCurrentEventHandlerValue = (target, event) => {
 
     const createFunction = document.defaultView.Function;
     if (event === "error" && element === null) {
-      const wrapperBody = document ? body + `\n//# sourceURL=${document.URL}` : body;
+      const sourceURL = document ? `\n//# sourceURL=${document.URL}` : "";
 
-      // eslint-disable-next-line no-new-func
-      fn = createFunction("window", `with (window) { return function onerror(event, source, lineno, colno, error) {
-  ${wrapperBody}
-}; }`)(window);
+      fn = createFunction(`\
+with (arguments[0]) { return function onerror(event, source, lineno, colno, error) {
+${body}
+}; }${sourceURL}`)(window);
 
       fn = OnErrorEventHandlerNonNull.convert(fn);
     } else {
-      const argNames = [];
-      const args = [];
-
-      argNames.push("window");
-      args.push(window);
-
+      const calls = [];
       if (element !== null) {
-        argNames.push("document");
-        args.push(idlUtils.wrapperForImpl(document));
+        calls.push(idlUtils.wrapperForImpl(document));
       }
+
       if (formOwner !== null) {
-        argNames.push("formOwner");
-        args.push(idlUtils.wrapperForImpl(formOwner));
+        calls.push(idlUtils.wrapperForImpl(formOwner));
       }
+
       if (element !== null) {
-        argNames.push("element");
-        args.push(idlUtils.wrapperForImpl(element));
+        calls.push(idlUtils.wrapperForImpl(element));
       }
-      let wrapperBody = `
-return function on${event}(event) {
-  ${body}
-};`;
-      for (let i = argNames.length - 1; i >= 0; --i) {
-        wrapperBody = `with (${argNames[i]}) { ${wrapperBody} }`;
+
+      let wrapperBody = `\
+with (arguments[0]) { return function on${event}(event) {
+${body}
+}; }`;
+
+      // eslint-disable-next-line no-unused-vars
+      for (const call of calls) {
+        wrapperBody = `\
+with (arguments[0]) { return function () {
+${wrapperBody}
+}; }`;
       }
+
       if (document) {
         wrapperBody += `\n//# sourceURL=${document.URL}`;
       }
-      argNames.push(wrapperBody);
-      fn = createFunction(...argNames)(...args);
+
+      fn = createFunction(wrapperBody)(window);
+      for (const call of calls) {
+        fn = fn(call);
+      }
 
       if (event === "beforeunload") {
         fn = OnBeforeUnloadEventHandlerNonNull.convert(fn);

diff --git a/...ts/to-upstream/html/webappapis/events/event-handler-object-environment-instantiation.html b/...ts/to-upstream/html/webappapis/events/event-handler-object-environment-instantiation.html
@@ -0,0 +1,35 @@
+<!DOCTYPE html>
+<meta charset="UTF-8" />
+<title>Inline handler correct ObjectEnvironment instantiation</title>
+<link rel="author" title="ExE Boss" href="https://ExE-Boss.tech" />
+<link rel="help" href="https://html.spec.whatwg.org/multipage/webappapis.html#getting-the-current-value-of-the-event-handler" />
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div id="log"></div>
+<script>
+  "use strict";
+
+  window.element = document.element = {
+    get [Symbol.unscopables]() {
+      return assert_unreached("get element[Symbol.unscopables]()");
+    },
+    onclickCalled: null
+  };
+
+</script>
+<div id="foo" onclick='
+  assert_equals(element, window.element, "element === window.element");
+  assert_equals(onclickCalled, false);
+  onclickCalled = true;
+'></div>
+<script>
+  "use strict";
+
+  test(() => {
+    const foo = document.getElementById("foo");
+    foo.onclickCalled = false;
+    foo.click();
+    assert_equals(foo.onclickCalled, true);
+  });
+
+</script>