730085 - Object.prototype.hasOwnProperty calls ToString on first argument before calling ToObject on this

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0.2) Gecko/20100101 Firefox/10.0.2
Build ID: 20120215223356

Steps to reproduce:

Execute the following code in nightly Spidermonkey shell:
---
Object.prototype.hasOwnProperty.call(null, {toString: function(){ throw "ToString called" }})
---


Actual results:

js> Object.prototype.hasOwnProperty.call(null, {toString: function(){ throw "ToString called" }})
typein:1: TypeError: can't convert null to object


Expected results:

js> Object.prototype.hasOwnProperty.call(null, {toString: function(){ throw "ToString called" }})
uncaught exception: "ToString called"

See ECMAScript5.1, 15.2.4.5  Object.prototype.hasOwnProperty (V):
1.  Let P be ToString(V). 
2.  Let O be the result of calling ToObject passing the this value as the argument. 
[...]

So ToString() must be called before ToObject(), but this isn't the case in Spidermonkey otherwise an exception should have been reported.

Comment 1

[Jeff Walden [:Waldo]]

I could have sworn we wrote code specifically to implement this oddity correctly, but it seems not.  A fairly easy bug for anyone interested in getting into SpiderMonkey...

Comment 2

[phillip]

Attachment: Patch to fix the bug

The output is now as expected:

linkenhe@c166177:build_DBG.OBJ$ ./js
js> Object.prototype.hasOwnProperty.call(null, {toString: function(){ throw "ToString called" }})
uncaught exception: ToString called
js> 

Results of the test runs on my machine:

===================================================
===================================================
Tests after pull (that is:
Revision: 91915
Changeset: a9b543de6b67b00bb67d41981451bd5e9f04b4b6 [a9b543de6b67]
Author: Tim Taubert <[e-mail]>
Date: 19. April 2012 10:48:59 MESZ

merge m-c to fx-team; a=desktop-only
):
===================================================


linkenhe@c166177:src$ jit-test/jit_test.py build_DBG.OBJ/js
[4236|   2|   0|4238]    100% =======================================>|  285.1s
FAILURES:
    -m /Users/linkenhe/hg/spidermonkey/js/src/jit-test/tests/sunspider/check-date-format-tofte.js
    -m -n /Users/linkenhe/hg/spidermonkey/js/src/jit-test/tests/sunspider/check-date-format-tofte.js
TIMEOUTS:

linkenhe@c166177:src$ python2.7 tests/jstests.py build_DBG.OBJ/js --args="-m -n"
[3161|   6| 211] 100% ===============================================>|  578.8s8s
REGRESSIONS
    ecma/Date/15.9.5.35-1.js
    ecma_3/Date/15.9.5.5.js
    ecma_3/Date/15.9.5.6.js
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/extensions/toLocaleFormat-02.js
    js1_5/Regress/regress-58116.js
FAIL

===================================================
===================================================
Tests with patch applied:
===================================================

linkenhe@c166177:src$ jit-test/jit_test.py build_DBG.OBJ/js
[4236|   2|   0|4238]    100% =======================================>|  282.2s
FAILURES:
    -m /Users/linkenhe/hg/spidermonkey/js/src/jit-test/tests/sunspider/check-date-format-tofte.js
    -m -n /Users/linkenhe/hg/spidermonkey/js/src/jit-test/tests/sunspider/check-date-format-tofte.js
TIMEOUTS:

linkenhe@c166177:src$ python2.7 tests/jstests.py build_DBG.OBJ/js --args="-m -n"
[3161|   6| 211] 100% ===============================================>|  571.7s7s
REGRESSIONS
    ecma/Date/15.9.5.35-1.js
    ecma_3/Date/15.9.5.5.js
    ecma_3/Date/15.9.5.6.js
    js1_5/extensions/toLocaleFormat-01.js
    js1_5/extensions/toLocaleFormat-02.js
    js1_5/Regress/regress-58116.js
FAIL

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 616548 [details] [diff] [review]
Patch to fix the bug

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

Sorry for the delay in getting to this, I've been kind of buried lately in other work and haven't been attending to reviews as promptly as I'd like.  :-\  Please do keep requesting review from me, tho, I'll be making a special effort to keep up with bugs I'm mentoring (and if that changes, I'll make sure to redirect appropriately).

::: js/src/jsobj.cpp
@@ +1264,5 @@
>  }
>  
> +static JSBool
> +obj_hasOwnProperty(JSContext *cx, unsigned argc, Value *vp)
> +{

This predated you, but lately we've been switching from using argc and vp directly to using CallArgs, like so:

  CallArgs args = CallArgsFromVp(argc, vp);
  // From here on use args.length() for argument length testing,
  // args[n] for the nth argument, args.thisv() for accessing the
  // passed-in |this| value, &args.rval() for accessing the
  // location in which to store the return value.

This form's a bit more readable than the current one, and in the long run we're going to do away with vp/argc entirely and just pass around CallArgs directly.

While you're touching this, mind changing obj_hasOwnProperty to use CallArgs?  For an example of how this works, see fun_bind in jsfun.cpp.

Also on the topic of style: we've taken to commenting the implementation to note each step taken, when it's taken, like /* Step 1. */, /* Steps 2-4. */, and so on.  (Sometimes the steps don't match up exactly to lines of code, but what correspondence there is, we find it helpful to note, even if it's out-of-order for efficiency or other reasons.)  And we prefix each method that corresponds to something in the standard with /* ES5 15.2.4.5. */ and such.  Mind applying that style while you're making changes here?  fun_bind is again a good example of how this works.

@@ +1270,5 @@
> +    // object (see line 2690) (I hope I'm right with that assumtation)
> +    
> +    // Fix: we have to convert the property name into a string first (bug 730085)
> +    
> +    // this code should trigger the toString call (I guess).

Yeah, we have a slightly different spelling of how we compute the names of properties -- this is the correct way to do it.

@@ +1290,5 @@
> +}
> +
> +JSBool
> +js_HasOwnPropertyHelper(JSContext *cx, LookupGenericOp lookup, unsigned argc,
> +                        Value *vp)

As it happens, js_HasOwnPropertyHelper really only has two callers: this one, and one in jsxml.cpp.  It looks to me like both cases can be changed so that inputs to the method that actually does the has-own-property computation (as opposed to exposing it to script calling some method) are the object, the LookupGenericOp to use to perform the lookup, and the jsid for the property.

Here's what I think we should do, then.  We should remove the ValueToId bits and the ToObject bits *both* from js_HasOwnPropertyHelper.  js_HasOwnPropertyHelper's signature should be changed to take (cx, obj, lookup, id, vp).  Then both callers can pick the order of operation they want -- this one to do ValueToId first and ToObject second, the XML one the other order.

Comment 4

[phillip]

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #3)
> Comment on attachment 616548 [details] [diff] [review]
> Patch to fix the bug
> 
> Review of attachment 616548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay in getting to this, I've been kind of buried lately in
> other work and haven't been attending to reviews as promptly as I'd like. 
> :-\  Please do keep requesting review from me, tho, I'll be making a special
> effort to keep up with bugs I'm mentoring (and if that changes, I'll make
> sure to redirect appropriately).
Thank you very much for the feedback. I hope I find time in the next day to modify my path according o your suggestions.

Comment 5

[Saurabh Anand [:sawrubh]]

Attachment: Patch v1

Comment 6

[Tom S [:evilpie]]

Comment on attachment 652857 [details] [diff] [review]
Patch v1

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

Thank you for picking this up. I hope you are going to find something else to work on soon :)
This patch looks good to me. But you still need some test that would have have failed previously.

Something like:

>var calledToString = true;
>assertThrowsInstanceOf(function () { Object.prototype.hasOwnProperty.call(null, {toString: function() { calledToString = true; }}); }, TypeError);
>assertEq(calledToString, false);

::: js/src/jsobj.cpp
@@ +750,1 @@
>  {

Remove the "Proposed" above this line.

@@ +762,5 @@
> +    return js_HasOwnPropertyHelper(cx, obj->getOps()->lookupGeneric, args.rval(), obj, id);
> +}
> +
> +inline JSBool
> +js_HasOwnPropertyHelper(JSContext *cx, LookupGenericOp lookup, MutableHandleValue rval,

Personally I would make rval the last parameter.

@@ +765,5 @@
> +inline JSBool
> +js_HasOwnPropertyHelper(JSContext *cx, LookupGenericOp lookup, MutableHandleValue rval,
> +                        HandleObject obj, HandleId id)
> +{
> +    /* Step 3. */

Not the right place.

@@ +770,3 @@
>      RootedObject obj2(cx);
>      RootedShape prop(cx);
>      if (obj->isProxy()) {

Maybe "Non-standard code for proxies" or something like that?

@@ +777,4 @@
>          return true;
>      }
> +
> +    /* Step 4. */

As always we do things a little bit different, but this is more or less Step 3.

@@ +781,3 @@
>      if (!js_HasOwnProperty(cx, lookup, obj, id, &obj2, &prop))
> +        return false;
> +    rval.setBoolean(!!prop);

This is actually /* Step 4., 5. */

@@ +781,5 @@
>      if (!js_HasOwnProperty(cx, lookup, obj, id, &obj2, &prop))
> +        return false;
> +    rval.setBoolean(!!prop);
> +
> +    /* Step 5. */

"return true" just means the function did not fail (in the sense of throwing some exception). You can remove /* Step 5. */ here.

::: js/src/jsxml.cpp
@@ +5885,3 @@
>  static JSBool
>  xml_hasOwnProperty(JSContext *cx, unsigned argc, jsval *vp)
>  {

Thanks for bringing this code into 2012. I still hope it's going to be gone soon.

Comment 7

[Saurabh Anand [:sawrubh]]

Attachment: Patch v2

Comment 8

[Saurabh Anand [:sawrubh]]

Attachment: Test v1

Comment 9

[Saurabh Anand [:sawrubh]]

https://tbpl.mozilla.org/?tree=Try&rev=865c2bd92d30

Comment 10

[Saurabh Anand [:sawrubh]]

Attachment: Patch v3

Removed the "inline" from the js_HasOwnPropertyHelper function and now the try run looks green on all the platforms.

Comment 11

[Saurabh Anand [:sawrubh]]

The try run with the patch in comment 10 is :

https://tbpl.mozilla.org/?tree=Try&rev=aac95965eb86

Comment 12

[Tom S [:evilpie]]

Comment on attachment 653048 [details] [diff] [review]
Patch v3

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

Thanks for taking this!

Comment 13

[Tom S [:evilpie]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/f1c9b2f9e8bd

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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