The issue is that Object.defineProperty(obj, propname, desc) does validation on desc. Specifically, if desc is this: var desc = { value: 17, writable: false, configurable: true, enumerable: true }; but script has done Object.prototype.get = 42; then Object.defineProperty validation of the descriptor will fail, because the descriptor has bits to appear to be both a data descriptor and an accessor descriptor. One workaround is to create the descriptor using std_Object_create(null). But really an intrinsic for adding/creating data properties is more desirable. So we should probably do that instead, here, as I don't think there's super-huge rush to immediately immediately fix the correctness issue here.

Test: Object.prototype.get = 5; try { new Intl.Collator().resolvedOptions(); print("PASS"); } catch (e) { print("FAIL"); }

As discussed. I moved some of the #defines from Utilities.js, the next patch will contain those for property descriptor attributes.

Introduces _DefineValueProperty, removes std_Object_defineProperty and replaces all uses of the latter with the former. Plus test from comment 1.

Comment on attachment 8398764 [details] [diff] [review] Part 1: Extract self-hosting #defines into builtin/SelfHostingDefines.h Review of attachment 8398764 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SelfHostingDefines.h @@ +17,5 @@ > +/* Assertions */ > +#ifdef DEBUG > +#define assert(b, info) if (!(b)) AssertionFailed(info) > +#else > +#define assert(b, info) Conceivably /* elided assertion */ might be nice here @@ +20,5 @@ > +#else > +#define assert(b, info) > +#endif > + > +/* Safe versions of ARRAY.push(ELEMENT) */ If these existed beforehand, okay. But given that the semantics of Array.prototype.push are -- according to the spec, if not to our implementation, as I recall -- supposed to invoke prototypal setters, I'm not sure this is the right primitive to expose, nor the right description of behavior to use. Followup bug.

Comment on attachment 8398765 [details] [diff] [review] Part 2: Add safe _DefineValueProperty self-hosting intrinsic. Review of attachment 8398765 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/SelfHostingDefines.h @@ +29,5 @@ > > +// Property descriptor attributes. > +#define ATTR_ENUMERABLE 1 > +#define ATTR_CONFIGURABLE 2 > +#define ATTR_WRITABLE 4 0x1, 0x2, 0x4 might make clearer this is a bit field. I would rather see the exact flags passed in at each site, specified explicitly, without implied defaults. That is, I'd rather see (ENUMERABLE | CONFIGURABLE | NONWRITABLE) or so at each location. I'm not sure about the best interface for this -- just adding NON* versions set to 0x0 for this would work, but there's no code enforcement of the policy, so I suspect it'd fall by the side over time. Having an ATTRS(enum, config, writ) helper that combines the flags together and requires passing all of them is prone to argument mis-ordering. You have any ideas here, that are better than adding NON* defines and doing our best to enforce use of them at review time? ::: js/src/jit-test/tests/self-hosting/define-value-property.js @@ +4,5 @@ > + new Intl.Collator().resolvedOptions(); > + } > + catch (e) > + { > + assertTrue(false, "Mustn't be reached"); Might as well simplify to just: Object.prototype.get = 5; new Intl.Collator().resolvedOptions(); The try-catch was just to make for nicer success/failure observations in the bug. But a thrown exception here is plenty good for indicating there's a bug. ::: js/src/jsobj.cpp @@ +943,5 @@ > if (!desc || !desc->initialize(cx, descriptor)) > return false; > > + printf("attr: %d\n", desc->attributes()); > + Doubtful this is wanted any more. :-) ::: js/src/vm/SelfHosting.cpp @@ +445,5 @@ > + > + if (args.length() != 4) { > + JS_ReportError(cx, "Incorrect number of arguments, exactly 4 required"); > + return false; > + } MOZ_ASSERT this, tests will exercise and ensure this. @@ +453,5 @@ > + > + RootedObject obj(cx, &args[0].toObject()); > + if (obj->is<ProxyObject>()) { > + JS_ReportError(cx, "_DefineValueProperty can't be used on proxies"); > + return false; Technically this might be enforceable at code-writing time, but it does seem slightly more prone to mistake, so an extra check seems fair enough.

Asking for re-review, because the patch changed substantially. I introduced NON* versions for all defineProperty attributes and gave them individual flags. _DefineValueProperty asserts that, for all attributes, either the YES-version xor the NON*-version is set. I also null'd Object.defineProperty at the beginning of Utilities.js so that it is extremely unlikely that someone accidentally re-introduces std_Object_defineProperty. If you think this setup is good, I'll apply it to defineProperties and Object.create, next. Try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=476d6d483229

Comment on attachment 8402194 [details] [diff] [review] Part 2: Add safe _DefineValueProperty self-hosting intrinsic. v2 Review of attachment 8402194 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, this seems fine/better. ::: js/src/builtin/Utilities.js @@ +24,5 @@ > */ > > #include "SelfHostingDefines.h" > > +// Remove built-in functions that're unsafe to use. "Remove unsafe builtin functions" is slightly more concise, avoids a slightly awkward/somewhat-ungrammatical contraction. ::: js/src/vm/SelfHosting.cpp @@ +462,5 @@ > + > + unsigned resolvedAttributes = JSPROP_PERMANENT | JSPROP_READONLY; > + > + MOZ_ASSERT((attributes & ATTR_ENUMERABLE && !(attributes & ATTR_NONENUMERABLE)) || > + attributes & ATTR_NONENUMERABLE, bool(attributes & ATTR_ENUMERABLE) != bool(attributes & ATTR_NONENUMERABLE) seems slightly more readable. (Or ! instead of bool if you want to be concise, but I think I like bool better.) @@ +468,5 @@ > + if (attributes & ATTR_ENUMERABLE) > + resolvedAttributes |= JSPROP_ENUMERATE; > + > + MOZ_ASSERT((attributes & ATTR_CONFIGURABLE && !(attributes & ATTR_NONCONFIGURABLE)) || > + attributes & ATTR_NONCONFIGURABLE, Same form here. @@ +475,5 @@ > + if (attributes & ATTR_CONFIGURABLE) > + resolvedAttributes &= ~JSPROP_PERMANENT; > + > + MOZ_ASSERT((attributes & ATTR_WRITABLE && !(attributes & ATTR_NONWRITABLE)) || > + attributes & ATTR_NONWRITABLE, Same form here.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/35134eeaf242 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fb38cd386f8c

https://hg.mozilla.org/mozilla-central/rev/35134eeaf242 https://hg.mozilla.org/mozilla-central/rev/fb38cd386f8c

Comment on attachment 8406190 [details] [diff] [review] Test for the Intl property Review of attachment 8406190 [details] [diff] [review]: ----------------------------------------------------------------- r=me