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

Assert that the global object inside the Shadow Realm is ordinary. #413

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

Ms2ger
Copy link
Collaborator

@Ms2ger Ms2ger commented Nov 15, 2024

@mhofman: please let me know if there's any requirements this doesn't capture.

@mhofman
Copy link
Member

mhofman commented Nov 19, 2024

If this is something we can actually assert, that would work for me. In this case we should likely also assert that the object is extensible and that all own properties are configurable.

I suppose the editor note for HostInitializeShadowRealm also needs updating.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 20, 2024

I don't think an assertion is the right tool here. Assertions are just editorial notes that explicitly state properties are an algorithm that are already true due to the algorithm itself. They are purely informative, and removing all assertions from the spec should not change its meaning.

Instead, we should pass a boolean requireOrdinaryGlobal (or isShadowRealm) to InitializeHostDefinedRealm(), and replace step 12 with

12. If _requireOrginaryGlobal_ is false and the host requires use of an exotic object to serve as realm's global object, then
   a. Let global be such an object created in a host-defined manner.
13. Else,
   a. If the host requires that the global object's prototype is not %Object.prototype%,
      i. Let _globalPrototype_ the object to use as the global object's prototype, created in a host-defined manner.
   b. Else,
       i. Let _globalPrototype_ be _realm_.[[Intrinsics]].[[%Object.prototype%]].
   c. Let _global_ be OrdinaryObjectCreate(_globalPrototype_).

By doing so we are actually requiring the host to create an ordinary object, rather than just asserting that it does without enforcing it, and we still allow the global to inherit from EventTarget.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Nov 20, 2024

Actually, looking at the HTML integration this would be easier for layering:

12. If the host requires use of a custom object to serve as _realm_'s global object, then
    a. If _requireOrdinaryGlobal_ is **false**, let _global_ be an object created for such purpose in a host-defined manner.
    b. Else, let _global_ be an ordinary object created for such purpose in a host-defined manner.
14. Else,
   c. Let _global_ be OrdinaryObjectCreate(_realm_.[[Intrinsics]].[[%Object.prototype%]]).

Also, note that the current proposed HTML integration defines the ShadowRealm global to behave exactly like an ordinary object, but it's not an ordinary object. Specifically, the [[SetPrototypeOf]] implementation is exotic and defined at https://webidl.spec.whatwg.org/#platform-object-setprototypeof. It however happens to behave exactly as the ordinary one, because is global prototype chain mutable returns true.

The reason this layering is simpler is because it's much easier for HTML to create a new object (for which it can use the WebIDL machinery) rather than to define new properties on an existing one.

@mhofman
Copy link
Member

mhofman commented Nov 21, 2024

the current proposed HTML integration defines the ShadowRealm global to behave exactly like an ordinary object, but it's not an ordinary object.

As I mentioned in #411, that's fine:

To clarify, while the host must not be allowed to observably use an exotic object, that requirement does not prevent an implementation to actually use an exotic object as global object, as long as the exotic behavior is observably indistinguishable from a regular object behavior.

The spec deals in observable behaviors. An implementation is always free to deviate from the letter of the spec as long as JS programs can never observe the deviation.

Assertions are just editorial notes that explicitly state properties are an algorithm that are already true due to the algorithm itself. They are purely informative, and removing all assertions from the spec should not change its meaning.

How do you recommend we constrain hosts on only adding configurable properties to the global object? In general how do we enforce invariants on host hooks?

@nicolo-ribaudo
Copy link
Member

How do you recommend we constrain hosts on only adding configurable properties to the global object? In general how do we enforce invariants on host hooks?

By not putting the "Assert:" annotation in front of it 😛 Normally for host hooks we say "the host must do X, Y and Z" rather than not stating requirements on what the host can do and then assert that when we previously called into the host it did X, Y and Z.

@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Nov 22, 2024

Also, note that the current proposed HTML integration defines the ShadowRealm global to behave exactly like an ordinary object, but it's not an ordinary object. Specifically, the [[SetPrototypeOf]] implementation is exotic and defined at https://webidl.spec.whatwg.org/#platform-object-setprototypeof. It however happens to behave exactly as the ordinary one, because is global prototype chain mutable returns true.

Fwiw, this is fixed in whatwg/webidl#1437

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

This works, together with tc39/ecma262#3498. Note that when opening the PR in the ECMA-262 repo, the new requirement should be added to InitializeHostDefinedRealm and not to HostInitializeShadowRealm.

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

Besides one minor correction, LGTM

spec.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Makes sense, LGTM with Mathieu's suggestion about "extensible"

Co-authored-by: Mathieu Hofman <[email protected]>
@Ms2ger
Copy link
Collaborator Author

Ms2ger commented Nov 25, 2024

Thanks!

@Ms2ger Ms2ger merged commit 629dd62 into tc39:main Nov 25, 2024
@Ms2ger Ms2ger deleted the assert-global-ordinary branch November 25, 2024 10:04
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.

4 participants