var statement should not pay attention to properties on the prototype

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

(Depends on: 1 bug, {regression})

Trunk
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: hardblocker [has patch] fixed-in-tracemonkey, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 510259 [details]
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.
blocking2.0: ? → betaN+
Whiteboard: hardblocker
(Assignee)

Comment 1

8 years ago
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

8 years ago
Igor, this is a hardblocker. Do you have time to work on this?
(Assignee)

Comment 3

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
(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!
(Assignee)

Comment 5

8 years ago
Created attachment 511236 [details] [diff] [review]
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.
Attachment #511236 - Flags: review?(jwalden+bmo)

Updated

8 years ago
Whiteboard: hardblocker → hardblocker [has patch]

Comment 6

8 years ago
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.
Attachment #511236 - Flags: review?(jwalden+bmo) → review-

Comment 7

8 years ago
...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.
(Assignee)

Comment 8

8 years ago
Created attachment 511388 [details] [diff] [review]
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.
Attachment #511236 - Attachment is obsolete: true
Attachment #511388 - Flags: review?(jwalden+bmo)

Comment 9

8 years ago
Comment on attachment 511388 [details] [diff] [review]
v2

Did you forget to qref?  Bugzilla claims it's the same patch.
Attachment #511388 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 10

8 years ago
Created attachment 511559 [details] [diff] [review]
v2 for real
Attachment #511388 - Attachment is obsolete: true
Attachment #511559 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 11

8 years ago
(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 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.
Attachment #511559 - Flags: review?(jwalden+bmo) → review+
...or not, I guess, since that's probably not cool for vars inside with.  Bleh.
(Assignee)

Comment 14

8 years ago
(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.
(Assignee)

Comment 15

8 years ago
http://hg.mozilla.org/tracemonkey/rev/38536e8a2194
Whiteboard: hardblocker [has patch] → hardblocker [has patch] fixed-in-tracemonkey
(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?
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

8 years ago
(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.
Blocks: 635252
Hm. Pretty sure this caused bug 638838.
Blocks: 638838
No longer blocks: 638838
Depends on: 638838

Updated

8 years ago
Depends on: 638768
You need to log in before you can comment on or make changes to this bug.