Editorial: Create the realm execution context slightly later #3497
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
That is, in InitializeHostDefinedRealm, take the steps that create + push the realm execution context, and move them to after the realm's fields have been finalized.
Comment on lines
11807
to
11808
| 1. Perform ? SetDefaultGlobalBindings(_realm_). | ||
| 1. Create any host-defined global object properties on _global_. |
There was a problem hiding this comment.
Is the only reason we can't also move these lines up because SetDefaultGlobalBindings might throw? If so, should we (normatively) reconsider whether those property definitions should be permitted to throw?
There was a problem hiding this comment.
Is the only reason we can't also move these lines up because
SetDefaultGlobalBindingsmight throw?
Well, the only other possible reason would be that those remaining steps need the realm EC to exist (and maybe that it be on the EC stack). That isn't the case if the global object is ordinary, but if it's a host-defined exotic object, then I suppose its relevant internal methods could be specified to access the realm EC.
michaelficarra
approved these changes
Nov 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
InitializeHostDefinedRealm(IHDR), in addition to creating a realm, also creates an execution context (EC) to house it, and pushes that EC onto the EC stack. In the status quo, the EC-related steps appear in the midst of the realm-creation steps, which is odd, and doesn't appear to be necessary. (You might think that the later realm-creation steps somehow assume that the realm EC exists and has been pushed on the EC stack, but as far as I can tell, they don't. I think it's just an artifact of how IHDR was introduced in ES6.)This PR moves the EC-related steps to a point after the realm's fields have been finalized.
(This makes it easier to see that the early settings of
[[GlobalObject]]and[[GlobalEnv]]to*undefined*are superfluous. I would have deleted those steps, but that overlaps with PR #3445.)I would have preferred to move the EC-related steps to just before the Return step. However, that would be a change in behavior. The invocation of
SetDefaultGlobalBindingscan return an abrupt completion, which is immediately returned by IHDR. In such a case, the status quo says that the EC-related things have happened, which is a side-effect that can be observed by IHDR's caller. In particular, when the HTML spec invokes IHDR, it ignores the return value; it fetches the realm EC from the EC stack regardless of whether IHDR returned an abrupt completion.(Mind you, I think the only way that
SetDefaultGlobalBindingscould throw is if IHDR's caller provided a global object that was specifically designed to make that happen.)I suggested this change in #3445 (comment)
This should probably have been a quick follow-up to PR #3139.
See also PR #3274, which considers changing some of the linkage between IHDR and its caller.