Fix bugs in Object.prototype this handling.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Gavin Barraclough]

undefined/null this values should throw TypeErrors, not convert to the global object.

[Gavin Barraclough]

Created attachment 101129 [details]
Preliminary patch

[Gavin Barraclough]

Created attachment 101202 [details]
The patch

[WebKit Review Bot]

Comment on attachment 101202 [details]
The patch

Attachment 101202 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9112479

New failing tests:
fast/js/object-prototype-properties.html
fast/js/object-prototype-toLocaleString.html

[Gavin Barraclough]

Created attachment 101232 [details]
Added missing updated results, fixed bug in harness.

[Gavin Barraclough]

Created attachment 101238 [details]
Ooops, revert some debug code from last patch

[Gavin Barraclough]

Fixed in r91224

[Ryosuke Niwa]

Filed crbug.com/89673 to track Chromium test failures.

[Mads Ager]

Comment on attachment 101238 [details]
Ooops, revert some debug code from last patch

View in context: https://bugs.webkit.org/attachment.cgi?id=101238&action=review

A couple of drive-by comments.

> LayoutTests/fast/js/script-tests/object-prototype-properties.js:6
> +shouldThrow("toString()");

According the the ES5.1 spec this should not throw an exception and return "[object Undefined]":

Section 15.2.4.2:

1. If the this value is undefined, return "[object Undefined]".

> LayoutTests/fast/js/script-tests/object-prototype-properties.js:9
> +shouldThrow("hasProperty('hasProperty')");

I guess this should be hasOwnProperty?

[Gavin Barraclough]

(In reply to comment #8)

> > +shouldThrow("toString()");
> 
> According the the ES5.1 spec this should not throw an exception and return "[object Undefined]":

Good catch.  I missed this coming out of draft, time to update my copy of the spec. :-)

> > +shouldThrow("hasProperty('hasProperty')");
>
> I guess this should be hasOwnProperty?

Gah, yes it should.

Thanks for the comments, will fix!

[Gavin Barraclough]

Created attachment 101409 [details]
Fix ES5.1 correctness issues

[Darin Adler]

Comment on attachment 101409 [details]
Fix ES5.1 correctness issues

View in context: https://bugs.webkit.org/attachment.cgi?id=101409&action=review

> Source/JavaScriptCore/runtime/ObjectPrototype.cpp:199
> +    if (thisValue.isUndefinedOrNull())
> +        return jsNontrivialString(exec, thisValue.isUndefined() ? "[object Undefined]" : "[object Null]");
>      return JSValue::encode(jsMakeNontrivialString(exec, "[object ", thisValue.toObject(exec)->className(), "]"));

I worry a bit about the performance implications of allocating a string every time.

[Gavin Barraclough]

(In reply to comment #11)
> I worry a bit about the performance implications of allocating a string every time.

That's a fair concern, but I think we should probably stick with the simple solution for now & optimize if necessary.

We don't cache the results of Object.prototype.toString for other classes of object, which I imagine is something we could do.  Undefined/null are probably the least likely types to be converted (since behavior here is not going to be uniform across browsers right now), so any caching that is specific to undefined/null values is likely to be suboptimal.

[Gavin Barraclough]

ES5.1 corrections per Mads Ager's comments landed in r91344.