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

Missing calls to GetActiveScriptOrModule in PerformEval and EnqueueJob? #871

Open
anba opened this Issue Apr 5, 2017 · 6 comments

Comments

Projects
None yet
3 participants
@anba
Contributor

anba commented Apr 5, 2017

For indirect eval and EnqueueJob calls, ScriptOrModule of the new execution context resp. new job is set to null (*), but maybe it should instead use the active script/module? Functions created through CreateDynamicFunction use the active script/module. And the dynamic import proposal also expects ScriptOrModule is set to a non-null value in https://tc39.github.io/proposal-dynamic-import/#sec-import-call-runtime-semantics-evaluation.

(*) It's null because the running execution context is the exec-context of a built-in function, and ScriptOrModule of exec-contexts of built-in functions is always null (https://tc39.github.io/ecma262/#sec-createbuiltinfunction).

@bterlson

This comment has been minimized.

Show comment
Hide comment
@bterlson
Member

bterlson commented Apr 6, 2017

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Apr 7, 2017

Contributor

The example I had in my mind for the dynamic import proposal are:

// File: example.js
Promise.resolve(`import("./example.js").catch(e => print("caught", e))`)
       .then(eval);

and

// File: example.js
Promise.resolve(`import("./example.js").catch(e => print("caught", e))`)
       .then(Function)
       .then(Function.prototype.call.bind(Function.prototype.call));
Contributor

anba commented Apr 7, 2017

The example I had in my mind for the dynamic import proposal are:

// File: example.js
Promise.resolve(`import("./example.js").catch(e => print("caught", e))`)
       .then(eval);

and

// File: example.js
Promise.resolve(`import("./example.js").catch(e => print("caught", e))`)
       .then(Function)
       .then(Function.prototype.call.bind(Function.prototype.call));
@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Apr 24, 2017

Member

So to be clear, in those examples, it seems like GetActiveScriptOrModule() will not find anything on the stack where the ScriptOrModule component is not null, right? Yeah, I guess that is bad.

One fix might be that EnqueueJob() should be using GetActiveScriptOrModule() instead of getting the running execution context's Realm's ScriptOrModule. What do you think of that?

Do you think eval needs to be fixed separately, or does the above take care of both of your examples?

Member

domenic commented Apr 24, 2017

So to be clear, in those examples, it seems like GetActiveScriptOrModule() will not find anything on the stack where the ScriptOrModule component is not null, right? Yeah, I guess that is bad.

One fix might be that EnqueueJob() should be using GetActiveScriptOrModule() instead of getting the running execution context's Realm's ScriptOrModule. What do you think of that?

Do you think eval needs to be fixed separately, or does the above take care of both of your examples?

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba Apr 25, 2017

Contributor

So to be clear, in those examples, it seems like GetActiveScriptOrModule() will not find anything on the stack where the ScriptOrModule component is not null, right?

Correct.

One fix might be that EnqueueJob() should be using GetActiveScriptOrModule() instead of getting the running execution context's Realm's ScriptOrModule. What do you think of that?

I can't really tell, because I don't know where and when GetActiveScriptOrModule() is used (except for its use in the dynamic import proposal). Sorry!

Do you think eval needs to be fixed separately, or does the above take care of both of your examples?

It should be sufficient to change EnqueueJob, because there should always be another execution context on the stack with a non-null ScriptOrModule component when eval is executed.

Contributor

anba commented Apr 25, 2017

So to be clear, in those examples, it seems like GetActiveScriptOrModule() will not find anything on the stack where the ScriptOrModule component is not null, right?

Correct.

One fix might be that EnqueueJob() should be using GetActiveScriptOrModule() instead of getting the running execution context's Realm's ScriptOrModule. What do you think of that?

I can't really tell, because I don't know where and when GetActiveScriptOrModule() is used (except for its use in the dynamic import proposal). Sorry!

Do you think eval needs to be fixed separately, or does the above take care of both of your examples?

It should be sufficient to change EnqueueJob, because there should always be another execution context on the stack with a non-null ScriptOrModule component when eval is executed.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic May 1, 2017

Member

I looked into this but it seems this is just part of the general brokenness of EnqueueJob. In reality nobody implements it as specced; they let the host environment take care of the job queuing. So this is addressed by #735... which would indeed be great to get merged.

Member

domenic commented May 1, 2017

I looked into this but it seems this is just part of the general brokenness of EnqueueJob. In reality nobody implements it as specced; they let the host environment take care of the job queuing. So this is addressed by #735... which would indeed be great to get merged.

@anba

This comment has been minimized.

Show comment
Hide comment
@anba

anba May 2, 2017

Contributor

#735 doesn't initialize any of the new execution context's components, so I don't see how that PR helps the dynamic import proposal to assert that there's always a non-null ScriptOrModule component during the evaluation of import().

Contributor

anba commented May 2, 2017

#735 doesn't initialize any of the new execution context's components, so I don't see how that PR helps the dynamic import proposal to assert that there's always a non-null ScriptOrModule component during the evaluation of import().

domenic added a commit to whatwg/html that referenced this issue Oct 27, 2017

Ensure there is an active script while running JS jobs
Fixes tc39/ecma262#871, given that we have our
own version of EnqueueJob. Important for #3117.

domenic added a commit to whatwg/html that referenced this issue Oct 27, 2017

Ensure there is an active script while running JS jobs
Fixes tc39/ecma262#871, given that we have our
own version of EnqueueJob. Important for #3117.

domenic added a commit to whatwg/html that referenced this issue Oct 27, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.

domenic added a commit to whatwg/html that referenced this issue Nov 17, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.

domenic added a commit to whatwg/html that referenced this issue Nov 17, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.

domenic added a commit to whatwg/html that referenced this issue Nov 17, 2017

Ensure there is an active script while running JS jobs
"Fixes" tc39/ecma262#871, at least for HTML,
given that we have our own version of EnqueueJob. Important for #3117.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment