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.
Created attachment 265684 [details] Screenshot of test run on Safari 9
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?
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).
Created attachment 267268 [details] Patch
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
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 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 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.
Committed r194021: <http://trac.webkit.org/changeset/194021>