Use clean global when setting up Window

[ianschmitz]

Fixes #2961.

Use a clean instance of the global object when looking for global entries to install into the jsdom environment. This avoids issues when the global object has had APIs polyfilled that aren't available within a new vm context.

Credit goes to @SimenB for helping troubleshoot!

[domenic]

Can you add a test that fails before this change, and work to pass the lint check?

[TimothyGu]

Other than the linting issue, this looks good. I'll let @domenic and @pmdartus take a look though as they are more familiar with this.

Oops my browser didn't refresh for @domenic's comment.

[ianschmitz]

Started writing a test today, but it started to get a little gross so wanted to run it by you first.

I wrote a describe block similar to

jsdom/test/api/options-run-scripts.js

Lines 127 to 149 in 8921128

	const jsSpecGlobalsDescribe = hasNode10 ? describe.skip : describe;
	jsSpecGlobalsDescribe("JS spec globals", () => {
	it("should include aliased globals by default", () => {
	// Sanity check that our global-generation process hasn't broken.
	assert.include(jsGlobals, "TypeError");
	assert.include(jsGlobals, "Math");
	assert.include(jsGlobals, "Function");
	const dom = new JSDOM();
	for (const globalName of jsGlobals) {
	assertAliasedGlobal(dom.window, globalName);
	}
	});
	for (const optionValue of ["outside-only", "dangerously"]) {
	it(`should include fresh globals when set to "${optionValue}"`, () => {
	const dom = new JSDOM(undefined, { runScripts: optionValue });
	for (const globalName of jsGlobals) {
	assertFreshGlobal(dom.window, globalName);
	}
	});
	}
	});

  // `globalThis` is not supported by Node 10 which makes it useful for testing
  const jsSpecGlobalsPolyfillDescribe = hasNode10 ? describe : describe.skip;
  jsSpecGlobalsPolyfillDescribe("JS spec polyfilled globals", () => {
    afterEach(() => {
      delete global.globalThis;
    });

    // Allows us to modify global object prior to running module code
    function getFreshJsdomModule() {
      delete require.cache[require.resolve("../..")];
      // eslint-disable-next-line global-require
      return require("../..");
    }

    for (const optionValue of ["outside-only", "dangerously"]) {
      it(`should not include globals not available in fresh global when set to "${optionValue}"`, () => {
        global.globalThis = global;
        const { JSDOM: FreshJSDOM } = getFreshJsdomModule();
        new FreshJSDOM(undefined, { runScripts: optionValue })
        assert.doesNotThrow(() => new FreshJSDOM(undefined, { runScripts: optionValue }));
      });
    }
  });

However there are some issues with this approach:

• To be able to get a fresh instance of the module we'd have to remove the top level require() and rewrite the other references in the test suite
• This test won't actually fail on Node 10, because the js-globals.json that gets generated won't include globalThis, so we'd have to potentially also mutate that module?

Which lead me to thinking.. perhaps instead of running generate-js-globals as part of prepare, could it be done as part of postinstall? This would then generate it in the consumer's environment, instead of the environment at the time of publish (which appears to be Node 12, which is why globalThis is present). Thoughts?

[SimenB]

This vmGlobal thing is essentially what the script does at build time.

jsdom/scripts/generate-js-globals.js

Lines 14 to 16 in 8921128

	const context = vm.createContext();
	const globals = vm.runInContext("Object.getOwnPropertyDescriptors(this)", context);

So this PR partly removes the optimization introduced by having it in a script anyways.

Would it make sense to just do the generate-js-globals thing lazily in the code? Or even not lazily, since running the script (without the write to fs) takes about 35ms

Benchmark #1: node file.js
  Time (mean ± σ):      35.8 ms ±   1.9 ms    [User: 29.9 ms, System: 6.1 ms]
  Range (min … max):    33.3 ms …  42.6 ms    76 runs

Doing runInNewContext rather than creating a context manually and using that is a tiny bit faster, fwiw

Benchmark #1: node file.js
  Time (mean ± σ):      32.7 ms ±   1.6 ms    [User: 27.7 ms, System: 5.2 ms]
  Range (min … max):    30.2 ms …  39.7 ms    90 runs

I don't think those ms will make a huge difference compared to just loading jsdom itself

Benchmark #1: node -e "require(\"jsdom\")"
  Time (mean ± σ):     367.1 ms ±   4.9 ms    [User: 397.9 ms, System: 53.5 ms]
  Range (min … max):   358.6 ms … 375.6 ms    10 runs

This is with inlining the script into the runtime

Benchmark #1: node -e "require(\"jsdom\")"
  Time (mean ± σ):     372.8 ms ±   3.2 ms    [User: 398.7 ms, System: 56.2 ms]
  Range (min … max):   368.1 ms … 378.3 ms    10 runs

So about 5ms slower, but it fixes this bug where global env when js-globals.json is generated differs from the runtime globals.

---

Generating the file in a postinstall like suggested above would probably work as well, but it would still behave oddly when switching node versions. Less likely than a mismatch out of the gate, but still pretty brittle I think, and very hard to debug

[ianschmitz]

@domenic do you have any thoughts on which approach you prefer? I like @SimenB's suggestion but wanted to get your input before changing this PR.

[ianschmitz]

I've updated the PR to generate the globals at runtime.

[SimenB]

@domenic ping 🙂

[domenic]

This currently fails the build and does not have a test.

[ianschmitz]

@domenic i'm waiting for feedback on the approach.

[domenic]

It's hard to evaluate the approach since I don't know if it works (no new tests), or if it can be made to pass jsdom's existing tests. Sorry.

[SimenB]

I'm not sure if it's possible to write a proper test for this since it'll involve switching node versions. It will only fail when running an old node against the json file generated on a newer version of node.

The bug is best boiled down to this, I think

console.log(process.version);

global.globalThis = global;

const {JSDOM} = require('jsdom');

new JSDOM('', {runScripts: 'outside-only'});

console.log('Great success!');

Running with node 10 prints

v10.20.1
evalmachine.<anonymous>:1
globalThis
^

ReferenceError: globalThis is not defined
    at evalmachine.<anonymous>:1:1
    at Script.runInContext (vm.js:133:20)
    at Object.runInContext (vm.js:311:6)
    at setupWindow (/Users/simen/repos/hhhhhu/node_modules/jsdom/lib/jsdom/browser/Window.js:51:55)
    at new Window (/Users/simen/repos/hhhhhu/node_modules/jsdom/lib/jsdom/browser/Window.js:107:3)
    at exports.createWindow (/Users/simen/repos/hhhhhu/node_modules/jsdom/lib/jsdom/browser/Window.js:38:10)
    at new JSDOM (/Users/simen/repos/hhhhhu/node_modules/jsdom/lib/api.js:36:20)
    at Object.<anonymous> (/Users/simen/repos/hhhhhu/file.js:7:1)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)

Running with node 12 prints

v12.19.0
Great success!

---

One possible solution is to publish new versions of JSDOM on the oldest version of node you support, but then you'd have the opposite problem - globals that should be there on newer versions of node are not there

[domenic]

The bug is best boiled down to this, I think

This seems like a great test to add.

[SimenB]

I don't think it'd be a valid test since the issue is that the list of globals is generated at build time and might lead to a mismatch between what jsdom thinks should exist and what does exist, which explodes if some global it thinks exist is polyfilled. If the list is generated at runtime (like this PR implements) that's not an issue.

I guess the test might guard against a future optimization of again going with a static list? Not sure how valuable that's in practice?

---

CI is currently red due to attempting to run vm.runInNewContext in a browser. That raises the question - do you need the list of globals for some browser compatibility thing? If not just skipping the added test in the browser seems fine?

[domenic]

I don't think it'd be a valid test

I'm pretty confused. You said that you have a test which fails on Node v10 on jsdom master, and would pass on Node v10 with this patch applied. If that's the case, it's a valid test. Or am I missing something?

CI is currently red due to attempting to run vm.runInNewContext in a browser. That raises the question - do you need the list of globals for some browser compatibility thing? If not just skipping the added test in the browser seems fine?

Well, we need to make sure JSDOM continues to work in the browser, including all our existing tests. We shouldn't skip tests that pass today.

For the new test, i.e. "The bug is best boiled down to this, I think", that test should also be able to pass in the browser... I don't see a reason to skip it.

[SimenB]

I don't think it'd be a valid test

I'm pretty confused. You said that you have a test which fails on Node v10 on jsdom master, and would pass on Node v10 with this patch applied. If that's the case, it's a valid test. Or am I missing something?

I have a script that illustrates how the current approach fails (which I pasted in the comment above), but I don't think it can be converted to a proper test. It would not fail on jsdom master unless the prepare script is first executed using node 12 followed by the script being ran with node 10.

Well, we need to make sure JSDOM continues to work in the browser, including all our existing tests. We shouldn't skip tests that pass today.

Good point - it seems vm is shimmed today, I'm not sure why it fails now. Error is TypeError: vm.runInNewContext is not a function, but it's shimmed here: https://github.com/dfkaye/vm-shim/blob/b65eb2af267e944d908e7757daf139cbc559f919/vm-shim.js#L19