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

Let the host create the global object #392

Closed
wants to merge 4 commits into from

Conversation

Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Dec 15, 2023

The ShadowRealm constructor (<emu-xref href="#sec-shadowrealm"></emu-xref>)
creates a new global object as an ordinary object. This
means all properties from the global object are deletable.
The host is expected to create global object whose properties are all deletable.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The host is expected to create global object whose properties are all deletable.
The host is expected to create a global object whose properties are all deletable.

Although I think this note is redundant, the paragraph above already states:

The host may use this hook to add properties to the ShadowRealm's global object. Those properties must be configurable.

...and configurable means deletable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@linusg with historical facts from Ecmascript and Web, it may seem obvious, but not necessarily. Hence @mhofman - along with the proposal champions - emphasizing on the Global being an ordinary object.

@mhofman
Copy link
Member

mhofman commented Dec 15, 2023

This change looks like a regression. It seems that the host gains the ability to create a global object with exotic behavior. In particular the main requirement is that all properties must be effectively deletable. I also believe the intent was for the prototype of the global object to be effectively mutable, and that the global object could be successfully frozen. The reason I mention "effectively" is that an exotic object can claim that properties are configurable, yet refuse to actually mutate or delete them.

@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Dec 18, 2023

I'm happy to add restrictions on the host, but would prefer if someone with more background suggested prose for it.

@mhofman
Copy link
Member

mhofman commented Jan 23, 2024

#394 raises an interesting point about discovery of the host added properties. If the host creates the object, it should implement all added properties as own properties.

I think basically the constraint should be that all behavior implemented by the host created global object be observably regular / non-exotic. In this case the object invariants are not sufficient as they provide too much leeway. At which point I'm still curious why we need this change since a host created object with regular object behavior, and a spec created regular object are observably indistinguishable, and can be considered an internal implementation concern.

Copy link
Collaborator

@caridy caridy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ByteEater-pl
Copy link

ByteEater-pl commented Jan 30, 2024

@mhofman, could @ljharb's "Get Intrinsic" Proposal help reconcile these concerns?

We could use it to enumerate the global object's properties in a ShadowRealm and delete all or some of them (because they're required to be configurable, though that requirement would have to be strengthened so that the delibility isn't faked), e.g. just those added by the host.

And hosts with EventTarget wishing to have ShadowRealms' global objects inherit from it would be free to do so.

@Jack-Works
Copy link
Member

I oppose to host creating the global object. HostInitializeShadowRealm is enough.

Without userland ShadowRealm API to do the same thing, an exotic global object is unacceptable.

Can you give the reason why the host should take over the creation?

@mhofman
Copy link
Member

mhofman commented Jan 30, 2024

could @ljharb's "Get Intrinsic" Proposal help reconcile these concerns?

I don't see how that proposal would help. The concern is not with the ability to enumerate globals, but with requiring that the global object has non-exotic behavior.

And hosts with EventTarget wishing to have ShadowRealms' global objects inherit from it would be free to do so.

Why is the host creating the global object required for this? The prototype of a fresh regular object is mutable. Nothing in the original steps says the global object has to inherit from Object.prototype. Also as I mentioned, whether in the actual implementation the host does in fact create the global object is unobservable, as long as the global object can be observed to have no exotic behavior.

@mhofman mhofman requested a review from caridy January 30, 2024 11:58
@Jack-Works
Copy link
Member

The host can set the global object's prototype to EventTarget.prototype in HostInitializeShadowRealm. The host can also attach the required internal slot of EventTarget to make it functional, this is virtualizable since a non-Web implementation can also do that by using their own fake "EventTarget" that accepts the global object as a valid this.

@ByteEater-pl
Copy link

@mhofman, you wrote:

If the host creates the object, it should implement all added properties as own properties.

If instead it creates a regular object as global and operates on it in HostInitializeShadowRealm (whereby setting its prototype, not just adding properties, should therefore be mentioned as an example of what's allowed and likely useful), what stops it from setting an exotic object as a prototype? If it does, and "use this hook to add properties to the ShadowRealm's global object" is interpreted (correctly, as we both seem to assume) as including inherited properties (to which the configurability requirement applies), we're back in square one, since the prototype (or any object up in the prototype chain) can:

  • fake delibility,
  • effectively deny enumerability.

What I mean by the (actually understated; worse things can happen) latter is, assuming that in the absence of Get Intrinsic (which I therefore mentioned) you'd traverse the prototype chain with getPrototypeOf (on Object or Reflect) and enumerate everything available, some object on the prototype chain may have a custom [[GetPrototypeOf]], which opens possibilities (not prevented by invariants of the essential internal methods) such as returning a different prototype to the enumerating code than in normal operation, sporting or lazily creating an infinite prototype chain, adding properties (even non-configurable ones) to the global object itself (or any ancestor in the prototype chain), throwing, or simply not terminating.

@ByteEater-pl
Copy link

Or maybe it's fine to change the prototype of the global object first (it's ordinary, so possible), thereby loosing its EventTarget quality, and then turn to deleting what you don't want on the object itself.

But even then, I believe the requirements of HostInitializeShadowRealm should be strengthened. If it's currently understood that it can set the global object's prototype, it probably can also delete, replace or reconfigure (even make non-configurable) the properties installed by SetDefaultGlobalBindings. In this and other ShadowRealms. And mangle your module graph (outside the ShadowRealm). And make demons fly out of your nose. Yea, I know other host operations aren't usually so carefully limited, but this one is special, security hinges on it.

@mhofman
Copy link
Member

mhofman commented Jan 30, 2024

what stops it from setting an exotic object as a prototype?

That's fine, as long as the global object remains extensible, the prototype object can be substituted, which is sufficient.

including inherited properties (to which the configurability requirement applies)

The configurability requirement does not apply to the prototype chain, only to the global object.

you'd traverse the prototype chain with getPrototypeOf (on Object or Reflect) and enumerate everything available

Nope, you'd likely replace the prototype of the global object with a wrapper or other object that behaves as expected.

Or maybe it's fine to change the prototype of the global object first (it's ordinary, so possible), thereby loosing its EventTarget quality, and then turn to deleting what you don't want on the object itself.

Right, but the user doesn't have to lose the EventTarget methods, that prototype can be inserted in the chain, or a wrapper for it, etc.

But even then, I believe the requirements of HostInitializeShadowRealm should be strengthened.

even make non-configurable) the properties installed by SetDefaultGlobalBindings

I agree, HostInitializeShadowRealm should not be allowed to add or make any properties of the global object non-configurable, nor should it be allowed to make the global object non-extensible.

In this and other ShadowRealms

Yes, I wanted to have some text somewhere that prevented the host from making properties appear or otherwise change properties on the global object after HostInitializeShadowRealm, but I can't track down where I asked for this.

And mangle your module graph (outside the ShadowRealm). And make demons fly out of your nose. Yea, I know other host operations aren't usually so carefully limited, but this one is special, security hinges on it.

I honestly don't care about what the host does outside the shadow realm after its creation, as long as it respects the callable boundary invariants (which are now spelled out precisely) and the above requirements to not further change the global object after initialization.

@Jack-Works
Copy link
Member

we're back in square one, since the prototype (or any object up in the prototype chain) can:

fake delibility,
effectively deny enumerability.

that's ok, because you can do it in the user land with Proxy. but you cannot make the global object itself becomes a Proxy.

If it's currently understood that it can set the global object's prototype, it probably can also delete, replace or reconfigure (even make non-configurable) the properties installed by SetDefaultGlobalBindings.

I agree. It is (if not, it must is) a formal requirement to implementations that all properties are configurable, even if they installed it in HostInitializeShadowRealm.

1. Perform SetRealmGlobalObject(_realmRec_, *undefined*, *undefined*).
1. Perform ? SetDefaultGlobalBindings(_O_.[[ShadowRealm]]).
1. Perform ? HostInitializeShadowRealm(_O_.[[ShadowRealm]]).
1. Let _realmRec_ be the Realm of _context_.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ms2ger I'm confused about this line, and the title of this PR. Who is creating the realm now?

Copy link
Member

@linusg linusg Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateRealm was folded into InitializeHostDefinedRealm in tc39/ecma262#3139, it creates a realm, customizable global object, and execution context which is pushed to the stack. This may be changed again slightly in tc39/ecma262#3274.

Copy link
Member

@mhofman mhofman Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also a little confused, and still don't understand the purpose of this PR. For ShadowRealm we need a global object with observably non-exotic behavior. Why can't we just let the 262 spec pretend it created a regular object to use as global, and have implementations actually implement it differently if they want to have their global object with exotic behavior that is non observable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In tc39/ecma262#3139 I see the following:

If the host requires use of an exotic object to serve as realm's global object, then

Can we in any way constraint here that the host is not allowed to use an exotic object for the global object of ShadowRealm? And thus pretend that observably we took the path of the Else and created a regular object for the global?

Copy link
Collaborator

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @mhofman 's comments in this PR's conversations.

@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Oct 24, 2024

I'll come back to this after #409

@caridy
Copy link
Collaborator

caridy commented Oct 30, 2024

@Ms2ger now that #409 is closed, can we get back to this and figure out what to do?

shannonbooth added a commit to shannonbooth/ladybird that referenced this pull request Nov 2, 2024
As described in the comments, this was a bug in the shadow realm spec.
For now, just apply a fixup to the execution context so that it works as
intended.

Once tc39/proposal-shadowrealm#392 is updated to
match these changes to the spec we should be able to implement this in a
better way :^)
shannonbooth added a commit to shannonbooth/ladybird that referenced this pull request Nov 2, 2024
As described in the comments, this was a bug in the shadow realm spec.
For now, just apply a fixup to the execution context so that it works as
intended.

Once tc39/proposal-shadowrealm#392 is updated to
match these changes to the spec we should be able to implement this in a
better way :^)
shannonbooth added a commit to shannonbooth/ladybird that referenced this pull request Nov 2, 2024
As described in the comments, this was a bug in the shadow realm spec.
For now, just apply a fixup to the execution context so that it works as
intended.

Once tc39/proposal-shadowrealm#392 is updated to
match these changes to the spec we should be able to implement this in a
better way :^)
shannonbooth added a commit to shannonbooth/ladybird that referenced this pull request Nov 2, 2024
As described in the comments, this was a bug in the shadow realm spec.
For now, just apply a fixup to the execution context so that it works as
intended.

Once tc39/proposal-shadowrealm#392 is updated to
match these changes to the spec we should be able to implement this in a
better way :^)
shannonbooth added a commit to shannonbooth/ladybird that referenced this pull request Nov 2, 2024
As described in the comments, this was a bug in the shadow realm spec.
For now, just apply a fixup to the execution context so that it works as
intended.

Once tc39/proposal-shadowrealm#392 is updated to
match these changes to the spec we should be able to implement this in a
better way :^)
@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Nov 4, 2024

I'm looking into this; at this point it seems like this PR might not be necessary.

@Ms2ger Ms2ger closed this Nov 4, 2024
@mhofman
Copy link
Member

mhofman commented Nov 4, 2024

I'm looking into this; at this point it seems like this PR might not be necessary.

That's because other PRs completely absorbed the original change from this PR. However the concerns I raise are valid more than ever. I will open an issue that must be addressed before proceeding to stage 3.

@mhofman
Copy link
Member

mhofman commented Nov 4, 2024

Filed #411

@mhofman mhofman reopened this Nov 4, 2024
@mhofman mhofman closed this Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants