Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vm: Provide a way to replace the underlying global object of the GlobalProxy #31807

Closed
ExE-Boss opened this issue Feb 15, 2020 · 10 comments
Labels
feature request Issues that request new features to be added to Node.js. v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.

Comments

@ExE-Boss
Copy link
Contributor

ExE-Boss commented Feb 15, 2020

Is your feature request related to a problem? Please describe.

I’ve been looking into what’s needed to implement proper navigation in JSDOM.

This requires replacing the underlying [[Window]] instance of the WindowProxy.

Describe the solution you'd like

The desired solution would be an API exposed on vm.Context (#30709) that would perform a new makeContext call, replacing the underlying #global private field and [[Window]] private slot of the WindowProxy, while ensuring that the following test passes:

globalProxy.test.js

const { createWindow, navigateWindow } = require("./windows.js");
const symbolProp = Symbol("non-configurable")

const {
	vmContext,
	globalImpl,
	globalObject,
	globalProxy,
} = createWindow({ url: "http://example.org/" });

assert.strictEqual(globalProxy.origin, "http://example.org");

// `globalProxy[symbolProp]` doesn't exist:
assert.strictEqual(Object.getOwnPropertyDescriptor(globalProxy, symbolProp), undefined);
assert.strictEqual(Object.getOwnPropertyDescriptor(globalObject, symbolProp), undefined);

Object.defineProperty(globalProxy, symbolProp, { value: "non-configurable" });

// `globalProxy[symbolProp]` is now a non-configurable own property of `globalObject`:
assert.strictEqual(Object.getOwnPropertyDescriptor(globalProxy, symbolProp).configurable, false);
assert.strictEqual(Object.getOwnPropertyDescriptor(globalObject, symbolProp).configurable, false);

const {
	newGlobalImpl,
	newGlobalObject,
} = navigateWindow(vmContext, { url: "https://example.org/" });
const newGlobalProxy = vm.runInContext("this", vmContext);

assert.strictEqual(globalProxy, newGlobalProxy);
assert.strictEqual(globalProxy.origin, "https://example.org");
assert.notStrictEqual(globalObject, newGlobalObject);
assert.notStrictEqual(globalImpl, newGlobalImpl);

// `globalProxy[symbolProp]` doesn't exist anymore, as the proxy target is now `newGlobalObject`:
assert.strictEqual(Object.getOwnPropertyDescriptor(globalProxy, symbolProp), undefined);
assert.strictEqual(Object.getOwnPropertyDescriptor(newGlobalObject, symbolProp), undefined);

// `globalObject[symbolProp]` is still the old non-configurable own property:
assert.strictEqual(Object.getOwnPropertyDescriptor(globalObject, symbolProp).configurable, false);
windows.js (New API is called here)

function setupWindow(window, globalProxy, options, vmContext) {
	installInterfaces(window);
	Object.setPrototypeOf(window, window.Window.prototype);
	Window.setup(
		/* globalObject */ window,
		/* wrapperInstance */ window,
		/* constructorArgs */ undefined,
		/* privateData */ { globalProxy, options, vmContext }
	);
}

exports.createWindow = (options = {}) => {
	let vmContext = new vm.Context({ origin: computeOrigin(options.url) });
	let window = context.global;
	let globalProxy = vm.runInContext("this", vmContext);

	setupWindow(window, vmContext, globalProxy, options)
	return {
		vmContext,
		globalImpl: idlUtils.implForWrapper(window),
		globalObject: window,
		globalProxy
	}
}

exports.navigateWindow = (vmContext, options = {}) => {
	// New API is called here:
	vmContext.createNewGlobalObject({ origin: computeOrigin(options.url) });

	let window = context.global;

	setupWindow(window, vmContext, globalProxy, options);
	return {
		newGlobalImpl: idlUtils.implForWrapper(window),
		newGlobalObject: window
	}
}

Describe alternatives you've considered

  1. Just replace the WindowImpl instance.

    Pros:
    • Simple to implement.
    Cons:
    • Keeps global properties from previous runs.
    Conclusion:

    Viable only for when runScripts isn't enabled, though it might be better to use option 3 even in this case.

  2. Use Reflect.deleteProperty and WebIDLWindow.setup to try and reset as much of the Window global object as possible (won't be able to delete non‑configurable properties, such as the symbolProp shown in the above example).

    Pros:
    • Resets most of the global object.
    Cons:
    • Harder to implement.
    • Results in a global which is in a broken state.
    Conclusion:

    I’d rather implement alternative 3.

  3. Implement a custom WindowProxy that makes all properties configurable, while keeping track of which ones should be treated as non-configurable.

    Pros:
    • Supports fully re-setting the global object.
    Cons:
    Conclusion:

    I’ll probably implement this after Refactor Window to use WebIDL2JS jsdom/jsdom#2835 is merged for Node versions without vm: introduce vm.Context #30709.

@devsnek devsnek added feature request Issues that request new features to be added to Node.js. v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem. labels Feb 15, 2020
@devsnek
Copy link
Member

devsnek commented Feb 15, 2020

I'm fairly sure this is not a functionality that V8 provides, so there would need to be some upstream work first.

@Dragiyski
Copy link

Dragiyski commented Feb 15, 2020

While this is not something V8 provides, vm could expose at least something about the window's constructor. Currently creation of context do the following:

  1. javascript: provides (or create) a regular JavaScript object (known as contextified sandbox);
  2. node: creates a v8::FunctionTemplate and install access handlers, so any property of its instance is redirected to property of the contextified sandbox; This constructor is not exposed anywhere;
  3. node: creates a v8::Context with the InstanceTemplate() of the created v8::FunctionTemplate from step 2;
  4. v8: Retrieve the constructor of the provided template from step 3 and create an object from its prototype template (similar to Object.create(prototype)), so the constructor function is not invoked. That object becomes the JSGlobalObject.
  5. v8: Creates a proxy from JSGlobalObject which becomes the JSGlobalProxy.

This means that sandbox would not contain the global environment (you can easily access the JSGlobalProxy executing trueGlobal = vm.runInContext('globalThis', sandbox). However if you have class MyWindow and sandbox instanceof MyWindow is true, trueGlobal instanceof MyWindow won't be true.

What this feature request could do is just expose the function from step 2. V8 also allows for access checks callbacks, security token management and limiting the stack trace to the accessible context and those cannot be used on a sandbox object. For this feature request you would be able to install interfaces either on the prototype of the function from step 2 you still cannot replace the [[Window]] of the WindowProxy, but you can replace its prototype (before it is created).

@devsnek
Copy link
Member

devsnek commented Feb 15, 2020

The FunctionTemplate in node is an implementation detail. It shouldn't technically even need to exist, it's just the result of V8's very very odd implementation of globals.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 4, 2022
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Mar 4, 2022

Please keep this open.

@github-actions github-actions bot removed the stale label Mar 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2022

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Sep 5, 2022
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Sep 5, 2022

Please keep open.

@github-actions github-actions bot removed the stale label Sep 6, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 6, 2023
@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Mar 6, 2023

keep open

@github-actions github-actions bot removed the stale label Mar 7, 2023
@bnoordhuis
Copy link
Member

I'm going to close this because if no one is going to work on it, then there's no point in keeping it open - and bumping it is kind of annoying because that ends up sending emails to lots of people.

If you want to see this happen, you'll have to put in the work yourself, starting with V8.

@bnoordhuis bnoordhuis closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. v8 engine Issues and PRs related to the V8 dependency. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants