828787 - Stop allowing indexed expandos on windows

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

The infrastructure in bug 823228 makes this easy.  The spec says to do it at the moment.  Do we think it's likely to be web-compatible enough?

Comment 1

[Jeff Walden [:Waldo]]

This would play nicer with separated storage/representation/hooks for elements and properties, fwiw.  Independent of any web compat/incompat concerns, of course.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Stop allowing indexed expandos on windows.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Try looks green.

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 703384 [details] [diff] [review]
Stop allowing indexed expandos on windows.

Review of attachment 703384 [details] [diff] [review]:
-----------------------------------------------------------------

r=bholley

Comment 5

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/059c7e8541e2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/059c7e8541e2

Comment 7

[Kohei Yoshino [:kohei]]

I've added this bug to the compatibility doc. Please correct the info if I'm wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_21

Comment 8

[Allen Wirfs-Brock]

I'm opening this bug, because the WebIDL spec. that says to do this is in conflict with the ECMAScript 5.1 spec. There is nothing in the ES spec. that gives WebIDL permission to require this and obviously we haven't traditionally done so or we wouldn't be making a change.

Also see https://bugs.ecmascript.org/show_bug.cgi?id=1453

Comment 9

[Jeff Walden [:Waldo]]

ES5 section 16 says implementations can add any extra properties they want, including indexed ones.  Given that, I think it is much more consistent with the spirit of the specification for it to be allowable for implementations to prohibit the addition/setting of certain properties.  If not in general, then definitely an exception should be made for the global object, which is all sorts of broken in enough ways that it's not worth trying to force it into shape on this issue.  I see no value in requiring non-prohibition of indexed properties on the global object.

Comment 10

[Allen Wirfs-Brock]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> ES5 section 16 says implementations can add any extra properties they want,
> including indexed ones.  Given that, I think it is much more consistent with
> the spirit of the specification for it to be allowable for implementations
> to prohibit the addition/setting of certain properties.

I don't at all see how you can infer that giving permission for implementations to add additional built-in properties also implies a permission for an implementation to restrict user written code from using certainly property keys that don't correspond to such such built-ins.

A major purpose of a language standard is to ensure that programs that are written in conformance to the standard will operate correctly on different implementations and different host environments (assuming no dependencies on permitted host extensions).

If WebIDL-based implementations make this restriction than ECMAScript code that legally (according to the ES spec.) add/access "array index" properties would not be portable from other platforms to the web platform.

ECMAScript permits property additions to the global object as an extension mechanism, but does it in a way that permits programs written for other implementations to still define/over-write global properties of the same name (see the handling of global level function declarations which does extra work to ensure this). 

>  If not in general,
> then definitely an exception should be made for the global object, which is
> all sorts of broken in enough ways that it's not worth trying to force it
> into shape on this issue.  I see no value in requiring non-prohibition of
> indexed properties on the global object.

As mentioned above, code portability among browser and non-browser ES implementations.

ES properties keys are strings (until ES6) even with they look like numbers. Does these changes as restrict non-array index numeric strings such as "1.0"?  If not, then why is it reasonable to allow "1.0" as an expando key of an object but not to allow "1"?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Alan, you can't just reopen a bug that's had a patch checked in unless you have also backed the patch out.

If you think the patch _should_ be backed out, please open a new bug to track that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

And also not that if you allow indexed expandos on Window, then you need to define their interaction with the existing arraylike behavior of Window.  Which would need to happen in WebIDL, since WebIDL is what defines such things at the moment...

> As mentioned above, code portability among browser and non-browser ES implementations.

Um...  Is the global arraylike in non-browser ES implementations?  If not, and if the code uses indexed properties on a global, then it's already non-portable in practice, since frame indices shadow expandos in implementations that do allow expandos, as far as I can tell, and have all along.

> Does these changes as restrict non-array index numeric strings such as "1.0"?

The restriction is on property names P for which the following algorithm returns true:

    Let i be ToUint32(P).
    Let s be ToString(i).
    If s != P or i = 2^32 − 1, then return false.
    Return true.

See WebIDL section 4.6.1.  This is identical to the definition of "array index" in ECMA-262 section 15.4, for what that's worth.

> ECMAScript permits property additions to the global object as an extension mechanism,
> but does it in a way that permits programs written for other implementations to still
> define/over-write global properties of the same name

Note that for security reasons the web platform defines several non-configurable own properties on Window objects, so this is to some extent already a lost cause.

In any case, if you think WebIDL is wrong here please file a bug on WebIDL to change.  Feel free to suggest what you think the behavior _should_ be in that bug if you have a plan.  Otherwise please at least describe what properties you think the desired behavior should have which the current one does not, keeping in mind the existing arraylike behavior of Window...  Yes, we all wish Window were not arraylike.  :(

Comment 13

[Jeff Walden [:Waldo]]

(In reply to Allen Wirfs-Brock from comment #10)
> I don't at all see how you can infer that giving permission for
> implementations to add additional built-in properties also implies a
> permission for an implementation to restrict user written code from using
> certainly property keys that don't correspond to such such built-ins.

If I read you right, I wasn't saying that the current language actually implies permission.  I was saying that by allowing custom behaviors for added properties, and not constraining the properties added, ES5 allows near-perfect emulation of exactly the behavior implemented here.  (Just that the behavior here is optimized to not actually create all those actual properties.  And to make those "poison-pill" "properties" appear not to exist -- a definite change, but one paling in significance compared to allowing additional properties in the first place.)  And that allowance is permits enough of what WebIDL does for window objects, that its spirit argues for giving implementations carte blanche to have arbitrary behavior for added properties.

But I think we might be ratholing on argument semantics when in reality window indexing on the global can't be removed.

> As mentioned above, code portability among browser and non-browser ES
> implementations.

Maybe.  Historically, there's been enough difference between the code people write in the browser, and code people write for other JS embeddings, that portability most code has always pretty much been a pipe dream.

Outside the purported portability win, I don't see much value in treating the global as an array-like object?  The browser case is an anti-pattern.  People can already use actual arrays for this purpose.  Or even arbitrary objects that aren't the global, if they just want to be weird.

> ES properties keys are strings (until ES6) even with they look like numbers.
> Does these changes as restrict non-array index numeric strings such as
> "1.0"?  If not, then why is it reasonable to allow "1.0" as an expando key
> of an object but not to allow "1"?

Why is it reasonable for |var arr = []; arr["1.0"] = 5;| to not increase the length of the array?  Really it's not, and arrays should have been significantly different from normal objects ab initio.  But that ship's sailed as much as indexing the global in browsers has sailed.

Comment 14

[Allen Wirfs-Brock]

(In reply to Boris Zbarsky (:bz) from comment #12)
> And also not that if you allow indexed expandos on Window, then you need to
> define their interaction with the existing arraylike behavior of Window. 
> Which would need to happen in WebIDL, since WebIDL is what defines such
> things at the moment...
> 

Ok, I'm sold. You said the magic words that clears up everything: "existing arraylike behavior of window".  I was confused  by the bug report against test262 https://bugs.ecmascript.org/show_bug.cgi?id=1453 which didn't make it clear that window has legacy array-like behavior and that this change is really only about the handling of array index expandos that are outside of the "length" range.

It is perfectly reasonable for an object that, in WebIDL terms, explicitly "supports indexed properties" to restrict the range of such properties to preclude adding regular array-index expandos beyond the length or to make "length" and the array index properties readonly. It wouldn't be reasonable to restrict "array-index" expandos on bjects that don't explicitly "supports indexed properties.

It also wasn't clear that the problem with the test262 tests WRT this change was not that array-like methods such as indexOf no longer work on the windows object (they should) but rather than the setup for testing indexOf (etc.) assumed that expandos could be used to define the array-like state of the global object as a precondition of the test.

Given this legacy window behavior I doubt if there is any real interoperability issues with non-browser implementations.  I just need to come up with ES spec. language for the global object that permits its.

Comment 15

[Boris Zbarsky [:bzbarsky]]

OK, good.  I was hoping there was just a miscommunication somewhere.

No one is proposing any sort of restrictions on adding array-index expandos on objects that don't support indexed properties; I think we all agree that that would be ... let's be polite and call it very weird.  ;)