Bug 151354 - [JSC] Should not emit get_by_id for indexed property access
Summary: [JSC] Should not emit get_by_id for indexed property access
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Safari 9
Hardware: Macintosh OS X 10.11
: P1 Critical
Assignee: Yusuke Suzuki
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-11-17 11:28 PST by Brian Kuhn
Modified: 2015-12-13 18:53 PST (History)
11 users (show)

See Also:


Attachments
Screenshot of test run on Safari 9 (606.81 KB, image/png)
2015-11-17 11:29 PST, Brian Kuhn
no flags Details
Patch (8.42 KB, patch)
2015-12-13 12:20 PST, Yusuke Suzuki
darin: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (1000.59 KB, application/zip)
2015-12-13 13:17 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Kuhn 2015-11-17 11:28:33 PST
Calling the following function many times can confuse Safari into caching the result (probably actually caching the compiled/optimized code for the function).

    function getOne(a) { return a['1']; }

This specific function is not the problem; it just represents the simplest case.  See the following live example:

    http://jsfiddle.net/60ygm4uk/15/

Steps to Reproduce:

    1) Call getOne({2: true}); many times (at least 36 times).
    2) Call getOne({1: true});

Expected Results:

    getOne({1: true}) should return true.

Actual Results:

    getOne({1: true}) returns undefined.

Severity:

    Causes bugs in major JavaScript libraries (i.e. Google Analytics).  Millions of sites potentially affected.
Comment 1 Brian Kuhn 2015-11-17 11:29:13 PST
Created attachment 265684 [details]
Screenshot of test run on Safari 9
Comment 2 Brian Kuhn 2015-12-02 10:44:41 PST
Ping?  

This is a pretty fundamental bug, and I've provided a clear, minimal testcase which is always reproducible.  

Can someone please take a look?
Comment 3 Yusuke Suzuki 2015-12-13 11:16:45 PST
Investigating, but I think I found the cause of this.

Seeing the result, while "1" should be handled as get_by_val, current bytecode compiler emits get_by_id. So, "1" is accidentally handled by Inline Caching.
But it's not correct. Elements are not handled by Structure's identity check. When adding new element property, there may be no Structure transition.

The situation is the following,

Call getOne({2: true});
And get_by_id "1" emits IC for this object's map.

And later we getOne({1: true}).
{1: true}'s Structure is the same to {2: true} since element property addition does not occur Structure transition. So IC accidentally handles this case.

Tomorrow, I'll check further and submit a patch (since now, 4:13 JST).
Comment 4 Yusuke Suzuki 2015-12-13 12:20:48 PST
Created attachment 267268 [details]
Patch
Comment 5 Build Bot 2015-12-13 13:17:02 PST
Comment on attachment 267268 [details]
Patch

Attachment 267268 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/555151

New failing tests:
js/parser-syntax-check.html
js/slow-stress/destructuring-arguments-length.html
js/destructuring-assignment.html
Comment 6 Build Bot 2015-12-13 13:17:05 PST
Created attachment 267269 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 7 Darin Adler 2015-12-13 13:25:54 PST
Comment on attachment 267268 [details]
Patch

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

looks like assertions are firing in at least three run-webkit-tests tests; please run those tests locally and fix before landing

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:605
> +static bool isIdStringElement(ExpressionNode* element)

Would be better to take ExpressionNode&.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:614
> +        if (isIdStringElement(m_subscript)) {

I am not sure that "id" is good terminology for property indentifiers that are not indices. I would name this isNonIndexStringElement.
Comment 8 Yusuke Suzuki 2015-12-13 18:41:34 PST
Comment on attachment 267268 [details]
Patch

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

Thank you for your review! I've fixed the assertions :)

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:605
>> +static bool isIdStringElement(ExpressionNode* element)
> 
> Would be better to take ExpressionNode&.

Nice! Fixed.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:614
>> +        if (isIdStringElement(m_subscript)) {
> 
> I am not sure that "id" is good terminology for property indentifiers that are not indices. I would name this isNonIndexStringElement.

Sounds nice, fixed.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:854
> +    bool subscriptIsIdString = isIdStringElement(m_subscript);

And also change it to subscriptIsNonIndexString.
Comment 9 Yusuke Suzuki 2015-12-13 18:53:18 PST
Committed r194021: <http://trac.webkit.org/changeset/194021>