603201 - Strict getters on primitive wrapper prototypes receive wrapped |this| values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Jim Blandy :jimb]

According to ES5, this should pass:

function strictThis() { 'use strict'; return this; }
Object.defineProperty(Boolean.prototype, 'strict', { get: strictThis });
assertEq(true.strict,  true);

In SpiderMonkey, the primitive gets wrapped in the JSOP_GETPROP case, but the getters don't get invoked until several calls in from there (youngest first):

ExternalInvoke
js::ExternalGetOrSet
js::Shape::get
js_NativeGetInline
js_GetPropertyHelperWithShapeInline
js_GetPropertyHelperInline
js_GetPropertyHelper
js::Interpret

None of these take a 'this' value distinct from the object whose method is
being invoked, as the custom [[Get]] in ES5 8.7.1 requires. Somehow, the property needs to be looked up on the primitive wrapper constructor, but the 'this' must remain a primitive.

Comment 1

[Jim Blandy :jimb]

Attachment: Test that strict getters on primitives receive unwrapped |this| values.

Comment 2

[Jim Blandy :jimb]

Perhaps we could have a new getHow flag indicating that the |this| was originally a primitive, let the wrapper object make its way through most of the dispatch stack, and unwrap the primitive before calling js::Shape::get?  js::Shape::get, js::Shape::set, and js::ExternalGetOrSet would need to change to take a Value instead of a JSObject *.

Comment 3

[Brendan Eich [:brendan]]

JSOP_CALLPROP changed for bug 365851 (IIRC) -- rationale was perf then, not strict mode, but same idea applies to JSOP_GETPROP too.

It would be great to use Value not JSObject* for |this| everywhere, and avoid bugs that love to hide in half-measures (my half-measures, anyway ;-).

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: proposed fix

This passes the getter test, and this getter+setter test, with -m, -j, and plain old interpreter:

function strictThis() { 'use strict'; return this; }
var glob;
Object.defineProperty(Boolean.prototype, 'strict',
  {get:strictThis,set:function(x){"use strict";glob=this}});
assertEq(true.strict, true, 'get');
true.strict = 42;
assertEq(glob, true, 'set true');
false.strict = 43;
assertEq(glob, false, 'set false');

The patch avoids adding net-extra parameters where possible (e.g. by folding strict into js_SetPropertyHelper's defineHow flags-parameter).

/be

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 482454 [details] [diff] [review]
proposed fix

Andreas, could you look at the jsproxy.cpp changes? I think the non-use of receiver was a bug there. Not covered by tests?

/be

Comment 6

[Brendan Eich [:brendan]]

(In reply to comment #4)
> The patch avoids adding net-extra parameters where possible (e.g. by folding
> strict into js_SetPropertyHelper's defineHow flags-parameter).

Another example is ExternalGetOrSet, which gains a thisv param while losing the JSAccessMode mode deadwood (which was inexplicably held over from bug 515475).

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: patch I hacked up to rebase bug 514570 patch queue

Forgot to say the proposed fix for this bug is based on the four patches in bug 514570, plus this patch (I hope it's correct -- trace-tests seem happy).

/be

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 482454 [details] [diff] [review]
proposed fix

>   do_getprop_with_lval:
>-    VALUE_TO_OBJECT(cx, vp, obj);
>-
>     {
>+        const Value &lval = *vp;

Nasty aliasing here, I consulted with Luke and changed this lval to be of Value type, so a copy, just to avoid any compiler or reader confusion. *vp is written later, after lval is effectively dead, but why even raise the possibility of viewing an updated *vp via lval-as-const-ref?

New patch if I make any other change, or maybe just for clarity's sake, in a bit.

/be

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 482454 [details] [diff] [review]
proposed fix

Still want gal to look at the proxy fixage, but new patch coming for jimb later today.

/be

Comment 10

[Andreas Gal :gal]

k, will look at the final patch

Comment 11

[Jim Blandy :jimb]

(In reply to comment #7)
> Created attachment 482458 [details] [diff] [review]
> patch I hacked up to rebase bug 514570 patch queue
> 
> Forgot to say the proposed fix for this bug is based on the four patches in bug
> 514570, plus this patch (I hope it's correct -- trace-tests seem happy).

This has been resolved, right?  (mq confusion)

Comment 12

[Brendan Eich [:brendan]]

(In reply to comment #11)
> (In reply to comment #7)
> > Created attachment 482458 [details] [diff] [review] [details]
> > patch I hacked up to rebase bug 514570 patch queue
> > 
> > Forgot to say the proposed fix for this bug is based on the four patches in bug
> > 514570, plus this patch (I hope it's correct -- trace-tests seem happy).
> 
> This has been resolved, right?  (mq confusion)

I think so -- it was late, I was using patch --fuzz=N --dry-run, and probably failed to strip off the dry-run :-(.

/be

Comment 13

[Brendan Eich [:brendan]]

Getting back to this, on top of mrbkap's work. Need to finish es5 so asking for betaN+.

/be

Comment 14

[Brendan Eich [:brendan]]

I will look into this but it's not going into b11 so it will probably miss moz2 and fx4. Will get it next time and early in (short!) beta cycle.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: passes shell tests, pushing to try: -a

I should say, this passes jit-test and jsreftests in the shell, but these five tests failed in a run-all-reftests attempt:

ecma_5/Object/15.2.3.6-dictionary-redefinition-1-of-8.js
ecma_5/Object/15.2.3.6-dictionary-redefinition-2-of-8.js
ecma_5/Object/15.2.3.6-dictionary-redefinition-3-of-8.js
ecma_5/Object/15.2.3.6-dictionary-redefinition-5-of-8.js
ecma_5/Object/15.2.3.6-dictionary-redefinition-6-of-8.js

Putting these lines in a file and feeding it to jstests.py -f, the five tests pass. I guess they timed out in the full run due to VM or other OS effects propagating from earlier shells to the shells that ran these in the full sequence.

Andreas, I need your close attention to jsproxy.cpp and xpconnect/wrappers changes.

This is far from trivial or easy to justify with one mini-beta to go. It's also arguably central enough that our tests cover it all. The other worry is perf loss due to extra thisv/receiver argument passing, which I hope try: -a measures well.

If this does ding perf, then I suggest we not take it, and redo all our property access code to work like JSC. Right now (due to how this evolved from "Mocha" in 1995, which had data properties and js::PropertyOp-precursor native accessors), we call the getter or setter under a deep flow from the GET or SET opcode through obj->{g,s}etProperty.

This requires the extra parameter. It also requires the JITs to deconstruct these ops and lookup the property first, to specialize for getter and setter calls.

The JSC way is to have lookup return a property descriptor, which is like an ECMA Reference type or a get{,Own}PropertyDescriptor return value. This design unwinds the get-property-descriptor stack before calling the getter or setter if it's an accessor, thus separating the decision of what receiver to pass (strict code -> primitive |this| value is not coerced) from the property descriptor lookup.

This lets everyone, interp and JITs, share more code, in theory. In practice it may be only somewhat cleaner since we already deconstruct everywhere (even in the interpreter for property cache specializations that pre-date the JITs; only the API layer benefits from the lookup-and-then-access combo), but it is definitely worth trying out later (after fx4). It's part of our goal to align SpiderMonkey's internal MOP with ES5's (and there's an effort to align ES5's OP with the Harmony Proxy MOP).

Therefore if the patch I'm attaching here hurts perf, then we're looking at later for fixing this bug.

/be

Comment 16

[Brendan Eich [:brendan]]

> SpiderMonkey's internal MOP with ES5's (and there's an effort to align ES5's OP

s/ES5's OP/ES's MOP/

The patch breaks xray wrappers by passing receiver, as the comments I removed warned against. D'oh! Fixed, will attach new patch soon.

Also, ecma_5/strict/15.5.5.2.js first testLenientAndStrict fails in the browser only, not the shell -- eval wrapper badness? Those tests are nicely concise thanks to functional programming, but they fail inscrutably, failing to say which of strict_pred and lenient_pred failed. Trying to isolate...

/be

Comment 17

[Jeff Walden [:Waldo]]

(In reply to comment #14)
> I will look into this but it's not going into b11 so it will probably miss moz2
> and fx4. Will get it next time and early in (short!) beta cycle.

I can probably live with this ES5 mis-following if I have to, for this release, although I would much prefer to fix it, since it seems quite doable to fix.

At the same time, if I'd known you were pressed to deal with this, I'd have been more than happy to take if off your hands as the last patch to completely finish strict |this| behavior.  Saying so earlier would have seen this done by now, I believe.

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> At the same time, if I'd known you were pressed to deal with this, I'd have
> been more than happy to take if off your hands as the last patch to completely
> finish strict |this| behavior.  Saying so earlier would have seen this done by
> now, I believe.

What exactly would you have given up to do this work?

It's always fair to steal bugs like this (I took it and the array length ones off the full-time ES5 team's hands because those hands were full), but what exactly were you working on that was less important?

Look at the patch. It's not nearly as easy as you seemed to say you thought it was when we spoke on IRC the other day. If we need to redo our MOP to fix this right, it's a clean miss. If I can get this patch beaten into shape, then get busy reviewing. Anything else is just talk.

/be

Comment 19

[Brendan Eich [:brendan]]

Attachment: fixed proxy receiver and stubs::SetElem bugs, re-try: -a'ing

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #18)
> What exactly would you have given up to do this work?

I dispute the premise.  There are many ongoing things which need to be done, to some extent, which I could temporarily forego to push another patch through.  Things like keeping fully up-to-date on newsgroups, or bugmail (although I've been deferring completely timely bugmail reading for something going on seven months now), or occasional brownbags on particularly interesting topics all count as such.  I could justify to myself not doing any of these things if this were done to fix a specific, valuable bug.

I should also note that when my mind turns to mush from a particular task, I take a quick break from it.  Bug 514568 occupied the majority of my attention for a long time, but when I just wasn't getting work done on it I would often switch to something else for a few hours, or a day, or whatever, to make progress *somewhere*.  A bug like this fits those intervals nicely.

(What filled those intervals at the time?  The bugs I had, for starters, but beyond that usually it devolved to the less-productive tasks which could be foregone for something clearly better, like a bug someone else apparently had under control but which was more in limbo than otherwise.)

> I took it and the array length ones off the full-time ES5 team's hands
> because those hands were full

I'm sorry to say this wasn't apparent to me.  When I see ASSIGNED, I take that to mean someone else has taken on the effort of fixing the bug, and I should leave them alone to do that, unless they request help or throw it back to the pool.

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> (In reply to comment #18)
> > What exactly would you have given up to do this work?
> 
> I dispute the premise.

What premise, that there's no free lunch? Or that the other bugs fixed so far for this release are strictly more important than this bug?

If you think something less important should have waited, name it. Better to have named it already since hindsight is 20-20.

If you think something is not getting done that is higher priority than other things that are getting done, say so when there is time to reprioritize. That's everyone's job.

If we do not agree on priorities we'll have to figure that out, but there's no point doing it too late.

Anyway, what's the big deal with not fixing this bug? You were praising our ES5 support on IRC two weeks ago, dismissing the remaining bugs as corner cases. You were bailing on bug 591059 by 591059 comment 3.

Don't write another long comment here. Instead start reviewing the patch. Ok?

/be

Comment 22

[Brendan Eich [:brendan]]

Watch http://tbpl.mozilla.org/?tree=MozillaTry&rev=94f45d117d75 for results.

/be

Comment 23

[Jeff Walden [:Waldo]]

(In reply to comment #21)
> You were praising our ES5 support on IRC two weeks ago, dismissing the
> remaining bugs as corner cases.

I might have meant strict mode.  If I meant that, it was conditioned on the seemingly imminent fix here.

If I didn't, I misspoke with respect *at least* to this bug and bug 591059.  My memory isn't perfect.

> You were bailing on bug 591059 by 591059 comment 3.

That bug is Hard To Fix without special-casing in Object.defineProperty, a very complex algorithm which I didn't understand at the time even with "exhaustive" tests that bring developer machines to a near-halt.  In some ways I still feel I don't fully understand it.  I am uncertain whether I'm happier with it unfixed or fixed with hairy hacks.  (I assume either way it's fixed later with changes to arrays to permit them to use the regular Object.defineProperty logic.)  Regardless of that uncertainty, I have reviewed and will review patches/etc. for the hack fix.

> start reviewing the patch.

You have only to make the request.  :-)

Comment 24

[Brendan Eich [:brendan]]

Waldo, give it up. This isn't about you, your hero-in-your-own-mind status is not relevant. We have this bug, it has a patch. Self-aggrandizing comments are *not* welcome.

/be

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #22)
> Watch http://tbpl.mozilla.org/?tree=MozillaTry&rev=94f45d117d75 for results.

There's a DecompileCode crash reached from a js_ReportValueErrorFlags call in js_GetPropertyHelperWithShapeInline on "Oth" mochitests, I will try to reproduce and debug.

The Linux DEBUG-only "J" jsreftest failures are crashes but no stack backtraces appear in the log file. Anyone able to help? I'm Mac-only atm.

/be

Comment 26

[Andreas Gal :gal]

Comment on attachment 510070 [details] [diff] [review]
fixed proxy receiver and stubs::SetElem bugs, re-try: -a'ing

Not a big fan of the name "ValuePropertyBearer". js_NativeSet might fit into one line and since you are already touching it, how about just js::NativeSet? Lets get Jessy to fuzz this.

Comment 27

[Brendan Eich [:brendan]]

(In reply to comment #26)
> Comment on attachment 510070 [details] [diff] [review]
> fixed proxy receiver and stubs::SetElem bugs, re-try: -a'ing
> 
> Not a big fan of the name "ValuePropertyBearer".

It's jimb's name (already in the tree). How about "ValueToPropertyBearer", since noun phrase names are supposed to be infallible getters and this is a fallible getter.

> js_NativeSet might fit into
> one line and since you are already touching it, how about just js::NativeSet?
> Lets get Jessy to fuzz this.

Let me get a new patch up and past try: -a. Have fixes, testing at home now.

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: fixed patch

Had to fix a latent JSOP_GETLOCALPROP js_DecompileValueGenerator bug to get past the jit-tests.

/be

Comment 29

[Brendan Eich [:brendan]]

Comment on attachment 510094 [details] [diff] [review]
fixed patch

Just for the methodjit bits, anything else you can look at. Thanks,

/be

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 510070 [details] [diff] [review]
fixed proxy receiver and stubs::SetElem bugs, re-try: -a'ing

(Note: I know they exist by there being new bugmail for this bug in my bugmail folder, but I have not read changes subsequent to my last comment.  I worry they're a continuation of our past flame war.  I hope they're not.  Either way, consider this patch review [which I've been working on non-stop since posting that comment, save for maybe half an hour of dinner and simultaneous patch-reading], done with this promptness, a peace offering of sorts.)

(Further note after trying to submit this review: so it seems those changes include a new version of the patch.  Hopefully the new version's not too different from this one.  I guess I can look at the extra differences when I review the next iteration of this patch, and I'll cancel the review on the newer patch and hope all my concerns here have not been addressed in it [seems unlikely, but you never know].)


>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

>@@ -3128,51 +3128,54 @@ BEGIN_CASE(JSOP_FORLOCAL)

>+    Value rval;
>+    if (!IteratorNext(cx, &regs.sp[-1].toObject(), &rval))
>+        goto error;
>+    if (!obj->setProperty(cx, id, &rval, script->strictModeCode))
>+        goto error;

Bill pointed out an interesting case recently where failure to initialize a temporary meant a same-compartment assertion would start complaining when it got uninitialized data.  At this late date perhaps initialize just in case IteratorNext ever assumes (apparently wrongly, but still) rval is initialized?  Just paranoia, no problem suspected, don't think this must be changed if you'd rather not.


>         if (lval.isObject()) {
>-            if (!js_GetMethod(cx, &objv.toObject(), id,
>-                              JS_LIKELY(!aobj->getOps()->getProperty)
>+            if (!js_GetMethod(cx, pobj, id,
>+                              JS_LIKELY(!pobj->getOps()->getProperty)
>                               ? JSGET_CACHE_RESULT | JSGET_NO_METHOD_BARRIER
>                               : JSGET_NO_METHOD_BARRIER,
>                               &rval)) {
>                 goto error;
>             }
>-            regs.sp[-1] = objv;
>-            regs.sp[-2] = rval;
>-            assertSameCompartment(cx, regs.sp[-1], regs.sp[-2]);
>         } else {
>-            JS_ASSERT(!objv.toObject().getOps()->getProperty);
>-            if (!js_GetPropertyHelper(cx, &objv.toObject(), id,
>+            JS_ASSERT(!pobj->getOps()->getProperty);
>+            if (!js_GetPropertyHelper(cx, pobj, lval, id,
>                                       JSGET_CACHE_RESULT | JSGET_NO_METHOD_BARRIER,
>                                       &rval)) {
>                 goto error;
>             }
>-            regs.sp[-1] = lval;
>-            regs.sp[-2] = rval;
>-            assertSameCompartment(cx, regs.sp[-1], regs.sp[-2]);
>         }
>+        assertSameCompartment(cx, lval, rval);
>+        regs.sp[-1] = lval;
>+        regs.sp[-2] = rval;

Holy Code Duplication Batman!  I wonder when/how that slipped in.  (Consider this repeated one or two other places in the patch.)


>@@ -4440,60 +4434,61 @@ BEGIN_CASE(JSOP_SETMETHOD)
> 
>             LOAD_ATOM(0, atom);
>         } else {
>             JS_ASSERT(atom);
>         }
> 
>         jsid id = ATOM_TO_JSID(atom);
>         if (entry && JS_LIKELY(!obj->getOps()->setProperty)) {
>-            uintN defineHow;
>+            uintN defineHow = uintN(script->strictModeCode);

I assume this puns some JSDNP_* constant?  (Later: yes, yes it does.)  Please make this explicit, compiler will eat the overlay for breakfast.  (And to be sure, is the overlay likely to help in this or a couple other instances anyway?  That member's from a bitfield as I recall, so it's going to have to convert that bit into a low bit here anyway, which would lose the potential optimization.)


> BEGIN_CASE(JSOP_SETELEM)
> {
>-    JSObject *obj;
>-    FETCH_OBJECT(cx, -3, obj);
>+    const Value &lref = regs.sp[-3];
>+    JSObject *obj = ValuePropertyBearer(cx, lref, -3);
>+    if (!obj)
>+        goto error;
>+
>     jsid id;
>     FETCH_ELEMENT_ID(obj, -2, id);
>+
>     Value rval;
>     do {
>+        if (lref.isString()) {
>+            if (script->strictModeCode) {
>+                obj->reportReadOnly(cx, id);
>+                goto error;
>+            }
>+            if (JSID_IS_INT(id) && !js_PrototypeHasIndexedProperties(cx, obj))
>+                goto end_setelem;
>+            break;
>+        }

A testcase which appears to me to fail with this code, where it should properly pass (see 8.7.2's special [[Put]] algorithm, step 6):

  "use strict";
  var s = "string";
  var set = false;
  Object.defineProperty(String.prototype, "foopy",
                        {
                          set: function(v)
                          {
                            assertEq(this, "string");
                            set = true;
                          }
                        });
  s.foopy = 17;
  assertEq(set, true);

Tangentially, I could have sworn WebKit didn't have this bug (and distinctly remember testing to be sure), for some reason it's failing that test.  Looks like I should report this to them.


>@@ -5955,22 +5977,23 @@ BEGIN_CASE(JSOP_INITMETHOD)
>         PCMETER(JS_PROPERTY_CACHE(cx).inipcmisses++);
> 
>         /* Get the immediate property name into id. */
>         JSAtom *atom;
>         LOAD_ATOM(0, atom);
>         jsid id = ATOM_TO_JSID(atom);
> 
>         /* No need to check for duplicate property; the compiler already did. */
>-
>-        uintN defineHow = (op == JSOP_INITMETHOD)
>-                          ? JSDNP_CACHE_RESULT | JSDNP_SET_METHOD
>-                          : JSDNP_CACHE_RESULT;
>+        uintN defineHow = uintN(script->strictModeCode)
>+                          | ((op == JSOP_INITMETHOD)
>+                             ? JSDNP_CACHE_RESULT | JSDNP_SET_METHOD
>+                             : JSDNP_CACHE_RESULT);

Same overlaying comment again.


>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>@@ -5471,28 +5471,29 @@ js::GetPropertyDefault(JSContext *cx, JS
> }
> 
> JSBool
> js_GetMethod(JSContext *cx, JSObject *obj, jsid id, uintN getHow, Value *vp)
> {
>     JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED);
> 
>     PropertyIdOp op = obj->getOps()->getProperty;
>+    const Value &thisv = ObjectValue(*obj);

Is this taking a reference to a compiler-created temporary?


>     if (!op) {
> #if JS_HAS_XML_SUPPORT
>         JS_ASSERT(!obj->isXML());
> #endif
>-        return js_GetPropertyHelper(cx, obj, id, getHow, vp);
>+        return js_GetPropertyHelper(cx, obj, thisv, id, getHow, vp);
>     }
>     JS_ASSERT_IF(getHow & JSGET_CACHE_RESULT, obj->isDenseArray());
> #if JS_HAS_XML_SUPPORT
>     if (obj->isXML())
>         return js_GetXMLMethod(cx, obj, id, vp);
> #endif
>-    return op(cx, obj, obj, id, vp);
>+    return op(cx, obj, thisv, id, vp);

If so, might thisv be bogus in the two subsequent uses?  I might be missing something, or be wrong about how C++ says this should work, but I would think just |Value thisv| is the way to go.



> JSBool
>-js_SetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp, JSBool strict)
>-{
>-    return js_SetPropertyHelper(cx, obj, id, 0, vp, strict);
>+js_SetProperty(JSContext *cx, JSObject *obj, const Value &thisv, jsid id, Value *vp,
>+               JSBool strict)
>+{
>+    return js_SetPropertyHelper(cx, obj, thisv, id, uintN(strict), vp);
> }

Same overlaying comment again.


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

> extern JSBool
>-js_GetProperty(JSContext *cx, JSObject *obj, JSObject *receiver, jsid id, js::Value *vp);
>+js_GetProperty(JSContext *cx, JSObject *obj, const js::Value &thisv, jsid id, js::Value *vp);
> 
> inline JSBool
> js_GetProperty(JSContext *cx, JSObject *obj, jsid id, js::Value *vp)
> {
>-    return js_GetProperty(cx, obj, obj, id, vp);
>+    return js_GetProperty(cx, obj, js::ObjectValue(*obj), id, vp);
> }

I worry about the availability of shortcuts like this leading to people unintentionally implementing algorithms wrongly.  It's happened before.  But I guess without the shortcut there's still a fair chance of bad copy-paste happening, so I suppose there's no good option with this setup.  So maybe this is okay to keep around.


> /*
>- * Flags for the defineHow parameter of js_DefineNativeProperty.
>+ * Flags for the defineHow parameter of js_DefineNativeProperty and also (since
>+ * it may attempt to define a property) js_SetPropertyHelper.
>  */
>-const uintN JSDNP_CACHE_RESULT = 1; /* an interpreter call from JSOP_INITPROP */
>-const uintN JSDNP_DONT_PURGE   = 2; /* suppress js_PurgeScopeChain */
>-const uintN JSDNP_SET_METHOD   = 4; /* js_{DefineNativeProperty,SetPropertyHelper}
>+const uintN JSDNP_STRICT_MODE  = 1; /* NB: same as JS_TRUE for JSBool strict */
>+const uintN JSDNP_CACHE_RESULT = 2; /* an interpreter call from JSOP_INITPROP */
>+const uintN JSDNP_DONT_PURGE   = 4; /* suppress js_PurgeScopeChain */
>+const uintN JSDNP_SET_METHOD   = 8; /* js_{DefineNativeProperty,SetPropertyHelper}
>                                        must pass the js::Shape::METHOD
>                                        flag on to JSObject::{add,put}Property */
>-const uintN JSDNP_UNQUALIFIED  = 8; /* Unqualified property set.  Only used in
>-                                       the defineHow argument of
>+const uintN JSDNP_UNQUALIFIED = 16; /* Unqualified property set. Used only with
>                                        js_SetPropertyHelper. */

The NB: isn't needed (or could be altered to indicate the correspondence is possibly an optimization) if the handful of places that implicitly rely on this property just make it explicit.


>@@ -1697,29 +1710,44 @@ const uintN JSGET_NO_METHOD_BARRIER = 2;
> 
> /*
>  * NB: js_NativeGet and js_NativeSet are called with the scope containing shape
>  * (pobj's scope for Get, obj's for Set) locked, and on successful return, that
>  * scope is again locked.  But on failure, both functions return false with the
>  * scope containing shape unlocked.
>  */

This comment still refers to the old parameter names.  But moreover, it looks obsolete to me -- remove it?


>diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp

I can't tell -- are these changes intended to be in this patch?  I don't see how they make sense as part of this bug; if they do, could you add an extensions/ test that fails before and passes after?  What they fix is not obvious to me.


>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp

> bool
>-JSProxyHandler::get(JSContext *cx, JSObject *proxy, JSObject *receiver, jsid id, Value *vp)
>+JSProxyHandler::get(JSContext *cx, JSObject *proxy, const Value &receiver, jsid id, Value *vp)
> {
>     JS_ASSERT(OperationInProgress(cx, proxy));
>     AutoPropertyDescriptorRooter desc(cx);
>     if (!getPropertyDescriptor(cx, proxy, id, false, &desc))
>         return false;
>     if (!desc.obj) {
>         vp->setUndefined();
>         return true;
>     }
>     if (!desc.getter ||
>         (!(desc.attrs & JSPROP_GETTER) && desc.getter == PropertyStub)) {
>         *vp = desc.value;
>         return true;
>     }
>-    if (desc.attrs & JSPROP_GETTER) {
>-        return ExternalGetOrSet(cx, receiver, id, CastAsObjectJsval(desc.getter),
>-                                JSACC_READ, 0, NULL, vp);
>-    }
>+    if (desc.attrs & JSPROP_GETTER)
>+        return ExternalGetOrSet(cx, receiver, id, CastAsObjectJsval(desc.getter), 0, NULL, vp);
>     if (!(desc.attrs & JSPROP_SHARED))
>         *vp = desc.value;
>     else
>         vp->setUndefined();
>     if (desc.attrs & JSPROP_SHORTID)
>         id = INT_TO_JSID(desc.shortid);
>-    return CallJSPropertyOp(cx, desc.getter, receiver, id, vp);
>+    return CallJSPropertyOp(cx, desc.getter, &receiver.toObject(), id, vp);
> }

I'm not certain that the |receiver.toObject()| bits are correct here (or in the other proxy changes), although I could easily be wrong.  Isn't all this code intended to deal with the case where |Boolean.prototype.__proto__| is changed to a proxy?  (Ugh.)  That would *appear* to me to produce badness here.  Or maybe I'm just reading all this wrong; proxies are not my strong suit.  Tests in js1_8_5/extensions/ would probably demonstrate correctness (or otherwise) in this concern.

This also makes me wonder if proxies need strictness propagated into them.  Or something.  But this even further outside my areas of competence.

> static JSBool
>-proxy_SetProperty(JSContext *cx, JSObject *obj, jsid id, Value *vp, JSBool strict)
>+proxy_SetProperty(JSContext *cx, JSObject *obj, const Value &thisv, jsid id, Value *vp,
>+                  JSBool strict)
> {
>     // FIXME (bug 596351): throwing away strict.
>-    return JSProxy::set(cx, obj, obj, id, vp);
>+    return JSProxy::set(cx, obj, thisv, id, vp);
> }

Ah, I guess they do!  Or something of similar substance.  I don't think we need to worry about getting this correct here, of course (although we should make sure we don't crash or anything, natch).


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

>         if (entry->vword.isFunObj()) {
>             rval.setObject(entry->vword.toFunObj());
>         } else if (entry->vword.isSlot()) {
>             uint32 slot = entry->vword.toSlot();
>             rval = obj2->nativeGetSlot(slot);
>         } else {
>             JS_ASSERT(entry->vword.isShape());
>             const Shape *shape = entry->vword.toShape();
>-            NATIVE_GET(cx, &objv.toObject(), obj2, shape, JSGET_NO_METHOD_BARRIER, &rval,
>-                       THROW());
>+            NATIVE_GET(cx, pobj, obj2, shape, JSGET_NO_METHOD_BARRIER, &rval, THROW());

Shouldn't this be passing along the |this| provided at the call site?  This makes me more than a bit twitchy.


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

>-#define NATIVE_SET(cx,obj,shape,entry,vp)                                     \
>+#define NATIVE_SET(cx,obj,thisv,shape,entry,vp)                               \
>     JS_BEGIN_MACRO                                                            \
>         if (shape->hasDefaultSetter() &&                                      \
>             (shape)->slot != SHAPE_INVALID_SLOT &&                            \
>             !(obj)->brandedOrHasMethodBarrier()) {                            \
>             /* Fast path for, e.g., plain Object instance properties. */      \
>             (obj)->nativeSetSlot((shape)->slot, *vp);                         \
>         } else {                                                              \
>-            if (!js_NativeSet(cx, obj, shape, false, vp))                     \
>+            if (!js_NativeSet(cx, obj, thisv, shape, false, vp))              \
>                 THROW();                                                      \
>         }                                                                     \
>     JS_END_MACRO
> 
> #define NATIVE_GET(cx,obj,pobj,shape,getHow,vp,onerr)                         \
>     JS_BEGIN_MACRO                                                            \
>         if (shape->isDataDescriptor() && shape->hasDefaultGetter()) {         \
>             /* Fast path for Object instance properties. */                   \

Yes, I think so.  NATIVE_GET is unchanged, which only works, or so it appears, because js_NativeGet has the shortcut method that converts an |obj| parameter into a |ObjectValue(*obj)| parameter.


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

>@@ -118,17 +118,17 @@ stubs::BindGlobalName(VMFrame &f)
> template<JSBool strict>
> void JS_FASTCALL
> stubs::SetName(VMFrame &f, JSAtom *origAtom)
> {
>     JSContext *cx = f.cx;
> 
>     Value rval = f.regs.sp[-1];
>     Value &lref = f.regs.sp[-2];
>-    JSObject *obj = ValueToObject(cx, &lref);
>+    JSObject *obj = ValuePropertyBearer(cx, lref, -2);
>     if (!obj)
>         THROW();

Could you enlighten me as to why this change is necessary?  I would have thought setname would always have pushed the global object, which of course wouldn't be a primitive in need of [[Prototype]]-lookup.  Of course it was necessary for the interpreter, but that was really only because JSOP_SETNAME shares its implementation with JSOP_SETPROP, which of course needs this.


>@@ -245,47 +245,48 @@ stubs::SetName(VMFrame &f, JSAtom *origA
> 
>             atom = origAtom;
>         } else {
>             JS_ASSERT(atom);
>         }
> 
>         jsid id = ATOM_TO_JSID(atom);
>         if (entry && JS_LIKELY(!obj->getOps()->setProperty)) {
>-            uintN defineHow;
>+            uintN defineHow = uintN(strict);

Same comment as before about being explicit.


>@@ -553,48 +562,58 @@ stubs::CallElem(VMFrame &f)
> 
> template<JSBool strict>
> void JS_FASTCALL
> stubs::SetElem(VMFrame &f)
...
>     do {
>+        if (lval.isString()) {
>+            if (f.fp()->script()->strictModeCode) {
>+                obj->reportReadOnly(cx, id);
>+                THROW();
>+            }
>+            if (JSID_IS_INT(id) && !js_PrototypeHasIndexedProperties(cx, obj))
>+                goto end_setelem;
>+            break;
>+        }

This bit of course has the same problem (if I diagnosed correctly) the corresponding interpreter code had  -- just noting so it's less likely to be forgotten.


>@@ -1888,36 +1916,36 @@ InlineGetProp(VMFrame &f)
>             if (entry->vword.isFunObj()) {
>                 rval.setObject(entry->vword.toFunObj());
>             } else if (entry->vword.isSlot()) {
>                 uint32 slot = entry->vword.toSlot();
>                 rval = obj2->nativeGetSlot(slot);
>             } else {
>                 JS_ASSERT(entry->vword.isShape());
>                 const Shape *shape = entry->vword.toShape();
>-                NATIVE_GET(cx, obj, obj2, shape,
>+                NATIVE_GET(cx, lval, obj2, shape,
>                         f.fp()->hasImacropc() ? JSGET_NO_METHOD_BARRIER : JSGET_METHOD_BARRIER,
>                         &rval, return false);

This is interesting -- so you have the NATIVE_GET macro "invoked" with both Value and JSObject* for the "type" of the second argument!  And the macro is bifurcating into calling the two different js_NativeGet overloads based on which is passed in.  Gotta love macros...



>         /* No need to check for duplicate property; the compiler already did. */
>+        uintN defineHow = uintN(f.fp()->script()->strictModeCode)
>+                          | ((op == JSOP_INITMETHOD)
>+                             ? JSDNP_CACHE_RESULT | JSDNP_SET_METHOD
>+                             : JSDNP_CACHE_RESULT);

Again, explicit please.


>diff --git a/js/src/tests/ecma_5/strict/15.5.5.2.js b/js/src/tests/ecma_5/strict/15.5.5.2.js

> assertEq(testLenientAndStrict('"foo"[0] = 1',
>                               returns(1), raisesException(TypeError)),
>          true);
> assertEq(testLenientAndStrict('delete "foo"[0]',
>                               returns(false), raisesException(TypeError)),
>          true);
> 
>+assertEq(testLenientAndStrict('(new String("foo"))[0] = 1',
>+                              returns(1), raisesException(TypeError)),
>+         true);
>+assertEq(testLenientAndStrict('delete (new String("foo"))[0]',
>+                              returns(false), raisesException(TypeError)),
>+         true);
>+
> reportCompare(true, true);

Mind adding in variations of these tests for an index greater than the string length, an index equal to the string length, and for "length" itself?  The first two are just normal don't-exist properties, so they should assign and delete just fine; the last should behave the same way as the case for an index less than the string length.


>diff --git a/js/src/tests/ecma_5/strict/8.7.1.js b/js/src/tests/ecma_5/strict/8.7.1.js

>+Object.defineProperty(Boolean.prototype, strict, { get: strictThis, set: strictSaveThis });
>+assertEq(true.strict,  true);
>+assertEq(false[strict], false);
>+assertEq((true.strict   = 1, savedThis), true);
>+assertEq((false[strict] = 1, savedThis), false);

Couldn't hurt to assert that the assignment expression equals 1, just as a safety check, since we're changing a fair bit of code (here, and throughout the test, for all the different mutate-y things).  It'd also help slightly with readability, as I mis-parsed this the first time I read it.


>+assertEq((--true.strict,     savedSetArg), 0);
>+assertEq((true[strict]--,    savedSetArg), 0);
>+assertEq((++false.strict,    savedSetArg), 1);
>+assertEq((false[strict]++,   savedSetArg), 1);
>+
>+Object.defineProperty(Number.prototype,  strict, { get: strictThis, set: strictSaveThis });
>+assertEq((42).strict, 42);
>+assertEq(43[strict],  43);
>+assertEq(((44).strict = 1, savedThis), 44);
>+assertEq((45[strict]  = 1, savedThis), 45);
>+
>+assertEq((--(46).strict,   savedSetArg), 45);
>+assertEq((47[strict]--,    savedSetArg), 46);
>+assertEq((++(46).strict,   savedSetArg), 47);
>+assertEq((47[strict]++,    savedSetArg), 48);
>+
>+Object.defineProperty(String.prototype,  strict, { get: strictThis, set: strictSaveThis });
>+assertEq("a".strict, "a");
>+assertEq("b"[strict], "b");
>+assertEq(("c".strict  = 1, savedThis), "c");
>+assertEq(("d"[strict] = 1, savedThis), "d");
>+
>+assertEq((--"46".strict,   savedSetArg), 45);
>+assertEq(("47"[strict]--,  savedSetArg), 46);
>+assertEq((++"46".strict,   savedSetArg), 47);
>+assertEq(("47"[strict]++,  savedSetArg), 48);

The string tests are at parity with the boolean/number tests, but since strings boxed up have indexes and a length property that are a bit funky, we should probably test all those variants (index less than length, index equal to length, index greater than length, "length") here for proper behavior.


>+function lenientThis()      { 'use lenient'; return this; }
>+function lenientSaveThis(x) { 'use lenient'; savedThis = this; savedSetArg = x; }

Any reason for the important-looking-but-not-really 'use lenient', versus just having nothing?

Same comments as for the strict versions apply to the lenient ones.

Probably we should have tests for the special for-in variants and all.  Ideally they'd be as rigorous as the tests here (which are quite fine, in my opinion).  However, since it's the same ValuePropertyBearer mechanism as for all these, I don't think it's necessary to test all of the number/boolean/string versions for correctness.


I'm generally reasonably satisfied with this patch, but the one bug I think I saw warrants another review.  And if I'm right about the proxies stuff being wrong, that would need it even more, for being value-typing violation.

Comment 31

[Brendan Eich [:brendan]]

Attachment: handle all JOF_VARPROP ops in js_DVG

http://tbpl.mozilla.org/?tree=MozillaTry&rev=2c39a2dbbe97

/be

Comment 32

[Brendan Eich [:brendan]]

Comment on attachment 510097 [details] [diff] [review]
handle all JOF_VARPROP ops in js_DVG

That last push to try missed the generalization from JSOP_GETLOCALPROP, d'oh.

/be

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #30)
> Comment on attachment 510070 [details] [diff] [review]
> >diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
> 
> >@@ -3128,51 +3128,54 @@ BEGIN_CASE(JSOP_FORLOCAL)
> 
> >+    Value rval;
> >+    if (!IteratorNext(cx, &regs.sp[-1].toObject(), &rval))
> >+        goto error;
> >+    if (!obj->setProperty(cx, id, &rval, script->strictModeCode))
> >+        goto error;
> 
> Bill pointed out an interesting case recently where failure to initialize a
> temporary meant a same-compartment assertion would start complaining when it
> got uninitialized data.  At this late date perhaps initialize just in case
> IteratorNext ever assumes (apparently wrongly, but still) rval is initialized? 
> Just paranoia, no problem suspected, don't think this must be changed if you'd
> rather not.

I could just leave the tvr code in place. This was in my mq for a long time (I worked on this bug last year), so I kept it.

But rval is an out param, not an in-out param. This isn't checked by C++ but it's obligatory, so any compartment assertions have to cope.

Rather than spin wheels here I'll probably just revert to the tvr code, evne though we don't need a tvr here.

> 
> 
> >         if (lval.isObject()) {
> >-            if (!js_GetMethod(cx, &objv.toObject(), id,
> >-                              JS_LIKELY(!aobj->getOps()->getProperty)
> >+            if (!js_GetMethod(cx, pobj, id,
> >+                              JS_LIKELY(!pobj->getOps()->getProperty)
> >                               ? JSGET_CACHE_RESULT | JSGET_NO_METHOD_BARRIER
> >                               : JSGET_NO_METHOD_BARRIER,
> >                               &rval)) {
> >                 goto error;
> >             }
> >-            regs.sp[-1] = objv;
> >-            regs.sp[-2] = rval;
> >-            assertSameCompartment(cx, regs.sp[-1], regs.sp[-2]);
> >         } else {
> >-            JS_ASSERT(!objv.toObject().getOps()->getProperty);
> >-            if (!js_GetPropertyHelper(cx, &objv.toObject(), id,
> >+            JS_ASSERT(!pobj->getOps()->getProperty);
> >+            if (!js_GetPropertyHelper(cx, pobj, lval, id,
> >                                       JSGET_CACHE_RESULT | JSGET_NO_METHOD_BARRIER,
> >                                       &rval)) {
> >                 goto error;
> >             }
> >-            regs.sp[-1] = lval;
> >-            regs.sp[-2] = rval;
> >-            assertSameCompartment(cx, regs.sp[-1], regs.sp[-2]);
> >         }
> >+        assertSameCompartment(cx, lval, rval);
> >+        regs.sp[-1] = lval;
> >+        regs.sp[-2] = rval;
> 
> Holy Code Duplication Batman!  I wonder when/how that slipped in.  (Consider
> this repeated one or two other places in the patch.)

What duplication? I'm unduplicating by fusing the common tail. Did you want some change here?

> >+            uintN defineHow = uintN(script->strictModeCode);
> 
> I assume this puns some JSDNP_* constant?  (Later: yes, yes it does.)  Please
> make this explicit, compiler will eat the overlay for breakfast.  (And to be
> sure, is the overlay likely to help in this or a couple other instances anyway?

The point is not to help compilers optimize but to avoid a lengthier ternary expression here and in several other places.

> > BEGIN_CASE(JSOP_SETELEM)
> > {
> >-    JSObject *obj;
> >-    FETCH_OBJECT(cx, -3, obj);
> >+    const Value &lref = regs.sp[-3];
> >+    JSObject *obj = ValuePropertyBearer(cx, lref, -3);
> >+    if (!obj)
> >+        goto error;
> >+
> >     jsid id;
> >     FETCH_ELEMENT_ID(obj, -2, id);
> >+
> >     Value rval;
> >     do {
> >+        if (lref.isString()) {
> >+            if (script->strictModeCode) {
> >+                obj->reportReadOnly(cx, id);
> >+                goto error;
> >+            }
> >+            if (JSID_IS_INT(id) && !js_PrototypeHasIndexedProperties(cx, obj))
> >+                goto end_setelem;
> >+            break;
> >+        }
> 
> A testcase which appears to me to fail with this code, where it should properly
> pass (see 8.7.2's special [[Put]] algorithm, step 6):
> 
>   "use strict";
>   var s = "string";
>   var set = false;
>   Object.defineProperty(String.prototype, "foopy",
>                         {
>                           set: function(v)
>                           {
>                             assertEq(this, "string");
>                             set = true;
>                           }
>                         });
>   s.foopy = 17;
>   assertEq(set, true);

This testcase passes (you can apply the patch and run directly). You aren't testing JSOP_SETELEM here.

> >diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
> 
> >@@ -5471,28 +5471,29 @@ js::GetPropertyDefault(JSContext *cx, JS
> > }
> > 
> > JSBool
> > js_GetMethod(JSContext *cx, JSObject *obj, jsid id, uintN getHow, Value *vp)
> > {
> >     JSAutoResolveFlags rf(cx, JSRESOLVE_QUALIFIED);
> > 
> >     PropertyIdOp op = obj->getOps()->getProperty;
> >+    const Value &thisv = ObjectValue(*obj);
> 
> Is this taking a reference to a compiler-created temporary?

Gack. Late night hoisting over-hack? Don't remember.

> >     if (!op) {
> > #if JS_HAS_XML_SUPPORT
> >         JS_ASSERT(!obj->isXML());
> > #endif
> >-        return js_GetPropertyHelper(cx, obj, id, getHow, vp);
> >+        return js_GetPropertyHelper(cx, obj, thisv, id, getHow, vp);
> >     }
> >     JS_ASSERT_IF(getHow & JSGET_CACHE_RESULT, obj->isDenseArray());
> > #if JS_HAS_XML_SUPPORT
> >     if (obj->isXML())
> >         return js_GetXMLMethod(cx, obj, id, vp);
> > #endif
> >-    return op(cx, obj, obj, id, vp);
> >+    return op(cx, obj, thisv, id, vp);
> 
> If so, might thisv be bogus in the two subsequent uses?  I might be missing
> something, or be wrong about how C++ says this should work, but I would think
> just |Value thisv| is the way to go.

It seems to work but I will fix it.

> I worry about the availability of shortcuts like this leading to people
> unintentionally implementing algorithms wrongly.  It's happened before.  But I
> guess without the shortcut there's still a fair chance of bad copy-paste
> happening, so I suppose there's no good option with this setup.  So maybe this
> is okay to keep around.

I did this because of the places that don't have a primitive this issue, which are too many. Tests save us, the later MOP cleanup will remove this completely.

> >@@ -1697,29 +1710,44 @@ const uintN JSGET_NO_METHOD_BARRIER = 2;
> > 
> > /*
> >  * NB: js_NativeGet and js_NativeSet are called with the scope containing shape
> >  * (pobj's scope for Get, obj's for Set) locked, and on successful return, that
> >  * scope is again locked.  But on failure, both functions return false with the
> >  * scope containing shape unlocked.
> >  */
> 
> This comment still refers to the old parameter names.  But moreover, it looks
> obsolete to me -- remove it?

It's way old but I didn't touch it (or evidently even read it!). I can remove it.

> >diff --git a/js/src/jsopcode.cpp b/js/src/jsopcode.cpp
> 
> I can't tell -- are these changes intended to be in this patch?  I don't see
> how they make sense as part of this bug; if they do, could you add an
> extensions/ test that fails before and passes after?  What they fix is not
> obvious to me.

There's a latent bug as noted in comment 28, but later patches have the right (and much shorter) fix.

> I'm not certain that the |receiver.toObject()| bits are correct here (or in the
> other proxy changes), although I could easily be wrong.  Isn't all this code
> intended to deal with the case where |Boolean.prototype.__proto__| is changed
> to a proxy?  (Ugh.)  That would *appear* to me to produce badness here.  Or
> maybe I'm just reading all this wrong; proxies are not my strong suit.  Tests
> in js1_8_5/extensions/ would probably demonstrate correctness (or otherwise) in
> this concern.

Conferring with gal, proxies cannot take primitive receivers. I will address this in a new patch.

> This also makes me wonder if proxies need strictness propagated into them.

ES5 strict is a subset of Harmony, so yes. I think this ought to be a followup bug, though.

> >diff --git a/js/src/methodjit/PolyIC.cpp b/js/src/methodjit/PolyIC.cpp
> 
> >         if (entry->vword.isFunObj()) {
> >             rval.setObject(entry->vword.toFunObj());
> >         } else if (entry->vword.isSlot()) {
> >             uint32 slot = entry->vword.toSlot();
> >             rval = obj2->nativeGetSlot(slot);
> >         } else {
> >             JS_ASSERT(entry->vword.isShape());
> >             const Shape *shape = entry->vword.toShape();
> >-            NATIVE_GET(cx, &objv.toObject(), obj2, shape, JSGET_NO_METHOD_BARRIER, &rval,
> >-                       THROW());
> >+            NATIVE_GET(cx, pobj, obj2, shape, JSGET_NO_METHOD_BARRIER, &rval, THROW());
> 
> Shouldn't this be passing along the |this| provided at the call site?

Good catch, this should match the interpreter.

> Yes, I think so.  NATIVE_GET is unchanged, which only works, or so it appears,
> because js_NativeGet has the shortcut method that converts an |obj| parameter
> into a |ObjectValue(*obj)| parameter.

Yes, the macro expands to an overloaded call.

We should totally get rid of NATIVE_GET and NATIVE_SET (and all the methodjit duplication of code) after fx4 branches.

> >diff --git a/js/src/methodjit/StubCalls.cpp b/js/src/methodjit/StubCalls.cpp
> 
> >@@ -118,17 +118,17 @@ stubs::BindGlobalName(VMFrame &f)
> > template<JSBool strict>
> > void JS_FASTCALL
> > stubs::SetName(VMFrame &f, JSAtom *origAtom)
> > {
> >     JSContext *cx = f.cx;
> > 
> >     Value rval = f.regs.sp[-1];
> >     Value &lref = f.regs.sp[-2];
> >-    JSObject *obj = ValueToObject(cx, &lref);
> >+    JSObject *obj = ValuePropertyBearer(cx, lref, -2);
> >     if (!obj)
> >         THROW();
> 
> Could you enlighten me as to why this change is necessary?

See methodjit/Compiler.cpp -- used for several ops including SETPROP.

> Any reason for the important-looking-but-not-really 'use lenient', versus just
> having nothing?

Jim wrote this part of the test (see first attachment). I'll work on the tests some more.

Thanks for the review -- bugzilla interdiff should work on future patches to allow delta re-review.

/be

Comment 34

[Brendan Eich [:brendan]]

(In reply to comment #33)
> > >diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp
> > 
> > >@@ -3128,51 +3128,54 @@ BEGIN_CASE(JSOP_FORLOCAL)
> > 
> > >+    Value rval;
> > >+    if (!IteratorNext(cx, &regs.sp[-1].toObject(), &rval))
> > >+        goto error;
 . . .
> Rather than spin wheels here I'll probably just revert to the tvr code, evne
> though we don't need a tvr here.

There's no bug here, nor any need for the old tvr or to initialize rval. We use temps throughout the interpreter this way (uninitialized at local decl site and immediately passed as out param to be initialized). Neither IteratorNext nor js_IteratorNext uses *rval before calling rval->setString(...) or *rval = ....

Whatever Bill ran into, it's not relevant here, and we do not prophylactically over-initialize locals passed immediately by address as out params.

/be

Comment 35

[Jim Blandy :jimb]

(In reply to comment #30)
> >+function lenientThis()      { 'use lenient'; return this; }
> >+function lenientSaveThis(x) { 'use lenient'; savedThis = this; savedSetArg = x; }
> 
> Any reason for the important-looking-but-not-really 'use lenient', versus just
> having nothing?

It was a joke.

Comment 36

[Jeff Walden [:Waldo]]

Comment on attachment 510097 [details] [diff] [review]
handle all JOF_VARPROP ops in js_DVG

(In reply to comment #33)
> What duplication? I'm unduplicating by fusing the common tail. Did you want
> some change here?

No change, just remarking on the duplication that was there until this patch removed it.


> The point is not to help compilers optimize but to avoid a lengthier ternary
> expression here and in several other places.

I think it would be more readable and require the reader to know less to use a ternary.


> This testcase passes (you can apply the patch and run directly). You aren't
> testing JSOP_SETELEM here.

Ah, yes -- the setelem version of that fails:

"use strict";
var s = "string";
var set = false;
Object.defineProperty(String.prototype, "foopy",
                      {
                       	set: function(v)
                        {
                          assertEq(this, "string");
                         set = true;
                        }
                      });
function foopy() { return "foopy"; }
s[foopy()] = 17;
assertEq(set, true);

Too bad it can't be copy-pasted into the shell due to each line being run as its own script, but copying it to and running it from a file isn't that much effort, I guess.


> I did this because of the places that don't have a primitive this issue, which
> are too many. Tests save us, the later MOP cleanup will remove this completely.

You might be right.  Either way, you have more faith in our tests than I do.

Comment 37

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 19 being moved from blocking2.0:betaN+ to blocking2.0:- as we reached the endgame of Firefox 4. The rationale for the move is:

 - the bug had been identified as a "soft" blocker which could be fixed in some follow up release
 - the bug had been identified as one requiring beta coverage, thus is not appropriate for a ".x" stability & security release

The owner of the bug may wish to renominate for .x consideration.

Comment 38

[Mike Beltzner [:beltzner, not reading bugmail]]

(er updating flag to "-" as per previous comment!)

Comment 40

[Mark S. Miller]

On FF Nightly 13.0a1 (2012-03-02) I'm seeing strange new bad behavior not seen on FF10 or on any other browsers:


> Object.defineProperty(Number.prototype, '___test_prop___', { get: function() { return this; }, set: void 0, enumerable: false, configurable: true });
0
> (3).___test_prop___
0
> Object.defineProperty(Number.prototype, '___test_prop___', { get: function() { "use strict"; return this; }, set: void 0, enumerable: false, configurable: true });
0
> (3).___test_prop___
0

The second and fourth zeros are bizarre.

Comment 41

[Jeff Walden [:Waldo]]

Ugh, yeah, that's completely wrong.  Recent-ish regression, filed bug 732669 on it.

Comment 42

[Brendan Eich [:brendan]]

So should this bug be dup'ed against bug 732669 or blocked by it?

/be

Comment 43

[Jeff Walden [:Waldo]]

It's a distinct problem from this one.  Note that this bug only concerns the case where the functions in the getters are strict mode functions, but that bug concerns the case with non-strict mode functions.  A see-also seems like the best thing here.

Comment 44

[Chris Peterson [:cpeterson]]

Waldo: is this bug still relevant?

Comment 45

[Jeff Walden [:Waldo]]

Yes.  It's worth at least a couple test262 points.  I've wanted to get to it for awhile but found other concerns piling up too far to get to it.

Comment 46

[Mark S. Miller]

Why was this downgraded in priority? An important property of ES5 is that strict JavaScript code not cause any implicit wrapping of primitives.

Regardless of priority, what's the status of getting this fixed?

Comment 47

[Jeff Walden [:Waldo]]

It's not clear to me that we've ascribed priority consistent meaning over time, such that P2->P3 represents an actual downgrade.  For what it's worth.

It's on the list of things I want to do soon.  It's lower in priority than some security work, and some other Mozilla infrastructural work, and maybe one or two other things.  I could see it happening in a couple months, maybe.

And for what it's worth, it's not clear to me what makes this an "important" property of ES5, as opposed to merely a property of it.  Implicit boxing is contrary to spec, sure.  But it's hard for me to see how it's likely to break much code, or to have serious impact beyond a trivial bit of performance.

Comment 50

[Tom S [:evilpie]]

Attachment: WIP

This makes a lot of stuff already work. I replaced all the HandleObject receiver parameters of GetProperty/GetElement with a HandleValue. I haven't looked at the JIT or the rest of Gecko yet.

Comment 51

[Jeff Walden [:Waldo]]

Comment on attachment 8477928 [details] [diff] [review]
WIP

Review of attachment 8477928 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/BaselineIC.cpp
@@ +6504,2 @@
>          // Handle when val is an object.
>          RootedObject obj(cx, ToObjectFromStack(cx, val));

We should handle this (and all the similar places) without boxing the value, seems to me.  The semantic fix to accord with spec is nice, but there's no reason we shouldn't do what the spec contemplates and eliminate boxing altogether in this case.

::: js/src/jsobj.h
@@ +1000,5 @@
>                                JSStrictPropertyOp setter = JS_StrictPropertyStub,
>                                unsigned attrs = JSPROP_ENUMERATE);
>  
> +    static bool getGeneric(JSContext *cx, js::HandleObject obj, js::HandleId id,
> +                           js::MutableHandleValue vp)

I'd prefer not having this method -- it was a deliberate omission -- because it's likely to make people not properly handle the case where the receiver isn't identical to the object where the lookup happens.  Better to make everyone explicit about it, and therefore cognizant of the issue.

I imagine this patch would be a lot smaller with, instead,

    static bool
    getGeneric(JSContext *cx, JS::HandleObject obj, JS::HandleObject receiver,
               JS::HandleId id, JS::MutableHandleValue vp)
    {
        JS::RootedValue recv(cx, ObjectValue(*receiver));
        return getGeneric(cx, obj, recv, id, vp);
    }

added to it.

Comment 52

[Tom S [:evilpie]]

Attachment: wip v2

Now with fewer changes.

Comment 53

[Jeff Walden [:Waldo]]

Attachment: Refactor GetPropertyOperation in the interpreter to handle primitive, non-primitive cases separately (no functionality change)

This splitting will be helpful later on, once we stop doing JS::ToObject on primitives in JSOP_GETPROP.  Doesn't change functionality at all.

Comment 54

[Jeff Walden [:Waldo]]

Attachment: Convert property-getting receiver-handling code to deal with Value, not JSObject* -- changes some JSAPI behavior, shouldn't change behavior viewable by script

efaust because he might be able to comment semi-sanely on the JSPropertyOp changes and their correctness or lack thereof.  peterv for the Codegen.py and BindingUtils.* changes -- my local testing says things changed the way I wanted them to, but possibly my technique isn't the 100% desired one.

Comment 55

[Jeff Walden [:Waldo]]

Attachment: Refactor GetElementOperation to call one of two methods to handle gets on primitives and gets on non-primitives

More refactoring akin to the bit for GetPropertyOperation previously.  No functionality change, intentionally, yet.  The IC hits don't call the object version of this operation because they'll eventually be made to do the right thing on primitives as well as objects, for what it's worth.

Comment 56

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8538641 [details] [diff] [review]
Refactor GetElementOperation to call one of two methods to handle gets on primitives and gets on non-primitives

Review of attachment 8538641 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter-inl.h
@@ +427,3 @@
>  {
>      MOZ_ASSERT(op == JSOP_GETELEM || op == JSOP_CALLELEM);
> +    MOZ_ASSERT(receiver.isObject(),

Why not take a JSObject?

Comment 57

[Jeff Walden [:Waldo]]

Attachment: Adjust SetPropertyOperation's signature slightly to be more pleasant

The same sort of splitting as for GetPropertyOperation, really.

Comment 58

[Eric Faust [:efaust]]

Comment on attachment 8539413 [details] [diff] [review]
Adjust SetPropertyOperation's signature slightly to be more pleasant

APPROVED.

Comment 59

[Eric Faust [:efaust]]

Comment on attachment 8538641 [details] [diff] [review]
Refactor GetElementOperation to call one of two methods to handle gets on primitives and gets on non-primitives

Review of attachment 8538641 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with Ms2Ger here. The old method took a JSObject, and I don't see any reason why the changes you made necessitates changing to a value. Can you comment on this? r=me at any rate.

Comment 60

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffa8fdec868d for one of these I landed Saturday.  Rest still to go.

Comment 61

[Peter Van der Beken [:peterv]]

Comment on attachment 8538585 [details] [diff] [review]
Convert property-getting receiver-handling code to deal with Value, not JSObject* -- changes some JSAPI behavior, shouldn't change behavior viewable by script

Review of attachment 8538585 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at BindingUtils.h/cpp and Codegen.py.

::: dom/bindings/Codegen.py
@@ +10137,5 @@
> +                }
> +                """,
> +                computeCondition=computeCondition,
> +                namedGetCode=CGProxyNamedGetter(self.descriptor, templateValues).define())
> +            namedGet += "\n"

Maybe:

            outerCondition = "!ignoreNamedProps"
            if self.descriptor.supportsIndexedProperties():
                outerCondition = "!IsArrayIndex(index) && " + outerCondition

            namedGet = fill("""
                bool callNamedGetter = false;
                if (${outerCondition}) {
                  $*{computeCondition}
                }
                if (callNamedGetter) {
                  $*{namedGetCode}
                }
                """,
                outerCondition=outerCondition,
                computeCondition=computeCondition,
                namedGetCode=CGProxyNamedGetter(self.descriptor, templateValues).define())

@@ +10378,5 @@
> +                    }
> +                    """)
> +
> +                delete = hasPreface + CGIfWrapper(CGGeneric(delete),
> +                                                  "!hasOnProto").define()

No need for the CGIfWrapper anymore:

                hasPreface = fill("""
                    bool hasOnProto;
                    if (!HasPropertyOnPrototype(cx, proxy, id, &hasOnProto)) {
                      return false;
                    }
                    if (!hasOnProto) {
                      $*{delete}
                    }
                    """,
                    delete=delete)

should be equivalent.

@@ +10522,5 @@
> +                    bool hasOnProto;
> +                    if (!HasPropertyOnPrototype(cx, proxy, id, &hasOnProto)) {
> +                      return false;
> +                    }
> +                    if (hasOnProto) {

This should be checking |!hasOnProto|, no?

@@ +10526,5 @@
> +                    if (hasOnProto) {
> +                      $*{protoLacksProperty}
> +                    }
> +                    """,
> +                    protoLacksProperty=CGGeneric(named + "return true;\n").define())

There's reallly no need for the CGGeneric, I'd just do:

                named = fill("""
                    bool hasOnProto;
                    if (!HasPropertyOnPrototype(cx, proxy, id, &hasOnProto)) {
                      return false;
                    }
                    if (!hasOnProto) {
                      $*{protoLacksProperty}
                      return true;
                    }
                    """,
                    protoLacksProperty=named)

Comment 62

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ffa8fdec868d

Comment 63

[Jeff Walden [:Waldo]]

On second look, not sure why I had receiver as Value for the object-only case.  Adjusted.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fd307cbb9d9

Comment 64

[Jeff Walden [:Waldo]]

Attachment: Convert property-getting receiver-handling code to deal with Value, not JSObject* -- changes some JSAPI behavior, shouldn't change behavior viewable by script

Updated for peterv's comments, thanks particularly for the inverted-condition notice.  Thought I'd caught all those by reviewing before/after code closely enough!

Comment 65

[Jeff Walden [:Waldo]]

Attachment: Implement JS_ForwardSetPropertyTo

Needed to forward primitive receiver values correctly in proxy-ful methods.  The boxing is obviously non-standard, but that'll go away later in the fixing here.

Comment 66

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4fd307cbb9d9

Comment 67

[Eric Faust [:efaust]]

Comment on attachment 8541761 [details] [diff] [review]
Convert property-getting receiver-handling code to deal with Value, not JSObject* -- changes some JSAPI behavior, shouldn't change behavior viewable by script

Review of attachment 8541761 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Obviously eventually we want to remove the object parallel signatures, but that's too much cruft for this patch. r=me

::: dom/base/WindowNamedPropertiesHandler.cpp
@@ +97,4 @@
>    }
>  
>    // Grab the DOM window.
> +  JS::Rooted<JSObject*> global(aCx, JS_GetGlobalForObject(aCx, aProxy));

Can you explain why this is necessary? It seems to be rooted above and then never used? Mild bitrot, maybe?

::: dom/bindings/BindingUtils.cpp
@@ +1604,5 @@
> +    *has = false;
> +    return true;
> +  }
> +
> +  return JS_HasPropertyById(cx, proto, id, has);

This is much cleaner than the hack that was there before :)

::: dom/bindings/Codegen.py
@@ +10137,5 @@
> +                """,
> +                outerCondition=outerCondition,
> +                computeCondition=computeCondition,
> +                namedGetCode=CGProxyNamedGetter(self.descriptor, templateValues).define())
> +            namedGet += "\n"

This all looks fine to me. I, of course, defer to Peter.

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +1365,3 @@
>  {
> +  JS::Rooted<JSObject*> obj(cx, JS::ToObject(cx, receiver));
> +  if (!obj)

In theory this also catches |null| and |undefined| as receivers, but I don't see how that can happen from script. If that happens from the C++ side, it's an API violation, so this is fine.

::: js/src/ctypes/CTypes.cpp
@@ +4554,5 @@
>  
>  bool
> +ArrayType::Getter(JSContext* cx, HandleValue receiver, HandleId idval, MutableHandleValue vp)
> +{
> +  if (!receiver.isObject() || !CData::IsCData(&receiver.toObject())) {

Do we want to leave some notion from the old comment that we don't ever expect not to have a CData, or was the comment just bogus? "We'll just check to be sure" feels "never right" to me. I would expect that either it's an assertable thing, or we are checking because we need to.

::: js/src/jsapi-tests/testClassGetter.cpp
@@ +42,5 @@
>      return true;
>  }
> +
> +static bool
> +test_fn(JSContext *cx, unsigned argc, jsval *vp)

r=themaid ;)

::: js/src/jsobj.h
@@ +643,2 @@
>  
>      static inline bool getElement(JSContext *cx, js::HandleObject obj, js::HandleObject receiver,

I understand why these are still here, but in theory shouldn't we aspire to have them not exist?

::: js/src/vm/NativeObject.h
@@ +1420,4 @@
>      return true;
>  }
>  
> +

nit: extra blank line added above.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +2065,5 @@
>  {
>      // Skip our Base if it isn't already ProxyHandler.
>      // NB: None of the functions we call are prepared for the receiver not
>      // being the wrapper, so ignore the receiver here.
> +    JS::Rooted<JS::Value> thisv(cx);

nit: XrayWrapper.cpp's current style is |RootedValue thisv(cx)|.

Comment 68

[Eric Faust [:efaust]]

Comment on attachment 8541786 [details] [diff] [review]
Implement JS_ForwardSetPropertyTo

Review of attachment 8541786 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. r=me with comment addressed.

::: js/src/jsapi-tests/testForwardSetProperty.cpp
@@ +74,5 @@
> +    EXEC("assertEq(foundValue, obj3, 'wrong receiver passed to strict setter');");
> +
> +    CHECK(JS_ForwardSetPropertyTo(cx, obj2, prop, setval, true, setval));
> +
> +    if (false) {

Can we leave more breadcrumbs for ourselves to find this again? I like Jason's approach. Maybe a try/catch where if it manages not to fail the assert then we throw with "please enable the rest of the test"?

Comment 69

[Jeff Walden [:Waldo]]

Feh, I missed (at least) Ion codegen bits for the get-receiver patch.  Debugging now, hopefully will have some progress over the weekend, and a completed stab earlier next week.  Whenever the tree reopens I'll probably be pushing as many of the other patches as I can, tho.

Comment 70

[Jason Orendorff [:jorendorff]]

Comment on attachment 8538087 [details] [diff] [review]
Refactor GetPropertyOperation in the interpreter to handle primitive, non-primitive cases separately (no functionality change)

Review of attachment 8538087 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. We can race to land (you'll win).

::: js/src/vm/Interpreter.cpp
@@ +191,5 @@
> +        NativeObject *proto = GlobalObject::getOrCreateNumberPrototype(cx, global);
> +        if (!proto)
> +            return false;
> +        if (ClassMethodIsNative(cx, proto, &NumberObject::class_, id, js_num_toString))
> +            obj = proto;

Narrow Optimization Hall of Infamy candidate (pre-existing, of course)

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1298539468f2
https://hg.mozilla.org/mozilla-central/rev/0bf28565a3ac
https://hg.mozilla.org/mozilla-central/rev/d0b014d15b00

Comment 73

[Tom S [:evilpie]]

Attachment: Pass around receiver as HandleValue

Just wanted to see how hard this is now. After bug 895223 JSGetterOps (former PropertyOps) always use their holder, so a lot of changes from Waldo's old patch are now unnecessary.

Comment 74

[Tom S [:evilpie]]

Oh there is some obvious bad/unfixed stuff, this just was a test and I didn't want to waste the patch.

Comment 75

[Jason Orendorff [:jorendorff]]

Attachment: partially - Support primitive receivers in assignment

Comment 76

[Jason Orendorff [:jorendorff]]

Attachment: partially - Support primitive receivers in assignment

Comment 77

[Tom S [:evilpie]]

Attachment: Change [[Get]] receiver to Value in js

Comment 78

[Tom S [:evilpie]]

Attachment: Change [[Get]] receiver to Value everywhere else

Comment 79

[Tom S [:evilpie]]

Attachment: some first test

Obviously need more here.

Comment 80

[Tom S [:evilpie]]

Comment on attachment 8590852 [details] [diff] [review]
Change [[Get]] receiver to Value in js

I rewrote most of this from scratch, but I took some stuff from Waldo's patch. It seems like at the VM entry points we sometimes need to reroot like this
+        RootedValue thisv(cx, lref);
+        return GetPrimitiveElementOperation(cx, op, thisv, rref, res);
because lref aliases res?

A comment in GetPrimitiveElementOperation says "We shouldn't be boxing here..", what should we do instead? Lookup the prototype?

Comment 81

[Tom S [:evilpie]]

Comment on attachment 8590854 [details] [diff] [review]
Change [[Get]] receiver to Value everywhere else

Review of attachment 8590854 [details] [diff] [review]:
-----------------------------------------------------------------

Waldo's old patch had a lot more changes in the Codegen.py area, but those changes were already done somewhere else. It seems like GetPropertyOnPrototype was incorrect even for objects, because we never passed the receiver and instead always the proxy.

Comment 82

[Tom S [:evilpie]]

Attachment: Tests for primitive receivers.

Comment 83

[Tom S [:evilpie]]

Waldo made me aware of bug 1118360. While I was critical at first, I am not sure if passing the receiver might cause some unexpected regressions. It seems easier to just box up the proxy as a Value again.

Comment 84

[Eric Faust [:efaust]]

Comment on attachment 8590852 [details] [diff] [review]
Change [[Get]] receiver to Value in js

Review of attachment 8590852 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. I was surprised to see that more wasn't required to get BC working, but looking at it deeper, I agree this looks right. r=me

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ -883,5 @@
>          return false;
>      Value argv[] = {
>          ObjectOrNullValue(target),
>          value,
> -        ObjectOrNullValue(receiver)

idle curiosity: Could receiver really have been null before?

Comment 85

[Eric Faust [:efaust]]

Comment on attachment 8591369 [details] [diff] [review]
Tests for primitive receivers.

Review of attachment 8591369 [details] [diff] [review]:
-----------------------------------------------------------------

Good.

::: js/src/tests/ecma_5/strict/primitive-this-getter.js
@@ +13,5 @@
> +        assertEq(this, value);
> +        return "getter";
> +    }})
> +
> +    assertEq(value.getter, "getter");

Can we put inline comments in this test what each subpiece is testing? strict mode uses this for accessors, even with a proxy, etc.

Comment 86

[Tom S [:evilpie]]

Attachment: Change [[Get]] receiver to Value in js

Rebased

Comment 87

[Tom S [:evilpie]]

Attachment: Change [[Get]] receiver to Value everywhere else

Comment 88

[Tom S [:evilpie]]

Attachment: Tests for primitive receivers with [[Get]].

Comment 89

[Tom S [:evilpie]]

Looking back at the review comments, I forgot to address efaust's feedback on the test patch.

Comment 90

[Tom S [:evilpie]]

I rebased my patches here and changed them so that we always pass around objects boxed as values. This means there is no observable change in behavior, but we only need to modify 2-3 places to get the correct behavior later. I don't think we need a new review here, because the changes are trivial.

Comment 91

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0dd901033bab
https://hg.mozilla.org/integration/mozilla-inbound/rev/813851422272

Comment 92

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0dd901033bab
https://hg.mozilla.org/mozilla-central/rev/813851422272

Comment 93

[Tom S [:evilpie]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce98c9fa1ed0

Comment 94

[Tom S [:evilpie]]

This is just enabling primitive this for [[Get]]. Try looks good, the last push was just based on a bad commit.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b4e64b6bd2

Todo are [[Set]] and tests. I previously gotten r+s from efaust and peterv for my [[Get]] tests.

Comment 95

[Tom S [:evilpie]]

Attachment: Enable primitive receiver for [[Set]]

Comment 96

[Jason Orendorff [:jorendorff]]

Comment on attachment 8697881 [details] [diff] [review]
Enable primitive receiver for [[Set]]

Review of attachment 8697881 [details] [diff] [review]:
-----------------------------------------------------------------

Great patch.

::: js/src/jit/BaselineIC.cpp
@@ +4771,5 @@
>          InitGlobalLexicalOperation(cx, lexicalScope, script, pc, v);
>      } else {
>          MOZ_ASSERT(op == JSOP_SETPROP || op == JSOP_STRICTSETPROP);
>  
> +        RootedValue v(cx, rhs); // why?

That can't be right. Remove it. Ask the original author if you like.

Comment 97

[Tom S [:evilpie]]

Attachment: Enable primitive receiver for [[Get]]

Comment 98

[Tom S [:evilpie]]

Attachment: Tests for primitive receiver [[Get]]

Was already reviewed.

Comment 99

[Jason Orendorff [:jorendorff]]

Comment on attachment 8700620 [details] [diff] [review]
Enable primitive receiver for [[Get]]

Review of attachment 8700620 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter-inl.h
@@ +459,5 @@
>                               HandleValue key, MutableHandleValue res)
>  {
>      MOZ_ASSERT(op == JSOP_GETELEM || op == JSOP_CALLELEM);
>  
> +    // FIXME: We shouldn't be boxing here.

File a follow-up bug and cite it here.

Comment 100

[Tom S [:evilpie]]

Thanks for the reviews. Try looks pretty good except for the obvious b2g failures (of course) https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad2630378e5

Comment 101

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21f8f7248ee7
https://hg.mozilla.org/integration/mozilla-inbound/rev/9652e130726a

Comment 102

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/21f8f7248ee7
https://hg.mozilla.org/mozilla-central/rev/9652e130726a

Comment 103

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2878b3c2378f

Comment 104

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2878b3c2378f

Comment 106

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/setting-properties-on-a-primitive-now-throws-in-strict-mode/

Thanks :bz for flagging!

Comment 107

[Florian Scholz (Open Web Docs)]

Added to
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode
https://developer.mozilla.org/en-US/Firefox/Releases/46#JavaScript