1178638 - Turn on the exceptions from bug 1107443 (Object.defineProperty on window with non-configurable property) on beta/release

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Need to see how web-compatible this is first...

Comment 1

[Boris Zbarsky [:bzbarsky]]

We have a second report of a web compat issue here now: http://media.tojicode.com/webgl-samples/dds.html does this:

        if(!window.animationStartTime) {
....
            Object.defineProperty(window, "animationStartTime", {
                enumerable: true, configurable: false, writeable: false,
                get: getter
            });
        }

This seems like an attempt to polyfill a "standard" version of https://developer.mozilla.org/en-US/docs/Web/API/Window/mozAnimationStartTime (which is gone in Firefox 42, even!).  Not sure how much we care about this particular page, or whether we can just get it fixed, but...

Comment 2

[Boris Zbarsky [:bzbarsky]]

And a third web compat report: https://shop.cyberlink.com/c/shop?curr=EUR&cntr=&ml=EN&LOCALE=en_EU&OFFER_ID=6112WA033101&AFFILIATE_ID=2581_-1_37_CJaffiliate_SID-617e8c1ffb8145a88908cf5f85919deb&AM=1&ID=P11028558 (see bug 1261572).

The relevant code here is:

	function ensure (context, name, value) {
....
		if (context[name] === UNDEFINED) defineProperty(context, name, { value: value, configurable: false, enumerable: true, writable: false });

The comments at the top of the file say:

  /**
   asknet_jq_ajax.js (c) 2016, asknet AG (rwm)
   */

I'm guessing this is produced by the http://www.asknet.com/ folks...

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, given that, I think actually shipping this is a non-starter.  It's too bad we never got any other browsers to even consider this before people started using this in production libraries, but here we are.

Mark, if you actually care about these invariants not being violated, please work with the other browser vendors to come up with a plan they're willing to accept.  I'm tired of rolling this rock uphill, on an issue I don't actually care about all that much.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, the code quoted in comment 2 is in https://shop.cyberlink.com/jscr/asknet_stable_ajax.js?ts=160318101810-601

Comment 5

[Mark S. Miller]

Hi Boris, I do care and will take this on. I had not realized that this felt like rolling a rock uphill. My apologies for earlier lack of attention. Thanks.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The rolling uphill bit is because none of the other browser vendors ever responded to any of my questions about this.  :(

Thank you for picking it up!

Comment 7

[Kohei Yoshino]

Somehow this has slipped under my radar. Just posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/window-now-throws-when-defining-non-configurable-property-currently-only-on-nightly-and-developer-edition/

Comment 8

[Boris Zbarsky [:bzbarsky]]

Kohei, the throwing behavior is not in fact what the HTML spec says to do.  It just has notes about how what it _does_ say to do is not compatible with the ES spec....

Comment 9

[Kohei Yoshino]

Gah, thanks, removed "as per the latest HTML spec."

Comment 10

[Tom S [:evilpie]]

Somebody writing a webextension just ran into this, are we ever going to enable this on release?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Unlikely, per above discussion.  Again, I've given up on spending time on this; the lack of interest from either TC39 or other browsers means this ship has likely sailed way too far.  We should probably just turn this off altogether and allow defining these properties, then violate the invariants no one seems to care about.

Comment 12

[Mark S. Miller]

Hi Boris, I thought you knew. My fault -- I should have updated this bug. TC39 has resolved this. We decided on the minimal corruption of the invariants that let's it be web compatible. The brief description is that Object.defineProperty(global, stuff) may indicate failure by returning false rather than throwing. However, Reflect.defineOwnProperty, the [[DEFINE_OWN_PROPERTY]] trap, the proxy trap, are all unmodified. The state invariants are not weakened at all. There is no change to when Object.defineProperty succeeds vs fails. The only change is in how it is allowed to report failure.

There is a good record in some meeting notes and there is a written proposal or PR. I am too busy today to look. Could someone else find them and post links here? Thanks.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So just to make sure I understand:

  Object.defineProperty(global, "foo", {value:5, configurable: false});

Per the TC39 resolution, this will return false and global.hasOwnProperty("foo") will be false?  As in, the property is not actually defined?

Or is the property defined, but defined as "configurable: true" and false is returned?  Or something else?

Link to the actual behavior decided on here would be much appreciated.

Comment 14

[Boris Zbarsky [:bzbarsky]]

(And no, no one bothered to tell me this was even being discussed.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

I found the minutes at https://github.com/tc39/tc39-notes/blob/master/es7/2016-09/sept-27.md#hostobjectdefinepropertyreturnfalse -- those do not explain the proposed behavior as far as I can tell.

Comment 16

[Mark S. Miller]

> (And no, no one bothered to tell me this was even being discussed.)


Sorry for not bringing all this to your attention. The TC39 discussion was indeed exactly the discussion you had called for.

Comment 17

[Mark S. Miller]

> So just to make sure I understand:
>
>   Object.defineProperty(global, "foo", {value:5, configurable: false});
>
> Per the TC39 resolution, this will return false and global.hasOwnProperty("foo") will be false?  As in, > the property is not actually defined?
> 
> Or is the property defined, but defined as "configurable: true" and false is returned?

Either of these would be allowed by the TC39 resolution. The global object may be an exotic object with any *allowable* [[DEFINE_OWN_PROPERTY]] behavior, including a Proxy with a custom defineOwnProperty trap. Which behaviors are allowed has not changed. The only change we made from ES6 requirements is how Object.defineProperty is allowed to report failure.

Concretely, it is a matter of browser standards to choose among the allowed behaviors. Of these, I would prefer the first --- no property is defined. This behavior seems more straightforward and safer, but may be less web compatible than first defining a similar configurable property, and then failing. Certainly the second is closer to the FF implementation discussed in this thread. In any case, for the canonical builtin browser global object, the relevant browser standards group should make a decision. 

Or, we could make this decision in TC39 anyway, since we do have an appendix of "normative requirements for JS in browsers".

What do you recommend?


> Or something else?

I have not thought of any plausible alternative to the two options you explain above.

Comment 18

[Boris Zbarsky [:bzbarsky]]

I think the "don't define the property" behavior is highly unlikely to be web-compatible given that people are in fact defining such properties.

OK, it sounds like we should:

1)  Wontfix this bug.
2)  Back out our current code: bug 1329323 filed.
3)  Implement the defineProperty behavior: bug 1329321 and bug 1329324 filed.