732669 - Primitives don't box correctly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch and test

Mark Miller pointed out in bug 603201 comment 40 a pretty ugly regression -- we don't box primitive values when getting properties from them, rather we replace them with the corresponding boxed-object prototype.  (!)  How this passed our tests I have no idea.

This regressed in bug 712714, which landed before the last merge, so this needs to be fixed in aurora as well as on trunk.

Comment 1

[Brian Hackett [Laid off!]]

Comment on attachment 602578 [details] [diff] [review]
Patch and test

Not a recent regression.  Before bug 712714 JSOP_CALLPROP behaved this way, now JSOP_GETPROP does too.  That's the only reason the code in that comment behaves differently now, we were only partially broken before.

Two other problems:

- This only fixes the interpreter, not the JIT.
- This will destroy performance when accessing prototype properties through primitives, e.g. 'foo'.charAt.

It looks to me like the boxing should happen when the scripted getter is invoked.

Comment 2

[Jeff Walden [:Waldo]]

(In reply to Brian Hackett (:bhackett) from comment #1)
> Not a recent regression.  Before bug 712714 JSOP_CALLPROP behaved this way,
> now JSOP_GETPROP does too.  That's the only reason the code in that comment
> behaves differently now, we were only partially broken before.

So, clarify this for me.  Before bug 712714 we had VALUE_TO_OBJECT in JSOP_GETPROP.  Now we don't.  How's that not a recent regression as concerns JSOP_GETPROP?

I don't believe it's a problem for JSOP_CALLPROP to behave the way it did.  The key is that JSOP_CALLPROP just gets the value of the property, but it doesn't affect the |this| value passed to the actual call at all, due to JSOP_CALL being separate from JSOP_CALLPROP.  What am I missing?

(Well, I think there might have been one mini-problem, if the property being accessed would exist on the boxed object and override the one on the prototype.  Only String boxed objects have properties, specifically the "length" and indexed properties.  But the indexed properties don't implicate JSOP_GETPROP.  So "foo".length() might act as though it called a property on String.prototype, so we'd call 0 rather than 3.  But that's a potential edge-case inaccuracy in error messages, not a semantic wrong, and it'd be pretty much cosmetic.)

> Two other problems:
> 
> - This only fixes the interpreter, not the JIT.

I tested the simple do-it-in-a-loop thing, and with -m -n -a and such, and didn't see problems.  When I look at the JIT, it looks to me like JSOP_GETPROP will hit this code in jsop_getprop if the value is never an object:

    /* If the incoming type will never PIC, take slow path. */
    if (top->isNotType(JSVAL_TYPE_OBJECT)) {
        jsop_getprop_slow(name, forPrototype);
        return true;
    }

...or this code if the value isn't an object at runtime:

    /* Guard that the type is an object. */
    Label typeCheck;
    if (doTypeCheck && !top->isTypeKnown()) {

And those revert back to GetPropertyOperation if I'm not mistaken.  So it seems to me that this does fix the JIT.  What am I missing?

> - This will destroy performance when accessing prototype properties through
> primitives, e.g. 'foo'.charAt.

This is simply reversion to the status quo ante, which doesn't seem inappropriate for immediately fixing a regression.  In the long run a bunch more needs to happen here, like bug 603201 wants.  But in the short run I don't see why this is unacceptable.

> It looks to me like the boxing should happen when the scripted getter is
> invoked.

Well, I think really the boxing shouldn't happen at all, unless the scripted getter wants it.  But in the space of changes which we can make quickly, and more importantly in aurora, I don't think we have that option.  What do you suggest as an alternative and more preferable narrowly-scoped fix?

Comment 3

[Brian Hackett [Laid off!]]

Object.defineProperty(Number.prototype, "f", {get: function() { print(this === Number.prototype); }});
x = 3;
x.f;
x.f();

> ancient-pre-type-inference-js test.js
false
true
test.js:5: TypeError: x.f is not a function

> tip-js test.js
true
true
test.js:5: TypeError: x.f is not a function

> d8 test.js
false
false
test.js:5: TypeError: Property 'f' of object 3 is not a function

This is the problem with JSOP_CALLPROP pre-712714.  712714 just made the two behave the same.

Comment 4

[Jeff Walden [:Waldo]]

Hmm, yes, I was misreading that JSOP_CALLPROP does set the this value.  So there was inconsistency here.

The fact remains, however, that correctness in the case where we were correct trumps consistency overall at cost of correctness.  And given this has migrated to aurora, we don't now have the luxury to make large changes.  So we should revert to our previous behavior, which is correct in one case even if it's inconsistent overall, and is strictly more correct than the current behavior, until we can make a full fix.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 602578 [details] [diff] [review]
Patch and test

OK, if this only affects scripted getter behavior then the JIT shouldn't need to be modified and perf should be ok.  The JIT *will* PIC property accesses on primitive strings, and won't box those strings when taking the PIC, but will stub any access on a scripted getter and go through GetPropertyOperation.

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4467b8a5721f

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 602578 [details] [diff] [review]
Patch and test

[Approval Request Comment]
Regression caused by (bug #): bug 712714
User impact if declined: getter properties, defined by functions in JS, won't work correctly if used with primitive values (strings, numbers, booleans).
Testing completed (on m-c, etc.): Patch includes tests.
Risk to taking this patch (and alternatives if risky): Low risk: the fix is straightforward as it just uses a different method than what's being used now.
String changes made by this patch: None.

Comment 8

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/4467b8a5721f

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 602578 [details] [diff] [review]
Patch and test

[Triage Comment]
Regression from a bug first fixed in FF12. Since the fix is low-risk, we'll take the patch instead of a backout.

Comment 10

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/823901e07ecd

Next time I should also remember to mention that taking the backout, given its size, would be substantially riskier than this regression spot-fix!  But the right decision was reached even without that information, so no harm no foul here.  :-)

Comment 11

[Domenic Denicola]

FYI IE9 and IE10 actually have a similar bug:

https://connect.microsoft.com/IE/feedback/details/737006