Bug 64678 - Fix bugs in Object.prototype this handling.
Summary: Fix bugs in Object.prototype this handling.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 64250
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-17 23:29 PDT by Gavin Barraclough
Modified: 2011-07-20 19:35 PDT (History)
3 users (show)

See Also:


Attachments
Preliminary patch (5.03 KB, patch)
2011-07-17 23:31 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
The patch (20.43 KB, patch)
2011-07-18 14:19 PDT, Gavin Barraclough
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Added missing updated results, fixed bug in harness. (22.69 KB, patch)
2011-07-18 16:38 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Ooops, revert some debug code from last patch (14.23 KB, patch)
2011-07-18 16:56 PDT, Gavin Barraclough
oliver: review+
Details | Formatted Diff | Diff
Fix ES5.1 correctness issues (15.36 KB, patch)
2011-07-19 16:59 PDT, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2011-07-17 23:29:02 PDT
undefined/null this values should throw TypeErrors, not convert to the global object.
Comment 1 Gavin Barraclough 2011-07-17 23:31:00 PDT
Created attachment 101129 [details]
Preliminary patch
Comment 2 Gavin Barraclough 2011-07-18 14:19:20 PDT
Created attachment 101202 [details]
The patch
Comment 3 WebKit Review Bot 2011-07-18 16:15:12 PDT
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
Comment 4 Gavin Barraclough 2011-07-18 16:38:08 PDT
Created attachment 101232 [details]
Added missing updated results, fixed bug in harness.
Comment 5 Gavin Barraclough 2011-07-18 16:56:19 PDT
Created attachment 101238 [details]
Ooops, revert some debug code from last patch
Comment 6 Gavin Barraclough 2011-07-18 17:26:56 PDT
Fixed in r91224
Comment 7 Ryosuke Niwa 2011-07-18 18:18:36 PDT
Filed crbug.com/89673 to track Chromium test failures.
Comment 8 Mads Ager 2011-07-19 02:44:34 PDT
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?
Comment 9 Gavin Barraclough 2011-07-19 12:26:47 PDT
(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!
Comment 10 Gavin Barraclough 2011-07-19 16:59:53 PDT
Created attachment 101409 [details]
Fix ES5.1 correctness issues
Comment 11 Darin Adler 2011-07-19 17:31:29 PDT
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.
Comment 12 Gavin Barraclough 2011-07-20 00:51:47 PDT
(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.
Comment 13 Gavin Barraclough 2011-07-20 00:52:37 PDT
ES5.1 corrections per Mads Ager's comments landed in r91344.