Global getters leak the global object, instead of the global this value?

[domenic]

Consider the code

window.__defineGetter__("test", function () {
  return window === this;
});

Here window is the global this value. The question is, does the statement

test;

return true or false? The answer really should be true, because otherwise we are leaking the actual global object to script, and hiding the actual global object is a security invariant in browsers.

But reading the spec, I am getting false. Here is the reasoning:

• We start in Runtime Semantics: Evaluation for IdentifierReference
• This takes us to ResolveBinding
• This takes us to GetIdentifierReference, which ends up returning a Reference whose base is the Global Environment Record, referenced name is "test".
• Now we need to evaluate the reference. This happens via Runtime Semantics: Evaluation for ExpressionStatement
• This calls GetValue on the reference, and we land in step 6.a
• This calls the global environment record's GetBindingValue, which (assuming there are no shadowing var/function declarations) ends up in step 5
• This calls the object environment record's GetBindingValue for the global object
• This calls Get(global object, "test")
• Which of course calls OrdinaryGet, which calls the getter, with the receiver the whole time being the global object, so the this that appears will be the global object.

This seems bad? Am I missing something? Is there a spec bug perhaps?

/cc @bzbarsky

[ljharb]

(worth noting that in Safari and Firefox, the defineGetter call errors with "undefined is not an object"; it works for me in Chrome)

[ljharb]

(worth noting that in Safari and Firefox, the defineGetter call errors with "undefined is not an object"; it works for me in Chrome)

[domenic]

Updated to window.__defineGetter__; should be the same, since DefineOwnProperty on WindowProxy just passes through to Window. The original version was meant to be more environment-agnostic.

With this modified version I admit that an answer might be "intercept all DefineOwnProperties that define getters and make sure they don't pass through to the real global object", i.e. it's a bug in HTML. But I hope that is not the answer, because requiring a proxy to track all its getters instead of using the target object is pretty bad... It seems like ES should be making the this on global getters be the global this value, instead of making the host environment do that.

[domenic]

Updated to window.__defineGetter__; should be the same, since DefineOwnProperty on WindowProxy just passes through to Window. The original version was meant to be more environment-agnostic.

With this modified version I admit that an answer might be "intercept all DefineOwnProperties that define getters and make sure they don't pass through to the real global object", i.e. it's a bug in HTML. But I hope that is not the answer, because requiring a proxy to track all its getters instead of using the target object is pretty bad... It seems like ES should be making the this on global getters be the global this value, instead of making the host environment do that.

[bzbarsky]

Seems like a spec bug to me at first glance.

I guess for bareword function invocation this is handled by the [[GlobalThisValue]] thing. For getters/setters, it definitely needs to be handled.

I can tell you what SpiderMonkey does, and it's probably not going to make you happy. What it does, conceptually, is that every Function has attached to it a boolean that indicates whether it requires its this to be a non-Window. This boolean is only set to false for certain built-in functions; specifically DOM getters and setters; for everything else it's set to true. If during invocation of a callable the this value is a Window and the callable is not a Function or does not have the relevant flag set to true, and the this value is set to the Window's WindowProxy instead.

This allows bareword DOM getters to work correctly (e.g. "document" in the scope of a non-current window will give you the non-current document, not some random other document or a security exception), but prevents leaking the global object to getters/setters in cases like the one above.

[bzbarsky]

Seems like a spec bug to me at first glance.

I guess for bareword function invocation this is handled by the [[GlobalThisValue]] thing. For getters/setters, it definitely needs to be handled.

I can tell you what SpiderMonkey does, and it's probably not going to make you happy. What it does, conceptually, is that every Function has attached to it a boolean that indicates whether it requires its this to be a non-Window. This boolean is only set to false for certain built-in functions; specifically DOM getters and setters; for everything else it's set to true. If during invocation of a callable the this value is a Window and the callable is not a Function or does not have the relevant flag set to true, and the this value is set to the Window's WindowProxy instead.

This allows bareword DOM getters to work correctly (e.g. "document" in the scope of a non-current window will give you the non-current document, not some random other document or a security exception), but prevents leaking the global object to getters/setters in cases like the one above.

[domenic]

Thanks @bzbarsky... that's pretty depressing...

Do you think there's a need for a solution similar to SpiderMonkey's for the spec, or do you think it'll be possible to just special-case global property access and pass the right this value? I haven't tried drafting a spec patch or anything, but I was really hoping the latter was feasible... But your idea that document should give you the non-current document makes it sound like a global solution won't work.

[domenic]

Thanks @bzbarsky... that's pretty depressing...

Do you think there's a need for a solution similar to SpiderMonkey's for the spec, or do you think it'll be possible to just special-case global property access and pass the right this value? I haven't tried drafting a spec patch or anything, but I was really hoping the latter was feasible... But your idea that document should give you the non-current document makes it sound like a global solution won't work.

[allenwb]

It's not clear to me why the [[ObjectRecord]] component of a Global Environment Record isn't created using the global thisValue as its binding object. In that case, all global environment record operations that perform MOP operations upon the encapsulated Object Environment Record will be proxied through the global object.

When would that [[ObjectRecord]] need direct access to the actual global object without proxying through the global this? That need wasn't obvious based upon a quick scan of usage within the ES spec.

[allenwb]

It's not clear to me why the [[ObjectRecord]] component of a Global Environment Record isn't created using the global thisValue as its binding object. In that case, all global environment record operations that perform MOP operations upon the encapsulated Object Environment Record will be proxied through the global object.

When would that [[ObjectRecord]] need direct access to the actual global object without proxying through the global this? That need wasn't obvious based upon a quick scan of usage within the ES spec.

[domenic]

@allenwb yeah, that makes the most sense to me, and was the kind of fix I was hoping for. In that case it was likely an oversight when introducing the global object/global this split.

But I am confused about this case where maybe DOM getters like document are supposed to get the original global object, per @bzbarsky's last paragraph.

[domenic]

@allenwb yeah, that makes the most sense to me, and was the kind of fix I was hoping for. In that case it was likely an oversight when introducing the global object/global this split.

But I am confused about this case where maybe DOM getters like document are supposed to get the original global object, per @bzbarsky's last paragraph.

[bzbarsky]

It's worth testing what DOM getters do in various situations like this in various browsers. Maybe we can just make them always go through the WindowProxy. It's really crappy behavior from an author's perspective, but if it's web-compatible and simpler to spec...

[bzbarsky]

It's worth testing what DOM getters do in various situations like this in various browsers. Maybe we can just make them always go through the WindowProxy. It's really crappy behavior from an author's perspective, but if it's web-compatible and simpler to spec...

[bzbarsky]

So a simple testcase:

<iframe></iframe>
<script>
  var doc;
  var getter1;
  var getter2;
  onload = function() {
    doc = frames[0].document;
    getter1 = new frames[0].Function("return document");
    getter2 = new frames[0].Function("return window.document");
  }
</script>
<input type="button" value="check equality" onclick="try { alert(doc == getter1()); } catch (e) { alert(e); }">
<input type="button" value="check equality via WindowProxy" onclick="try { alert(doc == getter2()); } catch (e) { alert(e); }">
<input type="button" value="navigate subframe" onclick="frames[0].location = 'https://example.com'">

Load it (but not from file:// because security policy around file:// is weird), click the two "check equality" buttons, click the "navigate" button, click the "check equality" buttons again. E.g. you can use https://jsfiddle.net/2ob2p57q/3/

Results:

• Firefox: true, true, true, security exception
• Chrome: true, true, true, exception because window returned null
• Safari: true, true, true, security exception
• Edge: true, true, "can't execute code from a freed script" exception, "can't execute code from a freed script" exception

If I do the same thing but with navigator instead of document (see https://jsfiddle.net/2ob2p57q/5/), the results are:

• Firefox: true, true, false, security exception
• Chrome: true, true, false, security exception
• Safari: true, true, false, exception because window returned null
• Edge: true, true, "can't execute code from a freed script" exception, "can't execute code from a freed script" exception

In Firefox and Safari, bareword "navigator" in a navigated-away-from window returns null. In Chrome, it returns a Navigator object, but not the same one as pre-navigation, and also not the same one as window.navigator would return, as you can tell from https://jsfiddle.net/2ob2p57q/11/. But in any case, the behavior is not the same as if the accessor were done with WindowProxy as this.

By the way, if I work around Chrome's "window becomes null" thing as in https://jsfiddle.net/2ob2p57q/14/ then I get:

• Firefox: true, true, true, security exception
• Chrome: true, true, true, security exception
• Safari: true, true, true, security exception
• Edge: true, true, "can't execute code from a freed script" exception, "can't execute code from a freed script" exception

[bzbarsky]

So a simple testcase:

<iframe></iframe>
<script>
  var doc;
  var getter1;
  var getter2;
  onload = function() {
    doc = frames[0].document;
    getter1 = new frames[0].Function("return document");
    getter2 = new frames[0].Function("return window.document");
  }
</script>
<input type="button" value="check equality" onclick="try { alert(doc == getter1()); } catch (e) { alert(e); }">
<input type="button" value="check equality via WindowProxy" onclick="try { alert(doc == getter2()); } catch (e) { alert(e); }">
<input type="button" value="navigate subframe" onclick="frames[0].location = 'https://example.com'">

Load it (but not from file:// because security policy around file:// is weird), click the two "check equality" buttons, click the "navigate" button, click the "check equality" buttons again. E.g. you can use https://jsfiddle.net/2ob2p57q/3/

Results:

• Firefox: true, true, true, security exception
• Chrome: true, true, true, exception because window returned null
• Safari: true, true, true, security exception
• Edge: true, true, "can't execute code from a freed script" exception, "can't execute code from a freed script" exception

If I do the same thing but with navigator instead of document (see https://jsfiddle.net/2ob2p57q/5/), the results are:

• Firefox: true, true, false, security exception
• Chrome: true, true, false, security exception
• Safari: true, true, false, exception because window returned null
• Edge: true, true, "can't execute code from a freed script" exception, "can't execute code from a freed script" exception

In Firefox and Safari, bareword "navigator" in a navigated-away-from window returns null. In Chrome, it returns a Navigator object, but not the same one as pre-navigation, and also not the same one as window.navigator would return, as you can tell from https://jsfiddle.net/2ob2p57q/11/. But in any case, the behavior is not the same as if the accessor were done with WindowProxy as this.

By the way, if I work around Chrome's "window becomes null" thing as in https://jsfiddle.net/2ob2p57q/14/ then I get:

• Firefox: true, true, true, security exception
• Chrome: true, true, true, security exception
• Safari: true, true, true, security exception
• Edge: true, true, "can't execute code from a freed script" exception, "can't execute code from a freed script" exception

[erights]

Since Edge treats the confusing case as an error, it must still be web compatible to do so. I ask that we once again consider Edge's conservative simple safe solution to many of these weird problems regarding Window vs WindowProxy. Execution within a navigated-away-from frame will always be terribly confusing otherwise.

[erights]

Since Edge treats the confusing case as an error, it must still be web compatible to do so. I ask that we once again consider Edge's conservative simple safe solution to many of these weird problems regarding Window vs WindowProxy. Execution within a navigated-away-from frame will always be terribly confusing otherwise.

[domenic]

Since Edge treats the confusing case as an error, it must still be web compatible to do so

Unfortunately that is not correct; we asked @travisleithead about this a while back, hoping to simplify Chrome's implementation. But he said "please, please, please don't go down this route", that he'd been trying to get this fixed for many years, and they were on the verge of fixing it, because Edge has experienced so many drastic compat bugs because of this that is hurting its ability to browse the web effectively.

[domenic]

Since Edge treats the confusing case as an error, it must still be web compatible to do so

Unfortunately that is not correct; we asked @travisleithead about this a while back, hoping to simplify Chrome's implementation. But he said "please, please, please don't go down this route", that he'd been trying to get this fixed for many years, and they were on the verge of fixing it, because Edge has experienced so many drastic compat bugs because of this that is hurting its ability to browse the web effectively.

[erights]

@domenic Thanks for the clear answer. I am sad but not at all surprised.

[erights]

@domenic Thanks for the clear answer. I am sad but not at all surprised.

[erights]

I am curious what actual sites they have encountered that break on Edge because of this.

Attn @travisleithead @bterlson

[erights]

I am curious what actual sites they have encountered that break on Edge because of this.

Attn @travisleithead @bterlson

[domenic]

So, that said, the problems Travis discussed were generally about Edge's throw-all-the-time-for-detached-iframes behavior, not specifically about global bareword getters, which we can hope are rarer. The issues they ran into were more about people e.g. navigating away from a document or removing an iframe, then traversing its DOM to pull out information they plan to keep using or transfer to a new document or something. This sounds like code "from the outside", not bareword-getters executing "from the inside".

That said, it's pretty easy to imagine e.g. running an event handler or promise job that happens to reference document.querySelector(...) or similar in the old iframe. (That is, it's not just strange frames[0].Function cases that can trigger this behavior.) Combined with the 3/4 agreement for document, I'm not that hopeful about changing things. But we'll see what the V8 folks say.

[domenic]

So, that said, the problems Travis discussed were generally about Edge's throw-all-the-time-for-detached-iframes behavior, not specifically about global bareword getters, which we can hope are rarer. The issues they ran into were more about people e.g. navigating away from a document or removing an iframe, then traversing its DOM to pull out information they plan to keep using or transfer to a new document or something. This sounds like code "from the outside", not bareword-getters executing "from the inside".

That said, it's pretty easy to imagine e.g. running an event handler or promise job that happens to reference document.querySelector(...) or similar in the old iframe. (That is, it's not just strange frames[0].Function cases that can trigger this behavior.) Combined with the 3/4 agreement for document, I'm not that hopeful about changing things. But we'll see what the V8 folks say.

[verwaest]

My motivation for looking into this is to limit places where we can accidentally leak the global object to JS; and where we get different answers for this.XXX and XXX. Making sure the receiver is converted from the global object to the global proxy before going back to JS is hard to get right in V8, let alone all blink callbacks.

V8 implements "contextual access" (XXX rather than this.XXX) as essentially Reflect.get(global_object, "XXX", global_proxy); This works well for regular data properties. In the past DOM attributes such as document were implemented as pseudo-data-properties, and they would get the holder of the document accessors in. Now that they are turned into regular accessors, I see that that changes things.

How relevant is this though? I see that document.querySelector() would all of a sudden reference a different document after navigation (or get a security exception). But that's what you would get for window.document.querySelector() anyway...

I'm in favor of trying to unify those two to reduce unexpected behavior and limit the surface where we could leak the global object. Basically what Mark suggested.

[verwaest]

My motivation for looking into this is to limit places where we can accidentally leak the global object to JS; and where we get different answers for this.XXX and XXX. Making sure the receiver is converted from the global object to the global proxy before going back to JS is hard to get right in V8, let alone all blink callbacks.

V8 implements "contextual access" (XXX rather than this.XXX) as essentially Reflect.get(global_object, "XXX", global_proxy); This works well for regular data properties. In the past DOM attributes such as document were implemented as pseudo-data-properties, and they would get the holder of the document accessors in. Now that they are turned into regular accessors, I see that that changes things.

How relevant is this though? I see that document.querySelector() would all of a sudden reference a different document after navigation (or get a security exception). But that's what you would get for window.document.querySelector() anyway...

I'm in favor of trying to unify those two to reduce unexpected behavior and limit the surface where we could leak the global object. Basically what Mark suggested.

[bzbarsky]

How relevant is this though?

You mean "how many sites would break if we changed the behavior"?

I should add one more note. The current behavior at least in Gecko is much faster (because it doesn't have to deal with a WindowProxy and can just take a fast path) than a behavior where you always to go through a WindowProxy. And regressing the performance of document is an absolute no-no...

[bzbarsky]

How relevant is this though?

You mean "how many sites would break if we changed the behavior"?

I should add one more note. The current behavior at least in Gecko is much faster (because it doesn't have to deal with a WindowProxy and can just take a fast path) than a behavior where you always to go through a WindowProxy. And regressing the performance of document is an absolute no-no...

[verwaest]

Yes, I mean how many sites would break.

V8 treats document special and turns it into a data property internally, which is faster (we cache the result). We don't actually call the getters after the first time. In other words, it's easy to optimize this so that the indirection doesn't matter.

[verwaest]

Yes, I mean how many sites would break.

V8 treats document special and turns it into a data property internally, which is faster (we cache the result). We don't actually call the getters after the first time. In other words, it's easy to optimize this so that the indirection doesn't matter.

[bzbarsky]

Yes, I mean how many sites would break.

Right. That needs measuring, presumably, or attempting to ship a behavior change and seeing what happens....

In other words, it's easy to optimize this so that the indirection doesn't matter.

In the current setup, where it keeps returning the old value, sure. But we're not talking about that setup.

[bzbarsky]

Yes, I mean how many sites would break.

Right. That needs measuring, presumably, or attempting to ship a behavior change and seeing what happens....

In other words, it's easy to optimize this so that the indirection doesn't matter.

In the current setup, where it keeps returning the old value, sure. But we're not talking about that setup.

[verwaest]

In the current setup, where it keeps returning the old value, sure. But we're not talking about that setup.

I'd just invalidate that cached property upon navigation, and after navigation take the slow path. I don't think tanking that performance is a no-go as well?

[verwaest]

In the current setup, where it keeps returning the old value, sure. But we're not talking about that setup.

I'd just invalidate that cached property upon navigation, and after navigation take the slow path. I don't think tanking that performance is a no-go as well?

[bzbarsky]

Probably not. Of course now we're presupposing that you can have a thing that is a data property "internally" but has the right behavior in terms of getOwnPropertyDescriptor and such.

[bzbarsky]

Probably not. Of course now we're presupposing that you can have a thing that is a data property "internally" but has the right behavior in terms of getOwnPropertyDescriptor and such.

[verwaest]

Yes, that's what we have.

[verwaest]

Yes, that's what we have.

[bzbarsky]

But should this be an explicit requirement for a web-compatible ES implementation?

[bzbarsky]

But should this be an explicit requirement for a web-compatible ES implementation?

[verwaest]

No, this is just an optimization. The spec would just get the WindowProxy in as receiver for the document getter.

[verwaest]

No, this is just an optimization. The spec would just get the WindowProxy in as receiver for the document getter.

[bzbarsky]

No, this is just an optimization.

It's an optimization that is required to be web-compatible in your proposed setup.

[bzbarsky]

No, this is just an optimization.

It's an optimization that is required to be web-compatible in your proposed setup.

[annevk]

How is it observable? Or how is it different from the web effectively requiring JITs?

[annevk]

How is it observable? Or how is it different from the web effectively requiring JITs?

[verwaest]

I'm confused. I suppose you assumed we'd use that to maintain the behavior as we currently have it. That's not what I meant.

The optimization isn't designed to maintain web compatibility; just to avoid the performance overhead you mentioned above. It wouldn't maintain web compatibility since it just caches the getter result, which could have been computed after navigation already.

[verwaest]

I'm confused. I suppose you assumed we'd use that to maintain the behavior as we currently have it. That's not what I meant.

The optimization isn't designed to maintain web compatibility; just to avoid the performance overhead you mentioned above. It wouldn't maintain web compatibility since it just caches the getter result, which could have been computed after navigation already.

[bzbarsky]

Or how is it different from the web effectively requiring JITs?

It's not.

[bzbarsky]

Or how is it different from the web effectively requiring JITs?

It's not.

[bzbarsky]

But to be clear, any implementor would know that in practice a JIT is a de facto requirement. The point of specs is to enable interop by documenting de facto requirements in a de jure manner. If we explicitly introduce de facto requirements that we are not willing to specify de jure, that is in my mind a big red flag for those requirements.

Or put another way, if there is only one possible way of implementing a spec, that way should be in the spec explicitly so people don't need to reinvent it independently. If there are multiple possible ways, of course, that doesn't apply. But the idea ought to be to not tie specs to particular implementation without spelling that out explicitly. That's why we aim for multiple independent implementations of specs.

If we think there are multiple ways to implement .document sanely, great. If we think the only way to do it is to have a concept of properties that doesn't exist in the ES spec, then we should explicitly introduce that concept to the ES spec.

[bzbarsky]

But to be clear, any implementor would know that in practice a JIT is a de facto requirement. The point of specs is to enable interop by documenting de facto requirements in a de jure manner. If we explicitly introduce de facto requirements that we are not willing to specify de jure, that is in my mind a big red flag for those requirements.

Or put another way, if there is only one possible way of implementing a spec, that way should be in the spec explicitly so people don't need to reinvent it independently. If there are multiple possible ways, of course, that doesn't apply. But the idea ought to be to not tie specs to particular implementation without spelling that out explicitly. That's why we aim for multiple independent implementations of specs.

If we think there are multiple ways to implement .document sanely, great. If we think the only way to do it is to have a concept of properties that doesn't exist in the ES spec, then we should explicitly introduce that concept to the ES spec.

[annevk]

But we don't want to specify a JIT or JavaScript's sixteen different string types as implementations might change the design of those over time, just like they might change how window.document is implemented.

[annevk]

But we don't want to specify a JIT or JavaScript's sixteen different string types as implementations might change the design of those over time, just like they might change how window.document is implemented.