1197958 - JSIL fails to load with 'Not allowed to define a non-configurable property on the WindowProxy object'

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Katelyn Gadd (:kael)]

At some point JS behavior changed such that JSIL applications fail to load with the error 'Not allowed to define a non-configurable property on the WindowProxy object', triggered in the JSIL loader file JSIL.js line 19. See pretty much any JSIL sample (like http://jsil.org/try) but a good one is SampleFNA (http://hildr.luminance.org/SampleFNA/).

This used to work in Firefox recently and still works in Chrome. Is this a recent JS spec change that SpiderMonkey is adhering to now and nobody else adheres to? This will definitely break some shipping applications (on newgrounds etc), and in some cases I can't get the author to update their code to a newer version of JSIL, so those apps will break forever.

Comment 1

[Boris Zbarsky [:bzbarsky]]

> Is this a recent JS spec change that SpiderMonkey is adhering to now and nobody else
> adheres to?

It's something that's been in the spec for years but no one was actually doing... but it's needed for the spec's security invariants, pretty much.

The relevant code is this:


  Object.defineProperty(
    globalNamespace, "JSIL",
    {
      value: JSIL,
      configurable: false,
      enumerable: true,
      writable: false
    }
  );

where globalNamespace is in fact the window object ("this" in global scope, passed to this function).

For now this is nightly/aurora only, precisely so we can figure out how web-compatible this is.  

Mark, if it turns out that this JSIL bustage means we can't ship the change from bug 1107443, then what are our options in spec terms?  :(

Comment 2

[Mark S. Miller]

Depends on the precise nature of the jsil breakage. Why, precisely, does it break?

Comment 3

[Katelyn Gadd (:kael)]

.NET public namespaces are emulated by putting public types in the global (window) scope. This happens to be done with non-configurable properties, since they're not meant to be overwritten.

So any previously shipped JSIL apps will break, and I'm not sure all those users can update, since they may be as much as 12 months behind the trunk version of the compiler.

Comment 4

[Mark S. Miller]

None of the options I see so far are pleasant:


* Redefine the contract of Object.defineProperty so that it *can* signal failure either by throwing (as now) or by returning false. Returning false rather than throwing is hazardous, because existing clients that rely only on normal control flow to determine success can be misled. Thus, we would allow false only on the narrowest case we can viably specify, such as non-writable, non-configurable on the WindowProxy when the underlying property on the Window (the real but unreifiable global object) has indeed been successfully made non-writable, non-configurable. Object.getOwnPropertydescriptor would still do what the draft spec says it should do -- report the property on the WindowProxy to be configurable. (Crucially, we do not alter the contract of Reflect.defineProperty or the contract of the [[DefineOwnProperty]] trap.)

* Prevent navigation of a frame whose WindowProxy has had a non-configurable property set.

* Have the property continue to appear to exist as a non-writable, non-configurable property on the WindowProxy despite further navigation, but throw on any attempt to read its value when the WindowProxy's Window is not the Window on which the property was set.


There may well be other options. We should brainstorm some more. Of the three above, I suspect that the first is the least unpleasant. Though, of course, it is still awful.

Comment 5

[Katelyn Gadd (:kael)]

The resulting behavior from making a nonconfigurable, nonwritable property on window is mostly for sanity, since those properties mostly exist for JS interop. So from the perspective of JSIL, there's not a problem with any behavior other than throwing (since throwing breaks the whole application).

I don't think any generated JSIL code relies on the configurable/writable status of the property.

Also, out of curiosity, does this restriction apply to objects using the window as a prototype? That's how assembly private namespaces are implemented in JSIL - i.e. privateNamespace = Object.create(window).

Comment 6

[Boris Zbarsky [:bzbarsky]]

> does this restriction apply to objects using the window as a prototype

No, because those don't have weird things happening on navigation (like nuking "non-configurable" properties).

Of the options in comment 4, preventing navigation is a non-starter in my opinion; it's user-hostile to do that.  Throwing on property accesses after navigation is also a non-starter: it allows script on one site to mess up scripts on another site.

We could do the thing with signaling failure from Object.defineProperty in a more complicated way.

We could also give JSIL consumers some time to update and then make the change anyway.  Note that updating does not require recompilation; it just requires removing the "configurable: false" line from that defineProperty call, right?  So people could just change it in-place.

Comment 7

[Mark S. Miller]

> from the perspective of JSIL, there's not a problem with any behavior other than throwing

So all three bullets remain candidates. Of these, I continue to favor the first bullet. Though I'd still prefer that someone thinks up some other alternative that's less unpleasant ;).

After

> privateNamespace = Object.create(window)

privateNamespace would be a normal object that inherits from a WindowProxy object. An own property on the privateNamespace object would just be a normal own property on a normal object.

Comment 8

[Mark S. Miller]

> Of the options in comment 4, ....

Ok, let's consider the second and third bullets above disqualified.

> updating [...] just requires removing the "configurable: false" line from that defineProperty call, right?

Right.


Of ideas so far, we have

a) (The first bullet above): weakening how Object.defineProperty might report failure, but without altering
Reflect.defineProperty
the [[DefineOwnProperty]] trap
Object.getOwnPropertydescriptor

b) Waiting for JSIL consumers to update.

Comment 9

[Katelyn Gadd (:kael)]

There are other callsites in the code that perform this particular type of set-property, unfortunately. I can patch them all up but that makes a drop-in replacement of the libraries less feasible for existing deployed applications. One I know of that's up on Newgrounds was released in late 2013, so it would be rather difficult for them to update, if I can even get in touch with them. Right now their game doesn't work anymore. There are various Ludum Dare games out there using it, and I know of at least one enterprise customer using it (but they're on trunk, so it's easy for them to update, at least).

There are more customers I don't have any way to know about, because multiple vendors have forked JSIL into commercial products. They may be months behind trunk in which case they won't get this fix either. I suppose if I used a copyleft license I'd know about all of them and I could tell them to update :-)

Comment 10

[Tom Van Cutsem]

MarkM, just to make sure I understand your proposal, this would be a change to be made in WebIDL, not the ES6 spec, right? (since WindowProxy is outside the scope of the ES6 spec)

That is, I assume you're not proposing to weaken the behavior of `Object.defineProperty` for arbitrary JS objects.

One issue with the proposal is that `Object.defineProperty(O, P, Desc)` has been specced since ES5 to always return the `O` object, and code may exploit this fact by chaining additional operations on O. 

Changing the return value of defineProperty from `O` to `false` in this narrow case may cause surprising behavior (e.g. trying to invoke a method on the value `false`).

Comment 11

[Mark S. Miller]

(In reply to Tom Van Cutsem from comment #10)
> MarkM, just to make sure I understand your proposal, this would be a change
> to be made in WebIDL, not the ES6 spec, right? (since WindowProxy is outside
> the scope of the ES6 spec)

No, it would be a change to both, though the important part of the change would be to the ES spec. (Well, rather, technically correct, since it would not be a change to ES2015 specifically but rather to some later edition.)

The reason of course is that the invariants apply to all objects that are in any way observable from ES. Furthermore, the contract of Object.defineProperty is specified by ES. WebIDL cannot permit Object.defineProperty to violate what ES says it must do. Or rather, when it does, the WebIDL spec should be ignored because it exceeds its jurisdiction.

The most important part of the invariants is the inductive implications via a proxy using a weird object as its target. That's why I propose no change be made to [[DefineOwnProperty]], Reflect.defineProperty, nor to Object.getOwnPropertydescriptor. In the context of this proposal, this has a good and a bad implication:

* Good: No one can further subvert the invariants by making a Proxy whose target is a WindowProxy, because the inductive invariant enforcement mechanism depends only on the behavior of the target's [[DefineOwnProperty]] trap.

* Bad: A Proxy, p, whose target is a WindowProxy, wp, cannot fully emulate the behavior of its target. Membranes would thus fail to be fully transparent. Specifically:

Object.defineProperty(wp, 'x', {configurable: false}); // false
Object.defineProperty(p, 'x', {configurable: false}); // thrown error

The good and bad news above are inseparable.


> That is, I assume you're not proposing to weaken the behavior of
> `Object.defineProperty` for arbitrary JS objects.

Correct.


> One issue with the proposal is that `Object.defineProperty(O, P, Desc)` has
> been specced since ES5 to always return the `O` object, and code may exploit
> this fact by chaining additional operations on O. 
>
> Changing the return value of defineProperty from `O` to `false` in this
> narrow case may cause surprising behavior (e.g. trying to invoke a method on
> the value `false`).

Exactly! Any such code will still turn the false return into a thrown error. Thus we work around the JSIL bug while minimizing damage to other code.

(I say "JSIL" bug above only reluctantly. But given the ES invariants and the varying behavior of the WindowProxy under navigation, we must judge it to have always been a bug, though an obscure one, to rely on Object.defineProperty(windowProxy, .., {.., configurable: false}) not throwing an error.)

Comment 12

[Tom Van Cutsem]

I'm looking at the ES6 spec for Object.defineProperty. The salient part is here <http://www.ecma-international.org/ecma-262/6.0/#sec-definepropertyorthrow>. [[DefineOwnProperty]] is already specced to return just a boolean success value, no change needed there.

I'm just wondering how we can go about making an exception in this algorithm for just WindowProxy, when WindowProxy is not defined in the ES spec. Are you thinking of introducing some more general mechanism like a whitelist of objects for which no exception will be raised, which is otherwise unused by the ES spec but can be used by e.g. WebIDL to add WindowProxy to that list?

This all feels like a giant hack, by the way :-/ (I'll grant that I don't have an alternative)

Comment 13

[Mark S. Miller]

> This all feels like a giant hack, by the way :-/ (I'll grant that I don't have an alternative)

It is all a giant hack.

Perhaps the best way to go is not to specify this hack at all in official standards docs, but just to implement it as an interim deployment measure until enough existing JSIL deployments are upgraded. Once that happens, then ES implementations can proceed to implement the full non-hacky spec. http://wiki.ecmascript.org/doku.php?id=conventions:recommendations_for_implementors seems a fine place to unofficially document such temporary hacks.

How does that sound?

Comment 14

[Katelyn Gadd (:kael)]

A temporary fix isn't really of value to me, and it sounds like it's not possible to do anything that's long-term compatible with old deployments of JSIL - so do what you gotta. Having a timeline (i.e. which specific version #) for when this will hit Release channel will be useful so I can do my best to get people to update.

Comment 15

[Mark S. Miller]

If we don't do a temporary fix (whew!), then the current FF43 Nightly behavior is fine. Boris, if nothing is done to intervene, when would FF43 hit Release channel?

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Mark S. Miller from comment #15)
> Boris, if nothing is done to intervene, when would FF43
> hit Release channel?

December 15 according to https://wiki.mozilla.org/RapidRelease/Calendar

Comment 17

[Tom S [:evilpie]]

This change is disabled for the release channel. See the blocked bug 1178638.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Right.  This change will not proceed to the release channel in Firefox until we make the explicit step of enabling it there, which we will not do until we're sufficiently sure it won't break too many things.  This is made harder by the fact that no other browser vendor has committed to making a similar change, and I'm not willing to unilaterally break content here.

So one plausible approach here is:

1)  Get JSIL trunk fixed to not depend on browser bugs.
2)  Communicate with JSIL users, to the extent possible about this problem, give them
    time to update or patch locally.
3)  Get browsers to flip the switch on this somewhat in concert, at which point whatever
    stragglers haven't updated will end up getting a none-to-gentle nudge in that
    direction.

#1 and #2 would fall on :kael.  #3.... I don't know.  As I said, I have had 0 success getting representatives from any other browser vendor to talk to me about this.  :(

Comment 19

[Katelyn Gadd (:kael)]

I fixed trunk already, so it's just a question of how fast it will propagate. I think some people won't ever be able to update to trunk, but they can potentially apply the fix by hand (it's not very much code). Once there's a timeline for this being flipped on I'll message it to the big customers I know about so they can either update to trunk or do the patch by hand.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please don't play with bug flags.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.