Closed Bug 632003 Opened 14 years ago Closed 14 years ago

var statement should not pay attention to properties on the prototype

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: igor, Assigned: igor)

References

()

Details

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

Attachments

(2 files, 2 obsolete files)

Attached file HTML test case (deleted) —
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
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
Igor, this is a hardblocker. Do you have time to work on this?
(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.
(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!
Attached patch v1 (obsolete) (deleted) — Splinter Review
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)
Whiteboard: hardblocker → hardblocker [has patch]
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-
...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.
Attached patch v2 (obsolete) (deleted) — Splinter Review
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 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)
Attached patch v2 for real (deleted) — Splinter Review
Attachment #511388 - Attachment is obsolete: true
Attachment #511559 - Flags: review?(jwalden+bmo)
(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.
(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.
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
Closed: 14 years ago
Resolution: --- → FIXED
(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
Depends on: 638768
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: