Bug 151354 – [JSC] Should not emit get_by_id for indexed property access

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Brian Kuhn]

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.

[Brian Kuhn]

Created attachment 265684 [details]
Screenshot of test run on Safari 9

[Brian Kuhn]

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?

[Yusuke Suzuki]

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).

[Yusuke Suzuki]

Created attachment 267268 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Darin Adler]

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.

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

Committed r194021: <http://trac.webkit.org/changeset/194021>