From 1ebb1fcd3823882e2cee36afcd1b78217b629ee4 Mon Sep 17 00:00:00 2001 From: Domenic Denicola Date: Wed, 29 Jan 2020 12:14:28 -0500 Subject: [PATCH] Re-do animation frame and timer callbacks This setup follows the spec more, is a lot simpler to follow, fixes #2800, and fixes #2617. --- lib/jsdom/browser/Window.js | 238 +++++++++++------- test/api/from-outside.js | 30 ++- test/web-platform-tests/to-run.yaml | 2 - .../animation-frames/nested-raf-not-sync.html | 28 +++ .../html/webappapis/timers/errors.html | 60 ++--- 5 files changed, 222 insertions(+), 136 deletions(-) create mode 100644 test/web-platform-tests/to-upstream/html/webappapis/animation-frames/nested-raf-not-sync.html diff --git a/lib/jsdom/browser/Window.js b/lib/jsdom/browser/Window.js index 2a52e6f2c9..9476048e3b 100644 --- a/lib/jsdom/browser/Window.js +++ b/lib/jsdom/browser/Window.js @@ -118,9 +118,6 @@ function Window(options) { this._globalProxy = this; Object.defineProperty(idlUtils.implForWrapper(this), idlUtils.wrapperSymbol, { get: () => this._globalProxy }); - let timers = Object.create(null); - let animationFrameCallbacks = Object.create(null); - // List options explicitly to be clear which are passed through this._document = Document.create(window, [], { options: { @@ -332,55 +329,157 @@ function Window(options) { ///// METHODS + // https://html.spec.whatwg.org/multipage/timers-and-user-prompts.html#timers + + // In the spec the list of active timers is a set of IDs. We make it a map of IDs to Node.js timer objects, so that + // we can call Node.js-side clearTimeout() when clearing, and thus allow process shutdown faster. + const listOfActiveTimers = new Map(); let latestTimerId = 0; - let latestAnimationFrameCallbackId = 0; - this.setTimeout = function (fn, ms) { - const args = []; - for (let i = 2; i < arguments.length; ++i) { - args[i - 2] = arguments[i]; + this.setTimeout = function (handler, timeout = 0, ...args) { + if (typeof handler !== "function") { + handler = webIDLConversions.DOMString(handler); } - return startTimer(window, setTimeout, clearTimeout, ++latestTimerId, fn, ms, timers, args); + timeout = webIDLConversions.long(timeout); + + return timerInitializationSteps(handler, timeout, args, { methodContext: window, repeat: false }); }; - this.setInterval = function (fn, ms) { - const args = []; - for (let i = 2; i < arguments.length; ++i) { - args[i - 2] = arguments[i]; + this.setInterval = function (handler, timeout = 0, ...args) { + if (typeof handler !== "function") { + handler = webIDLConversions.DOMString(handler); } - return startTimer(window, setInterval, clearInterval, ++latestTimerId, fn, ms, timers, args); + timeout = webIDLConversions.long(timeout); + + return timerInitializationSteps(handler, timeout, args, { methodContext: window, repeat: true }); }; - this.clearInterval = stopTimer.bind(this, timers); - this.clearTimeout = stopTimer.bind(this, timers); + + this.clearTimeout = function (handle = 0) { + handle = webIDLConversions.long(handle); + + const nodejsTimer = listOfActiveTimers.get(handle); + if (nodejsTimer) { + clearTimeout(nodejsTimer); + listOfActiveTimers.delete(handle); + } + }; + this.clearInterval = function (handle = 0) { + handle = webIDLConversions.long(handle); + + const nodejsTimer = listOfActiveTimers.get(handle); + if (nodejsTimer) { + // We use setTimeout() in timerInitializationSteps even for this.setInterval(). + clearTimeout(nodejsTimer); + listOfActiveTimers.delete(handle); + } + }; + + function timerInitializationSteps(handler, timeout, args, { methodContext, repeat, previousHandle }) { + // This appears to be unspecced, but matches browser behavior for close()ed windows. + if (!methodContext._document) { + return 0; + } + + // TODO: implement timer nesting level behavior. + + const methodContextProxy = methodContext._globalProxy; + const handle = previousHandle !== undefined ? previousHandle : ++latestTimerId; + + function task() { + if (!listOfActiveTimers.has(handle)) { + return; + } + + try { + if (typeof handler === "function") { + handler.apply(methodContextProxy, args); + } else if (window._runScripts === "dangerously") { + vm.runInContext(handler, window, { filename: window.location.href, displayErrors: false }); + } + } catch (e) { + reportException(window, e, window.location.href); + } + + if (repeat && listOfActiveTimers.has(handle)) { + timerInitializationSteps(handler, timeout, args, { methodContext, repeat: true, previousHandle: handle }); + } + } + + if (timeout < 0) { + timeout = 0; + } + + const nodejsTimer = setTimeout(task, timeout); + listOfActiveTimers.set(handle, nodejsTimer); + + return handle; + } + + // https://html.spec.whatwg.org/multipage/imagebitmap-and-animations.html#animation-frames + + let animationFrameCallbackId = 0; + const mapOfAnimationFrameCallbacks = new Map(); + let animationFrameNodejsInterval = null; + + // Unlike the spec, where an animation frame happens every 60 Hz regardless, we optimize so that if there are no + // requestAnimationFrame() calls outstanding, we don't fire the timer. This helps us track that. + let numberOfOngoingAnimationFrameCallbacks = 0; if (this._pretendToBeVisual) { - this.requestAnimationFrame = fn => { - const timestamp = rawPerformance.now() - windowInitialized; - const fps = 1000 / 60; - - return startTimer( - window, - setTimeout, - clearTimeout, - ++latestAnimationFrameCallbackId, - fn, - fps, - animationFrameCallbacks, - [timestamp] - ); + this.requestAnimationFrame = function (callback) { + callback = webIDLConversions.Function(callback); + + const handle = ++animationFrameCallbackId; + mapOfAnimationFrameCallbacks.set(handle, callback); + + ++numberOfOngoingAnimationFrameCallbacks; + if (numberOfOngoingAnimationFrameCallbacks === 1) { + animationFrameNodejsInterval = setInterval(() => { + runAnimationFrameCallbacks(rawPerformance.now() - windowInitialized); + }, 1000 / 60); + } + + return handle; }; - this.cancelAnimationFrame = stopTimer.bind(this, animationFrameCallbacks); - } - this.__stopAllTimers = function () { - stopAllTimers(timers); - stopAllTimers(animationFrameCallbacks); + this.cancelAnimationFrame = function (handle) { + handle = webIDLConversions["unsigned long"](handle); - latestTimerId = 0; - latestAnimationFrameCallbackId = 0; + if (mapOfAnimationFrameCallbacks.has(handle)) { + --numberOfOngoingAnimationFrameCallbacks; + if (numberOfOngoingAnimationFrameCallbacks === 0) { + clearInterval(animationFrameNodejsInterval); + } + } - timers = Object.create(null); - animationFrameCallbacks = Object.create(null); - }; + mapOfAnimationFrameCallbacks.delete(handle); + }; + + function runAnimationFrameCallbacks(now) { + // Converting to an array is important to get a sync snapshot and thus match spec semantics. + const callbackHandles = [...mapOfAnimationFrameCallbacks.keys()]; + for (const handle of callbackHandles) { + // This has() can be false if a callback calls cancelAnimationFrame(). + if (mapOfAnimationFrameCallbacks.has(handle)) { + const callback = mapOfAnimationFrameCallbacks.get(handle); + mapOfAnimationFrameCallbacks.delete(handle); + try { + callback(now); + } catch (e) { + reportException(window, e, window.location.href); + } + } + } + } + } + + function stopAllTimers() { + for (const nodejsTimer of listOfActiveTimers.values()) { + clearTimeout(nodejsTimer); + } + listOfActiveTimers.clear(); + + clearInterval(animationFrameNodejsInterval); + } function Option(text, value, defaultSelected, selected) { if (text === undefined) { @@ -506,18 +605,10 @@ function Window(options) { }; this.close = function () { - // Recursively close child frame windows, then ourselves. - const currentWindow = this; - (function windowCleaner(windowToClean) { - for (let i = 0; i < windowToClean.length; i++) { - windowCleaner(windowToClean[i]); - } - - // We"re already in our own window.close(). - if (windowToClean !== currentWindow) { - windowToClean.close(); - } - }(this)); + // Recursively close child frame windows, then ourselves (depth-first). + for (let i = 0; i < this.length; ++i) { + this[i].close(); + } // Clear out all listeners. Any in-flight or upcoming events should not get delivered. idlUtils.implForWrapper(this)._eventListeners = Object.create(null); @@ -540,7 +631,7 @@ function Window(options) { delete this._document; } - this.__stopAllTimers(); + stopAllTimers(); WebSocketImpl.cleanUpWindow(this); }; @@ -673,47 +764,6 @@ function Window(options) { }); } -function startTimer(window, startFn, stopFn, timerId, callback, ms, timerStorage, args) { - if (!window || !window._document) { - return undefined; - } - if (typeof callback !== "function") { - const code = String(callback); - callback = window._globalProxy.eval.bind(window, code + `\n//# sourceURL=${window.location.href}`); - } - - const oldCallback = callback; - callback = () => { - try { - oldCallback.apply(window._globalProxy, args); - } catch (e) { - reportException(window, e, window.location.href); - } - delete timerStorage[timerId]; - }; - - const res = startFn(callback, ms); - timerStorage[timerId] = [res, stopFn]; - return timerId; -} - -function stopTimer(timerStorage, id) { - const timer = timerStorage[id]; - if (timer) { - // Need to .call() with undefined to ensure the thisArg is not timer itself - timer[1].call(undefined, timer[0]); - delete timerStorage[id]; - } -} - -function stopAllTimers(timers) { - Object.keys(timers).forEach(key => { - const timer = timers[key]; - // Need to .call() with undefined to ensure the thisArg is not timer itself - timer[1].call(undefined, timer[0]); - }); -} - function contextifyWindow(window) { if (vm.isContext(window)) { return; diff --git a/test/api/from-outside.js b/test/api/from-outside.js index 3a9a4b4f9b..292a7144bf 100644 --- a/test/api/from-outside.js +++ b/test/api/from-outside.js @@ -2,15 +2,41 @@ const { assert } = require("chai"); const { describe, it } = require("mocha-sugar-free"); const { JSDOM } = require("../.."); +const { delay } = require("../util"); describe("Test cases only possible to test from the outside", () => { - it("should not register timer after window.close() called", () => { + it("window.close() should prevent timers from registering and cause them to return 0", async () => { const { window } = new JSDOM(); assert.notEqual(window.setTimeout(() => {}, 100), undefined); window.close(); - assert.equal(window.setTimeout(() => {}), undefined); + let ran = false; + assert.equal(window.setTimeout(() => { + ran = true; + }), 0); + + await delay(10); + + assert.equal(ran, false); + }); + + it("window.close() should stop a setInterval()", async () => { + const { window } = new JSDOM(``, { runScripts: "dangerously" }); + + await delay(55); + window.close(); + + // We can't assert it's equal to 5, because the event loop might have been busy and not fully executed all 5. + assert.isAtLeast(window.counter, 1); + const counterBeforeSecondDelay = window.counter; + + await delay(50); + + assert.equal(window.counter, counterBeforeSecondDelay); }); }); diff --git a/test/web-platform-tests/to-run.yaml b/test/web-platform-tests/to-run.yaml index 7693e99628..478a56f48f 100644 --- a/test/web-platform-tests/to-run.yaml +++ b/test/web-platform-tests/to-run.yaml @@ -866,8 +866,6 @@ serializing-html-fragments/serializing.html: [fail, https://github.com/inikulin/ DIR: html/webappapis/animation-frames -same-dispatch-time.html: [fail, Probably https://github.com/jsdom/jsdom/pull/1994#discussion_r147567274] - --- DIR: html/webappapis/atob diff --git a/test/web-platform-tests/to-upstream/html/webappapis/animation-frames/nested-raf-not-sync.html b/test/web-platform-tests/to-upstream/html/webappapis/animation-frames/nested-raf-not-sync.html new file mode 100644 index 0000000000..70c76fed87 --- /dev/null +++ b/test/web-platform-tests/to-upstream/html/webappapis/animation-frames/nested-raf-not-sync.html @@ -0,0 +1,28 @@ + + +requestAnimationFrame from inside requestAnimationFrame must be delayed, not sync + + + + + diff --git a/test/web-platform-tests/to-upstream/html/webappapis/timers/errors.html b/test/web-platform-tests/to-upstream/html/webappapis/timers/errors.html index de37df6f60..26564915b5 100644 --- a/test/web-platform-tests/to-upstream/html/webappapis/timers/errors.html +++ b/test/web-platform-tests/to-upstream/html/webappapis/timers/errors.html @@ -9,69 +9,53 @@ "use strict"; setup({ allow_uncaught_exception: true }); -promise_test(() => { - return new Promise(resolve => { - setTimeout("throw new Error('test1')", 10); +function setupListener(t, errorMessage, promiseResolve, optionalIntervalHandle) { + const listener = t.step_func(event => { + assert_equals(event.constructor.name, "ErrorEvent"); + assert_equals(event.error.message, errorMessage); + promiseResolve(); + }); - window.onerror = message => { - window.onerror = null; + window.addEventListener("error", listener); + t.add_cleanup(() => { + window.removeEventListener("error", listener); + clearInterval(optionalIntervalHandle); + }); +} - assert_equals(message, "test1"); +promise_test(t => { + return new Promise(resolve => { + setTimeout("throw new Error('test1')", 10); - resolve(); - return true; - }; + setupListener(t, "test1", resolve); }); }, "setTimeout, string argument"); -promise_test(() => { +promise_test(t => { return new Promise(resolve => { setTimeout(() => { throw new Error("test2"); }, 10); - window.onerror = message => { - window.onerror = null; - - assert_equals(message, "test2"); - - resolve(); - return true; - }; + setupListener(t, "test2", resolve); }); }, "setTimeout, function argument"); -promise_test(() => { +promise_test(t => { return new Promise(resolve => { const i = setInterval("throw new Error('test3')", 10); - window.onerror = message => { - window.onerror = null; - clearInterval(i); - - assert_equals(message, "test3"); - - resolve(); - return true; - }; + setupListener(t, "test3", resolve, i); }); }, "setInterval, string argument"); -promise_test(() => { +promise_test(t => { return new Promise(resolve => { const i = setInterval(() => { throw new Error("test4"); }, 10); - window.onerror = message => { - window.onerror = null; - clearInterval(i); - - assert_equals(message, "test4"); - - resolve(); - return true; - }; + setupListener(t, "test4", resolve, i); }); }, "setInterval, function argument");