781739 - DOM properties on the global should be on the object itself, not the prototype

Status: RESOLVED DUPLICATE of bug 932322

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

Description

[Brendan Eich [:brendan]]

The patch for bug #722121 undid Igor's fix for bug 632003, which fix was matched by an ES5 erratum that was fixed in ES5.1 (ISO version of ES5).

This unwarranted and uncommented change in the midst of a "cleanup" bug, combined with perfectly valid WebIDL use in the IndexedDB spec, has left Firefox broken compared to Chrome. See bug 770844 and

http://lists.w3.org/Archives/Public/public-webapps/2012JulSep/0392.html

This needs to get fixed last year.

/be

Comment 1

[Brendan Eich [:brendan]]

Jason, are you willing to take this and fix it quickly? Or give it to Waldo if he's back.

/be

Comment 2

[Jason Orendorff [:jorendorff]]

OK. Reading the spec to figure out what we should be doing.

I wonder why the test in bug 632003 didn't detect this regression.

Comment 3

[Brendan Eich [:brendan]]

I'm backwards on "Igor's fix". The bug 632003 has summary "var statement should not pay attention to properties on the prototype", which walks directly into the var+obj-detection problem indexedDB unprefixing hit.

I have downloaded the latest ES5.1 spec from ecma-international.org, checked my copy (same), and Jason's HTML version at http://people.mozilla.org/~jorendorff/es5.1-final.html. They all seem to say what I just wrote to public-webapps:

ES5.1 says in 10.5 step 8:

8. For each VariableDeclaration and VariableDeclarationNoIn d in code, in source text order do
 a. Let dn be the Identifier in d.
 b. Let varAlreadyDeclared be the result of calling env’s HasBinding concrete method passing dn as the argument.
 c. If varAlreadyDeclared is false, then
  i. Call env’s CreateMutableBinding concrete method passing dn and configurableBindings as the arguments.
  ii. Call env’s SetMutableBinding concrete method passing dn, undefined, and strict as the arguments.

The concrete HasBinding method of object environment records (such as the global object's env. rec) is 10.2.1.2.1:

10.2.1.2.1 HasBinding(N)

The concrete Environment Record method HasBinding for object environment records determines if its associated binding object has a property whose name is the value of the argument N:

1. Let envRec be the object environment record for which the method was invoked.
2. Let bindings be the binding object for envRec.
3. Return the result of calling the [[HasProperty]] internal method of bindings, passing N as the property name.

The [[HasProperty]] internal method is:

8.12.6 [[HasProperty]] (P)

When the [[HasProperty]] internal method of O is called with property name P, the following steps are taken:

1. Let desc be the result of calling the [[GetProperty]] internal method of O with property name P.
2. If desc is undefined, then return false.
3. Else return true.

The [[GetProperty]] internal method is:

8.12.2 [[GetProperty]] (P)

When the [[GetProperty]] internal method of O is called with property name P, the following steps are taken:

1. Let prop be the result of calling the [[GetOwnProperty]] internal method of O with property name P.
2. If prop is not undefined, return prop.
3. Let proto be the value of the [[Prototype]] internal property of O.
4. If proto is null, return undefined.
5. Return the result of calling the [[GetProperty]] internal method of proto with argument P.

Notice the tail call at step 5 to walk the prototype chain.

Allen pointed out the pending (not fixed in ES5.1 even) erratum: https://bugs.ecmascript.org/show_bug.cgi?id=78

That is indeed the erratum Igor patched SpiderMonkey to anticipate in bug 632003. I'm wrong about Waldo's cleanup patch for bug 722121 regressing things. Jeff's patch did add && obj.isGlobal() conjunct to the obj2 != obj test that detects a proto-property named by a var decl. Maybe you or he remember why this was added. It looks like narrowing the erratum-fix to apply only to globals was needed for web compat, but I don't yet see a separately filed bug motivating this isGlobal() test.

For ES6 we need to go back to the lab. The var+object-detection pattern is too sweet to ignore, given infernal vendor-prefixing of window-inherited proto-properties (per WebIDL). Allen points out we should not break let/const/module in whatever way var ends up broken for compatibility. I still think we are pushing a stone up a tall hill in trying to change the ancient var vs. protoprop behavior.

/be

Comment 4

[Brendan Eich [:brendan]]

To summarize: bug 632003 was motivated by the ability in ES5 to make non-configurable non-writable properties on prototypes of the global object, which defeat var in global code.

That is a problem and I'm not sure of the fix, but I'm pretty sure it is not the break in backward compatibility that blew back on indexedDB unprefixing vs. var+object-detection. I say we should stand on the ES5.1 spec as written and figure out what to do with https://bugs.ecmascript.org/show_bug.cgi?id=78 now that we have more experience.

/be

Comment 5

[Brendan Eich [:brendan]]

The patch for bug 722121 also changed the old js_CheckRedeclaration logic forbidding var after const of same name, still disallowing const after var. Presumably this was to avoid re-regressing bug 539448 (which kept 'var undefined;' working).

Lotta changes for a cleanup patch! But lots of constraints, not all in the official spec, to be fair.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

OK, having read all this, what's the good state we're trying to revert to? ES5.1? Whatever Chrome does?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

And if someone says "whatever chrome does", please specify which exact chrome behaviors you are referring to.

Comment 8

[Allen Wirfs-Brock]

Some background copied from my post at https://mail.mozilla.org/pipermail/es-discuss/2012-August/024481.html :

Neither ES5 or ES5.1 does a "own" check for either global var or function declarations. See 10.5 step 5.c/8.b each of which calls HasBinding 10.2.1.2.1 which does a [[HasProperty]] which, if necessary, looks up the [[Prototype]] chain.

The change made in ES5.1 (renumbered existing ES5 10.5 step 5.e as 5.f and inserted a new step 5.e + substeps related to dealing with function declarations that over-wrote accessor properties (both own or inherited) of the global object as well function declarations that tried to redefined non-configurable global object properties.  This change was motivated by https://bugzilla.mozilla.org/show_bug.cgi?id=577325 and the es-discuss thread referenced in that bug.

Note that the ES5.1 change only related to function declarations.  ES5.1 did not  make any changes to the specified semantics of global var declarations. 

Subsequent discussion  of Mozilla bug 577325 lead to https://bugs.ecmascript.org/show_bug.cgi?id=78 .  It proposes an errata  (subsequently approved by TC39)  to ES5.1's 10.5 that extends the function declaration fix to apply to var declarations. It also changed the check for a pre-existing global property to a "own" check.  This latter change is presumably what has been implemented in FF and is causing the indexedDB issue. 

These post ES5.1 errata was based upon these  desired semantics:

1) "variable" accesses that bind to inherited properties of the global object
should return the current value of the inherited property.  (note such "variable" accesses
may be to properties created by function declarations)

2) "variable" assignments  to inherited properties of the global object should
be equivalent to a [[Put]] to the global object.  Whether or not a own property
is created depends upon the [[Writable]] attribute of the inherited property
and the extensible internal property of the global object.

3) global function and var declarations always create own properties of the
global object.  If an inherited property of the same name already exists it is
over-ridden with an own property. 

4) The declaration instantiation rules relating to pre-existing bindings
only consider own properties of the global object.  Inherited properties of the
global object have no effect upon the processing of function and var
declarations.

Supporting requirements 3&4 are where the "own" property checks were introduced. 

However, I don't thing we can just drop them without some careful thought.  If we did that, we would reintroduce problems related to global declarations firing inherited setters and also interactions between inherited property attributes and global declarations.

It feel like we are still playing semantics ping-pong with compat. bug paddles. I'm not sure that there is an ideal solution.  Perhaps we need to consider var and function declarations separately.  Perhaps we need be have different redeclaration rules for own and for inherited global object properties.  In any chase I think we need to do a more care analysis then just fixing this bug and it needs be be coordinated between the ES spec. and WebIDL.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Who will make sure that that happens? And can we reach a decision and come up with a fix in order to avoid implementing a backup plan? I.e. can we do that before the current Aurora code (which has the IndexedDB "breakage") is uplifted to Beta on August 27th?


Since we are on the subject, a similar thing which have been breaking in Firefox but working in Chrome is code which does:

function onmessage(event) { ... }

in global scope in workers.

In Firefox the global scope object has on its prototype chain a setter for the 'onmessage' property. However this setter isn't run and instead a shadowing variable is declared.

In Chrome the global scope object has the setter on the object itself, causing the setter to run.

This caused the page to work in Chrome since the setter is run and thus an event handler was registered, while in Firefox a "expando" variable is created and nothing else happens.



Another thing that I just realized is that allowing 'var' to simply use "in" rather than "own" would likely still cause the following code to break:

var indexedDB = window.indexedDB || window.mozIndexedDB || ...;

since that would try to set the indexedDB property which has a getter on the prototype chain, but no setter.



So I definitely don't have a good answer for what the right solution here is. Though the chrome strategy of putting attributes on the object itself rather than on the prototype definitely seems appealing for at least the global object.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> This caused the page to work in Chrome since the setter is run and thus an event handler
> was registered, while in Firefox a "expando" variable is created and nothing else happens.

To make this work, either we need a prop on the global itself, not on the proto, or the spec needs to change to not have function decls shadow proto props.  This last is not great, since the spec has required such shadowing for a long time and every single browser implements it interoperably.

> since that would try to set the indexedDB property which has a getter on the prototype
> chain, but no setter.

That's not a problem in non-strict mode: the set is just silently ignored and everything is fine.

In strict mode, this would throw, but people don't use that much...

Comment 11

[Brendan Eich [:brendan]]

es-discuss and public-script-coord have the cross-posted mega-thread striving for the right answer, just in case anyone reading this bug has missed it.

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #10)
> > This caused the page to work in Chrome since the setter is run and thus an event handler
> > was registered, while in Firefox a "expando" variable is created and nothing else happens.
> 
> To make this work, either we need a prop on the global itself, not on the
> proto, or the spec needs to change to not have function decls shadow proto
> props.  This last is not great, since the spec has required such shadowing
> for a long time and every single browser implements it interoperably.

According to the erratum, 'function' must (1) not only shadow proto-props, it must (2) not invoke an "own" setter that it overrides.

You're right that (1) is a long-standing compatibility point. Point (2), not invoking an own setter of the same name, hurts us currently re: Chrome in the web worker onmessage case, but that's new and we should try to standardize away from the Chrome behavior. Are we?

> > since that would try to set the indexedDB property which has a getter on the prototype
> > chain, but no setter.
> 
> That's not a problem in non-strict mode: the set is just silently ignored
> and everything is fine.
> 
> In strict mode, this would throw, but people don't use that much...

We can't just ignore strict mode. People do use it in global code, and having indexedDB var + object detection patterns mysteriously throw in such code is bad business. Consider how it may be used at the front of a concatenation (this made trouble in the past before all the top browsers' recent versions supported strict mode, but we are past that I think).

Who knows how strict mode will fare in the long run, but based on Perl's experience, people have plausible hope that it starts turning up over time. It would help if strict mode actually improved performance, rather than being neutral or provoking de-optimizations.

But none of this justifies ignoring strict mode or saying "don't use it" in certain var object detection scenarios.

/be

Comment 12

[Boris Zbarsky [:bzbarsky]]

There are only two ways to make strict mode work with object-detection patterns:

1)  Make the patterns test using "in" and only set the prop if not already present.
2)  Don't use readonly properties in specifications, ever (either use writable data props
    or accessor props with a setter).

Note that accessor props with a setter could have a setter that does absolutely nothing; this makes such props behave in strict mode just like outside it.  But it's not clearly a win to do that, really, since it's just manually circumventing strict mode.  Writable data props are less of a circumvention, I guess.  Of course it's not like teaching the world to write less-concise detection code to work in strict mode is easy either.  Any specific proposals to deal with strict mode?

Comment 13

[Brendan Eich [:brendan]]

Web standards shouldn't mix readonly properties into the global object -- even ignoring the var indexedDB = ... || window.indexedDB; pattern, preempting names used for global vars by a script written 8 years ago is bad for web compat.

Given this, accessors with dummy setters aren't enough. For utmost compat we would want the setter to udpate the hidden data slot that the getter returns.

/be

Comment 14

[Cameron McCormack (:heycam)]

(In reply to Brendan Eich [:brendan] from comment #13)
> Given this, accessors with dummy setters aren't enough. For utmost compat we
> would want the setter to udpate the hidden data slot that the getter returns.

That is what [Replaceable] does, for the limited set of IDL attributes that it is specified on.  But it configures a shadowing data property when the setter is invoked, rather than updating the hidden state.

Comment 15

[Brendan Eich [:brendan]]

(In reply to Cameron McCormack (:heycam) (limited attention August 13 - 17) from comment #14)
> (In reply to Brendan Eich [:brendan] from comment #13)
> > Given this, accessors with dummy setters aren't enough. For utmost compat we
> > would want the setter to udpate the hidden data slot that the getter returns.
> 
> That is what [Replaceable] does, for the limited set of IDL attributes that
> it is specified on.  But it configures a shadowing data property when the
> setter is invoked, rather than updating the hidden state.

That'll do -- comment 12 talked about "2) Don't use readonly properties" in a way that seemed to be for-all-objects, but my comment 13 focused on the global object. The global object needs [[Replaceable]] most. Would a conventions and style guide cover this, or do we need more spec language singling out the global exception?

/be

Comment 16

[Cameron McCormack (:heycam)]

I think it would be better to treat all IDL attributes on the interface that the global object implements as [Replaceable], rather than requiring HTML (and other specs) to put it on all of them.  Since [Replaceable] is only used on Window etc., we could drop it from the spec altogether.  We then need some spec wording to indicate that Window etc. are implemented by the global object, either in prose or by adding a [Global] or Window in the IDL.

Comment 17

[Brendan Eich [:brendan]]

(In reply to Cameron McCormack (:heycam) (limited attention August 13 - 17) from comment #16)
> I think it would be better to treat all IDL attributes on the interface that
> the global object implements as [Replaceable], rather than requiring HTML
> (and other specs) to put it on all of them.

Yes, agreed -- normative spec language rather than advisory conventions/styles. That's what I was trying to say in comment 15.

> Since [Replaceable] is only
> used on Window etc., we could drop it from the spec altogether.  We then
> need some spec wording to indicate that Window etc. are implemented by the
> global object, either in prose or by adding a [Global] or Window in the IDL.

Cool, I was not sure [Replaceable] was used only on window-extending interfaces and Window. Best to make the special case simple and avoid magic [qualifiers] that might be attractive nuisances.

/be

Comment 18

[Boris Zbarsky [:bzbarsky]]

We can't just auto-default [Replaceable] without having a way to opt out because the existing non-replaceable properties need to stay non-replaceable, right?  There was certainly a lot of angst about minimizing the number of replaceable properties at one point (mostly from other engines; I think Gecko had one of the larger sets).

Or do we think it would be ok to make everything that's not [Unforgeable] be [Replaceable]?

In any case, that discussion is worth having separately (on public-script-coord, probably, though I'm not sure how much Apple and Google are really active there), but indexedDB could be marked [Replaceable] in the meantime.

Comment 19

[Allen Wirfs-Brock]

(In reply to Boris Zbarsky (:bz) [In and out Aug 1 - 10, out Aug 11-20] from comment #18)
> We can't just auto-default [Replaceable] without having a way to opt out
> because the existing non-replaceable properties need to stay
> non-replaceable, right?  There was certainly a lot of angst about minimizing
> the number of replaceable properties at one point (mostly from other
> engines; I think Gecko had one of the larger sets).
> 
> Or do we think it would be ok to make everything that's not [Unforgeable] be
> [Replaceable]?
> 

this sounds right to me.  For the global object, [Replaeable] should be the default and [Unforgeable] should be the explicit opt-out of that default.  This would bring the WebIDL defaults for the global scope in line with the ES defaults.

WRT angst about [Replaceable] as the default, I think it is probably important to keep emphasizing we are only talking about interfaces of the global object not all WebIDL interfaces.

Comment 20

[Boris Zbarsky [:bzbarsky]]

The angst was specifically about properties of the global object.  Worth reading the existing W3C bug reports about replaceable, and the ones where we changed some things that used to be replaceable to not be so, to align with other UAs.

Note that making something that didn't use to be replaceable in UAs be replaceable has compat risks, so it's not something to be done lightly.  The path of least compat resistance is to grandfather in existing non-replaceable properties and make new things being added replaceable.

Comment 21

[Cameron McCormack (:heycam)]

Are the ones we can't make effectively [Replaceable] just the same ones that are currently marked [Unforgeable]?

Comment 22

[Boris Zbarsky [:bzbarsky]]

"can't" is a strong statement.

The ones that are _risky_ to make replaceable are the ones that are not already either [Replaceable] or [Unforgeable].  There are quite a lot of properties like that.

Comment 23

[Jason Orendorff [:jorendorff]]

  <script>
    var indexedDB = window.indexedDB || window.mozIndexedDB
    if (typeof indexedDB !== 'object') alert('FAIL');
  </script>

As far as I can tell, this is the motivating test case here, and the agreed-upon fix is to make indexedDB and others into own properties of the global.

http://lists.w3.org/Archives/Public/public-script-coord/2012JulSep/0096.html

So I think we should bounce this bug to the DOM and settle any remaining JS engine issues in specific follow-up bugs (with test cases, ideally). Objections?

Comment 24

[cscott]

Ran into this today: bug 805794.

Comment 25

[cscott]

Note that in my testing Chrome has different behavior for window.webkitIndexedDB and window.webkitRequestAnimationFrame -- I'm assuming that the JS spec work will also tell chrome which of these is wrong?  http://code.google.com/p/chromium/issues/detail?id=158049

Comment 26

[Boris Zbarsky [:bzbarsky]]

As a DOM bug, this would be a dup of bug 770844, I think.

Comment 27

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Repurposing this bug.