632003 - var statement should not pay attention to properties on the prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Attachment: HTML test case

As a result of the bug 539488 the var statement does not create a variable in the global object if the property exists on the prototype as the attached test case demonstrates.

Although this is what ES5 dictates, I believe this is a bug in the specs and deviates from the function declarations behavior (see bug 628298), where recently ES5 errata was added to explicitly use HasOwnProperty, not HasProperty, when dealing with variables. It is also a regression form Firefox 3.6. Also Opera 11.1 and Google Chrome 9.0.597.84 behaves as Firefox 3.6.

Comment 1

[Igor Bukanov]

ES5 specs alreday has been corrected with an errata, see the new step 8.d.ii at hthe end of https://mail.mozilla.org/pipermail/es5-discuss/2011-January/003882.html

Comment 2

[Robert Sayre]

Igor, this is a hardblocker. Do you have time to work on this?

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Igor, this is a hardblocker. Do you have time to work on this?

I think a two-liner patch would address the issue, but I would like to check few other related things. But if that investigation should wait, I can have a patch and try server results on Friday.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
 
> I think a two-liner patch would address the issue, but I would like to check
> few other related things. But if that investigation should wait, I can have a
> patch and try server results on Friday.

I meant on Wednesday, not Friday!

Comment 5

[Igor Bukanov]

Attachment: v1

The patch makes JSOP_DEFVAR to ignore prototype properties. In addition there is a CheckRedeclaration cleanup.

In particular, the patch removes the CheckRedeclaration call from methodjit/StubCalls.cpp as the corresponding call in the interpreter was removed in bug 522158. This allowed to remove JSPROP_INITIALIZER and the corresponding code in CheckRedeclaration. Another observation was that the out parameters for obj/prop for the function are no longer necessary so the patch removes them as well.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 511236 [details] [diff] [review]
v1

>diff --git a/js/src/jsutil.h b/js/src/jsutil.h

>+#define JS_ALWAYS_FALSE(expr) JS_ASSERT(!expr)

Good to see this, I'm pretty sure I've added at least one place which would have used this had it been around (probably should have added it myself, really).


>diff --git a/js/src/methodjit/StubCalls.cpp b/js/src/methodjit/StubCalls.cpp

>-    /*
>-     * Check for property redeclaration strict warning (we may be in an object
>-     * initialiser, not an array initialiser).
>-     */
>-    if (!CheckRedeclaration(cx, obj, id, JSPROP_INITIALIZER, NULL, NULL))
>-        THROW();
>+    /* No need to check for duplicate property; the compiler already did. */

Don't sharp variables (ugh) belie this change?  Recall that they let partially-constructed object literals escape.


>diff --git a/js/src/tests/ecma_5/misc/regress-bug632003.js b/js/src/tests/ecma_5/misc/regress-bug632003.js

>+Object.defineProperty(Object.prototype, "p1", {value:1});
>+Object.defineProperty(Object.prototype, "p2", {value:2,writable: true});
>+Object.defineProperty(Object.prototype, "p3", {value:3,writable: true, configurable: true});
>+Object.defineProperty(Object.prototype, "p4", {get: function() { return 4; }});
>+Object.defineProperty(Object.prototype, "p5", {get: function() { return 4; },
>+					       set: function() { }});

Why not cover all the cases, for fullest certainty?  Add the {value, configurable:true} case, symmetric value cases adding {enumerable:true}, a {set} case, and the accessor cases where you have one or both halves plus {enumerable:true} or {configurable:true} or {enumerable:true, configurable:true}, please.  It's kind of a lot, yes, but they should be quick to write, and I think we want that peace of mind (particularly now).

>+Object.defineProperty(Object.prototype, "q1", {value:1});
>+Object.defineProperty(Object.prototype, "q2", {value:2,writable: true});
>+Object.defineProperty(Object.prototype, "q3", {value:3,writable: true, configurable: true});
>+Object.defineProperty(Object.prototype, "q4", {get: function() { return 4; }});
>+Object.defineProperty(Object.prototype, "q5", {get: function() { return 4; },
>+					       set: function() { }});

Likewise here.

Comment 7

[Jeff Walden [:Waldo]]

...which I guess means the change in bug 522158 was wrong, and I (!) shouldn't have +'d it.  Unless I'm wrong here, which I don't believe I am.  Whether that mistake can be leveraged into observably bad behavior, I'm not sure, but I think we need to give it another look, here or a new bug, your choice.

Comment 8

[Igor Bukanov]

Attachment: v2

The new version adds coverage for all cases of property descriptors. I have kept CheckRedeclaration chnages from the prev patch intact but the patch does remove comments about "no need to check for dups". The issue about what to do with sharp variables that are used to alter the partially initialized object is left for the bug 633177.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 511388 [details] [diff] [review]
v2

Did you forget to qref?  Bugzilla claims it's the same patch.

Comment 10

[Igor Bukanov]

Attachment: v2 for real

Comment 11

[Igor Bukanov]

(In reply to comment #9)
> Did you forget to qref?  Bugzilla claims it's the same patch.

It was worse - I was fixing the issues with v1 in a unrelated patch applied on top of this patch in my mq. Now the proper patch is attached and the diff between v2 for real and v1 shows the progress.

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 511559 [details] [diff] [review]
v2 for real

>+        /*
>+         * As attrs includes readonly, CheckRedeclaration can succeed only
>+         * if prop does not exist.
>+         */
>+        shouldDefine = true;

Add a JS_ASSERT(!obj->nativeLookup(id)) here.

Comment 13

[Jeff Walden [:Waldo]]

...or not, I guess, since that's probably not cool for vars inside with.  Bleh.

Comment 14

[Igor Bukanov]

(In reply to comment #13)
> ...or not, I guess, since that's probably not cool for vars inside with.  Bleh.

With is not a problem as all var declarations are hoisted to the top level of a script or function. The problem is a proxy on the prototype of a global that makes lookupProperty to return false while adding a property to the global. That will be fixed in the bug 628298, but then we have resolve hooks that execute JS code and that potentially may add a property to the global while reporting false.

Comment 15

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/38536e8a2194

Comment 16

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> With is not a problem as all var declarations are hoisted to the top level of a
> script or function. The problem is a proxy on the prototype of a global that
> makes lookupProperty to return false while adding a property to the global.

But nativeLookup only looks at directly own, already-existing properties, right?  So why would a proxy on the prototype chain, or a resolve hook on the object, matter?

Comment 17

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/38536e8a2194

Comment 18

[Igor Bukanov]

(In reply to comment #16)
> But nativeLookup only looks at directly own, already-existing properties,
> right?  So why would a proxy on the prototype chain, or a resolve hook on the
> object, matter?

Proxy on prototype can return false from its has method while adding a property to the global object that nativeLookup will see.

Comment 19

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Hm. Pretty sure this caused bug 638838.

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

And bug 638768.