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

Fix and expand incumbent settings object definition #1189

Merged
merged 2 commits into from Jun 15, 2016

Conversation

4 participants
@domenic
Member

domenic commented May 4, 2016

As discussed in #473, starting especially from around
#473 (comment), the
definition of incumbent introduced in #401 falls down in certain
important cases. In order to fix this, we introduce the "backup
incumbent settings object stack", which takes care of these trickier
cases. This commit also adds a few examples of how exactly incumbent
settings object calculation works, especially in the case where the
backup incumbent settings object stack ends up mattering.

For this story to be fully coherent, we will also need the minor fixes
found in tc39/ecma262#556, as well as further revisions to Web IDL to
update it for this new framework. This commit is the first step,
however.

@bzbarsky review appreciated!!

source Outdated
</div>
<div class="example">
<p>Consider the following more complicated example:</p>

This comment has been minimized.

@annevk

annevk May 5, 2016

Member

I would group these divs into one and start this paragraph with "Now".

This comment has been minimized.

@domenic

domenic May 5, 2016

Member

They're separate examples though...

This comment has been minimized.

@annevk

annevk May 5, 2016

Member

Yeah never mind.

@domenic

This comment has been minimized.

Member

domenic commented May 11, 2016

Ping @bzbarsky to review if possible :)

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented May 11, 2016

This is totally on my list to review, but likely not happening until tomorrow or Friday.

source Outdated
<p class="note">This assert would fail if you try to obtain the <span>incumbent settings
object</span> from inside an algorithm that was triggered neither by <a
href="#calling-scripts">calling scripts</a> nor by Web IDL <span
data-x="es-invoking-callback-functions">invoking</span> a callback. For example, it would

This comment has been minimized.

@bzbarsky

bzbarsky May 18, 2016

Collaborator

Aren't Promise jobs an example of something that is not triggered by either one of those? Again, if we think this incumbent thing is needed we need to make it play nice with Promise jobs too...

This comment has been minimized.

@domenic

domenic May 19, 2016

Member

You're right. But, good news: handling this turned out to be easier than I thought. I've pushed a second commit which I think takes care of the issue. I'd appreciate a review.

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented May 18, 2016

This generally looks great, thanks!

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented May 18, 2016

Oh, and we presumably still need IDL changes here too, right, to push/pop stuff onto this stack?

@bzbarsky bzbarsky removed their assignment May 18, 2016

@domenic

This comment has been minimized.

Member

domenic commented May 19, 2016

Oh, and we presumably still need IDL changes here too, right, to push/pop stuff onto this stack?

Yeah, that's still needed. Hoping to land the entry stuff first, then do incumbent as a follow-up.

@domenic

This comment has been minimized.

Member

domenic commented May 20, 2016

Ping @bzbarsky to review again now that I think I've fixed promise jobs as well.

If you prefer the diff there are two separate commits.

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented May 23, 2016

Thank you. So I was reading through this again carefully... and I'm not sure it works as we have it now. Consider the case when script in document A adds a bound builtin function as an event listener to a node. Then script in document B calls click() on that node. What is the incumbent when the builtin is called?

Per proposed spec, what will happen? GetActiveScriptOrModule() will look at the execution context stack. There's something there that we pushed when preparing to run the script for the event listener, but it has no ScriptOrModule, right? So we keep going up the execution context stack and find the script that called click(). The backup incumbent stack is not consulted at all in this situation, right?

The EnqueueJob bits look ok, except for the obvious problem of being a monkeypatch. :( I'm not sure what we can do about that, though, short of the ES spec admitting that someone else might be managing the job queue.

@domenic domenic assigned domenic and unassigned bzbarsky May 28, 2016

@domenic

This comment has been minimized.

Member

domenic commented Jun 1, 2016

Sorry for the delay! TC39 week and subsequent recovery is my excuse.

For the record I translated your prose into an example, which I'll dump here for reference:

<!-- a.html -->
<!DOCTYPE html>
<button>click me</button>
<script>
const bound = window.postMessage.bind(window, "some data", "*");
document.querySelector("button").addEventListener("click", bound);
</script>

<!-- b.html -->
<!DOCTYPE html>
<iframe src="a.html"></iframe>
<script>
  const iframe = document.querySelector("iframe");
  iframe.onload = () => {
    iframe.contentWindow.document.querySelector("button").click();
  };
</script>

I agree that the spec in this PR will set the incumbent settings object while postMessage runs to that of document B. Is that incorrect? It matches the intuitive notion of "the most-recently-entered author function or script on the stack". It also seems congruent with the example in this PR, with click() taking the place of that example's setTimeout, as in both are ways to indirectly invoke some built-in code.

Of course I'm not arguing that we should change implementations' concept of incumbent settings object to match my intuition; since we're considering it largely legacy anyway, I just want to match what was in place before. So, assuming document B is not what implementations do, could you give any suggestions for better intuition? Perhaps even suggested algorithms, based on what Firefox does?

The EnqueueJob bits look ok, except for the obvious problem of being a monkeypatch. :( I'm not sure what we can do about that, though, short of the ES spec admitting that someone else might be managing the job queue.

I've actually talked about this with the editor, and he's amenable to that. It just hasn't been too high priority to fix yet. So there's hope :)

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 1, 2016

Is that incorrect?

It's incorrect if we want the "you can't change what incumbent a function to some other code that is not you merely by running it async through some platform API" invariant.

It also seems congruent with the example in this PR, with click() taking the place of that example's setTimeout, as in both are ways to indirectly invoke some built-in code.

The thing that takes the place of setTimeout here is the addEventListener call. That's where the function is set up for async execution.

Note that "legacy" is not a useful argument here because I think only Gecko actually implements the invariant I describe above. Again, that invariant is an absolute must-have if the incumbent global is ever used for anything security-related. If it's not, then it probably doesn't matter very much what the incumbent global actually is in various edge cases...

What Firefox does is the following:

  1. There is a JavaScript callstack. The things on this callstack have various information associated with them, and more or less correspond to the JS Execution Contexts of ES6. One of the bits of information associated with things on this stack is whether there is a "scripted caller override" (Firefox internal terminology) in place for that stack frame. This is implemented as a counter.
  2. There is a separate incumbent settings stack.
  3. When something is pushed on the incumbent settings stack, the top (non-builtin?) frame on the JavaScript stack has its scripted caller override counter incremented. The interaction with builtins here is not obvious to me, not least because of the "more or less" aspect from item 1. In particular, the thing that gets marked might in fact encompass several JS frames. But I think the upshot is that the scripted caller override for the topmost non-builtin frame, if any, is incremented. That's certainly the intent.
  4. When something is popped off the incumbent settings stack, the scripted caller override counter is decremented for the top (non-builtin?) frame on the JavaScript stack.
  5. To get the incumbent global, we examine the top non-builtin frame on the JavaScript stack. If its scripted caller override counter is nonzero, we return the top thing on the incumbent settings stack. Otherwise, we return the settings corresponding to the thing on the JavaScript stack.
@domenic

This comment has been minimized.

Member

domenic commented Jun 1, 2016

When are things pushed/popped onto the incumbent settings stack? Is it equivalent to the spec's backup incumbent settings object stack, i.e. it's manipulated by Web IDL invoking things?

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 1, 2016

When are things pushed/popped onto the incumbent settings stack?

When Web IDL callbacks are invoked. Each Web IDL callback stores the incumbent settings object from its creation and pushes it onto the incumbent settings stack before running. So yes, I think it's equivalent to the backup incumbent settings stack.

@domenic

This comment has been minimized.

Member

domenic commented Jun 1, 2016

OK, great. With that in mind, here's my draft for how to just copy Firefox's approach. I can't imagine anything we come up with will be substantially better so that seems like a good idea.

Add incumbent counter to code evaluation state for every execution context (not just realm execution contexts), initially zero.

Add operation: set up for invoking a callback callback with environment settings object settings

  1. Prepare to run script with settings
  2. Let callback settings be callback's callback context.
  3. Push callback settings onto the backup incumbent settings object stack.
  4. Let ec be the topmost execution context on the JavaScript execution context stack whose ScriptOrModule component is not null.
  5. If ec exists, increment ec's incumbent counter.

Add operation: clean up after invoking a callback callback with environment settings object settings

  1. Let ec be the topmost execution context on the JavaScript execution context stack whose ScriptOrModule component is not null.
  2. If ec exists, decrement ec's incumbent counter.
  3. Let callback settings be callback's callback context.
  4. Pop callback settings off of the backup incumbent settings object stack.
  5. Clean up after running script with settings

Then, change Web IDL's current usage of "Prepare to run script with settings" and "clean up after running script with settings" usage inside its four invoke-callback algorithms to use these instead.

Also, the PromiseReactionJob patch would need to add similar steps to manage the incumbent counter.

I guess we'd then delete GetActiveScriptOrModule() from ES as it's no longer used.


Let me know what you think of the above draft. If it makes sense to you, I'll work on turning it into a real pull request this week.

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 1, 2016

That, plus changes to how the actual incumbent settings is determined, makes sense. When popping off the backup incumbent settings object stack, we should be able to assert that the topmost thing is the callback's callback context....

@domenic

This comment has been minimized.

Member

domenic commented Jun 2, 2016

That, plus changes to how the actual incumbent settings is determined, makes sense.

Oh, right, of course, I forgot that part. Is this correct? Seems a little sketchy...

  1. Let ec be the topmost execution context on the JavaScript execution context stack whose ScriptOrModule component is not null.
  2. If ec exists and ec's incumbent counter is greater than zero, or if ec does not exist:
    1. Assert: the backup incumbent settings object stack is nonempty. (+ explanatory note explaining why this is true)
    2. Return the topmost entry on the backup incumbent settings object stack.
  3. Return ec's Realm component's settings object

(Maybe "incumbent counter" is not great, since it's used to determine whether it's not the incumbent... suggestions appreciated.)

When popping off the backup incumbent settings object stack, we should be able to assert that the topmost thing is the callback's callback context....

That sounds like a good tweak. I'll replace step 4 of the cleanup with

  1. Assert: the topmost entry of the backup incumbent settings object stack is callback settings
  2. Pop callback settings off of the backup incumbent settings object stack.
@domenic

This comment has been minimized.

Member

domenic commented Jun 3, 2016

OK, so, I think I've got this all written up. Rendered versions:

I've also submitted a Web IDL pull request at heycam/webidl#128. (There's no real order in which to land or review the two; they basically need to be landed at the same time.) You can view the rendered version of that as well. If you follow the dfn.js popups starting at callback context you should see all the changes.

@domenic domenic assigned bzbarsky and unassigned domenic Jun 3, 2016

@domenic

This comment has been minimized.

Member

domenic commented Jun 9, 2016

Ping @bzbarsky to review this and heycam/webidl#128.

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 9, 2016

Sorry for the lag on this. :(

The incumbent section looks good to me.

The EnqueueJob section I'm not quite following: it's talking about "job.[[Realm]]" but a Job has no [[Realm]] field. A PendingJob record does, but that's what EnqueueJob is supposed to create and the part we're overriding...

In terms of the actual behavior we want here, by the way, I would think we want the entry settings to be the same when f is called in these two scenarios:

someOtherWindow.setTimeout(f);

someOtherWindow.Promise.resolve().then(f);

In the first case, the entry settings corresponds to the Realm of f. In the second case, ideally it would be the same thing. The problem is that EnqueueJob doesn't really have that information...

Oh, and the [[Realm]] of the PendingJob as defined in ES6 is also not amazing. In my testcase above it's the Realm of the then function, as far as I can tell: The promise is fulfilled when PerformPromiseThen is called, so we EnqueueJob right there and end up with the [[Realm]] of the PendingJob being set to the Realm of the "running execution context". But if I were resolving a Promise via its constructor resolve method I think it would be the [[Realm]] of that method, and if we have something like:

somePromise.then((x) => x).then(f)

followed by resolving somePromise, then we have to start digging into what Realm we're in when we resolve the promise that is the return value of the first then call, which is basically going to depend on what SpeciesConstructor did on somePromise (i.e. what Realm the function it returned came from) and may have no bearing on either of the then functions involved or f. Note that in particular in the ES spec the [[Realm]] of the PendingJob for a PromiseReactionJob depends on whether the promise is resolved before or after the then call, unless I'm missing something here. Which means I really hope it's just not observable by anything...

@domenic

This comment has been minimized.

Member

domenic commented Jun 9, 2016

The EnqueueJob section I'm not quite following: it's talking about "job.[[Realm]]" but a Job has no [[Realm]] field. A PendingJob record does, but that's what EnqueueJob is supposed to create and the part we're overriding...

Oh, dang, that's obviously wrong; thank you. We should be able to ignore PendingJobs, and their problematic [[Realm]] fields, entirely.

So the problem is we don't have the proper settings with which to "prepare to run a script", in order to make entry settings work. But this is solvable. We just want to do the thing analogous to what Web IDL does, which is grab the realm of the callback. EnqueueJob doesn't have that information right now, it's true. But we can patch ES to give it to us.

I'm not sure exactly how to do this, however:

  • For PromiseReactionJob, it should be probably reaction.[[Handler]]'s [[Realm]], if the handler is not undefined. But if the handler is undefined, e.g. PromiseSubclass.resolve().then(undefined, onRejected), this could still invoke user code: with a subclass, promiseCapability.[[Resolve]] is whatever PromiseSubclass passed up during its constructor. What do we do?
  • For PromiseResolveThenableJob, we have an obvious candidate of then's [[Realm]]. But again, we could potentially execute the newly-created resolvingFunctions.[[Reject]]. Is it OK to do so with an entry realm equal to then's [[Realm]]?

These problems feel very similar to those faced by the { handleEvent } case :(. I'd be interested to hear what Firefox does.

I guess the attraction of the PendingJob [[Realm]] is that it basically just grabs the current realm and bypasses all this heartache, but as you said that leads to some weird scenarios.

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 9, 2016

For PromiseReactionJob seems to me like we should just use promiseCapability.[[Resolve]] as the thing to get the entry settings from. It doesn't much matter whether it was explicitly passed in to then or came from subclass constructors or whatever, right?

I'm pretty sure that's what Gecko does: take the actual callable we plan to invoke, and use it to get the entry settings.

For PromiseResolveThenableJob, I agree that the obvious candidate is then's [[Realm]]. Pretty sure that this is what Gecko does right now. We really need tests for all this stuff.... :(

But again, we could potentially execute the newly-created resolvingFunctions.[[Reject]].

Yes, but that comes from CreateResolvingFunctions and hence is not user code and will not directly invoke user code, afaict. In fact, the fact that it's a "built-in function" at all, as opposed to an abstract operation, is just a specification device so you can meaningfully talk about holding on to it. All that stuff could be rewritten in terms of abstract operations too, as far as I can tell.

I think the only real headache with promises is whether we can land in a promise resolve function somehow without already having an entry settings on the stack. I hope not, because it does a Get(resolution, "then") in there.... I'm actually not sure what happens in Gecko here; afaict we just do the Get and may well be doing it without ensuring there is an entry settings. :(

@domenic

This comment has been minimized.

Member

domenic commented Jun 9, 2016

For PromiseReactionJob seems to me like we should just use promiseCapability.[[Resolve]] as the thing to get the entry settings from. It doesn't much matter whether it was explicitly passed in to then or came from subclass constructors or whatever, right?

I'm pretty sure that's what Gecko does: take the actual callable we plan to invoke, and use it to get the entry settings.

The problem is you could invoke either, or both. Consider this more in-depth example:

class SubPromise extends Promise {
  constructor(executor) {
    super((builtInResolve, builtInReject) => {
      executor(
        function a(x) {
          console.log("a");
          resolve(x + 1); // (*)
        },
        function b(x) {
          console.log("b");
          reject(x - 1);
        }
      );
    });
  }
}

const p1 = SubPromise.resolve(5).then(function c() { throw new Error("argh"); });
const p2 = SubPromise.resolve(5).then(undefined);

In both cases, PromiseReactionJob is enqueued at (*). For the p1 case, reaction.[[Handler]] is c, and we also invoke b. In the p2 case, reaction.[[Handler]] is undefined, and we invoke a.

Do you think it's OK to just use a's realm in all cases? Even if it mismatches c's realm in the p1 case? In the p1 case, c's realm seems better...

Yes, but that comes from CreateResolvingFunctions and hence is not user code and will not directly invoke user code, afaict.

Oh, you're right. OK, that's nice...

I think the only real headache with promises is whether we can land in a promise resolve function somehow without already having an entry settings on the stack. I hope not, because it does a Get(resolution, "then") in there....

I don't believe you can, as long as we plug the gaps in EnqueueJob. (Promise resolve functions are the default [[Resolve]] of a promise capability object, so if we don't, we're in trouble.)

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 9, 2016

For the p1 case, reaction.[[Handler]] is c, and we also invoke b

In Gecko right now, I believe we invoke c with the entry settings corresponding to c's [[Realm]]. Then if it throws we invoke b with the entry settings corresponding to b's [[Realm]]. That's because we actually treat both c and b as Web IDL callbacks at the moment and hence the call to each one has its own setup and teardown.

In the new setup being worked on that moves Promises out of Gecko and into SpiderMonkey... I'm not actually sure. I'll check....

I don't believe you can, as long as we plug the gaps in EnqueueJob.

It's possible to resolve promises not off a job, right? I mean, in specs that are not ES that are using Promises.

@domenic

This comment has been minimized.

Member

domenic commented Jun 9, 2016

That's because we actually treat both c and b as Web IDL callbacks at the moment and hence the call to each one has its own setup and teardown.

Yeah, this would be hard to do from a spec point of view :(. Curious what your investigations turn up for the SpiderMonkey version...

It's possible to resolve promises not off a job, right? I mean, in specs that are not ES that are using Promises.

Ugh, I think you are right. In most cases they are being resolved with newly-created objects, not ones that could have then properties. But it's certainly not rigorous... :(

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 9, 2016

Curious what your investigations turn up for the SpiderMonkey version

That code is still being reviewed, but it looks to me that in the new setup's proposed patch at https://bug911216.bmoattachments.org/attachment.cgi?id=8761429 PromiseReactionJob will use the entry settings for handler if there is one. If there isn't one.... I think it uses the entry settings of the current Realm at the point when we're creating the Job. I have no idea what that is in practice in various cases, and whether it's even reliably the same thing all the time for a given set of promises, independent of resolution times. :(

domenic added some commits May 4, 2016

Fix and incumbent settings object definition and add examples
As discussed in #473, starting especially from around
#473 (comment), the
definition of incumbent introduced in #401 falls down in certain
important cases. In order to fix this, we introduce several new
concepts, which takes care of these trickier examples. These examples
are now included in the spec, and spell out exactly how exactly
incumbent settings object calculation works in increasingly-complex
scenarios.

The new algorithms "prepare to run a callback" and "clean up after
running a callback" will be used by Web IDL, similarly to how it already
uses "prepare to run script" and "clean up after running script."

Another notable change is that EnqueueJob now correctly tracks the
necessary goings-on in order to make the incumbent settings object work
correctly when promises are used to schedule callbacks.
@domenic

This comment has been minimized.

Member

domenic commented Jun 14, 2016

OK, so in the interests of moving this forward, I've changed it to the following:

Let job settings be some appropriate environment settings object.

Warning: It is not yet clear how to specify the environment settings object that should be used here. In practice, this means that the entry concept is not correctly specified while executing a job. See discussion in issue #1189.

I think that should be enough to make incremental progress, and merge this and the counterpart Web IDL PR. Let me know if you disagree.

@bzbarsky

This comment has been minimized.

Collaborator

bzbarsky commented Jun 15, 2016

That seems fine, but we should have a followup issue for sorting Jobs out.

@domenic domenic merged commit f97c3e4 into master Jun 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@domenic domenic deleted the fix-incumbent-settings branch Jun 15, 2016

@jeisinger

This comment has been minimized.

Member

jeisinger commented Mar 7, 2017

fwiw v8 tracks the Realm the promise constructor came from that was used to create the promise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment