While it's not incorrect to initialize the butterfly storage when the indexingType is Undecided, it is inefficient as we'll end up initializing it again later when we convert the storage to a different indexingType. Some of our code already skips initializing Undecided butterflies. This patch makes it the consistent behavior everywhere.
Created attachment 315464 [details] proposed patch.
r=me
Comment on attachment 315464 [details] proposed patch. Attachment 315464 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4122591 New failing tests: imported/w3c/IndexedDB-private-browsing/idbfactory_open12.html http/tests/canvas/canvas-tainted-after-draw-image.html
Created attachment 315517 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5
Created attachment 315816 [details] patch for landing. Thanks for the review.
Comment on attachment 315816 [details] patch for landing. Clearing flags on attachment: 315816 Committed r219636: <http://trac.webkit.org/changeset/219636>
All reviewed patches have been landed. Closing bug.
I have to rollout this patch due to a bug, so I reopen the bug to remember to recommit a fixed version later on.
Created attachment 333655 [details] Patch
(In reply to Robin Morisset from comment #9) > Created attachment 333655 [details] > Patch This patch fixes the immediate problem, but I am not entirely happy with it. I see two problems: - It is a bit repetitive, with the two cases (ArrayWithDouble, and !ArrayWithUndecided) only differing by a few characters. I have not managed to simplify it because of typing issues. - It only fixes the fast path, the slow path (a dozen lines above, using moveElements) appears to have exactly the same problem. Sadly, it is not as easily testable, because it uses constructEmptyArray instead of tryCreateUninitializedRestricted, and as a result is not monitored by ObjectInitializationScope. Should I try fixing the slow path even though it currently already pass the test, should I force it to use ObjectInitializationScope somehow, or should I just ignore it?
Created attachment 333811 [details] New patch, includes some refactoring, waiting for EWS to test it before asking for review
Created attachment 333812 [details] New patch, includes some refactoring, waiting for EWS to test it before asking for review (fixed test)
Comment on attachment 333812 [details] New patch, includes some refactoring, waiting for EWS to test it before asking for review (fixed test) Attachment 333812 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6502895 New failing tests: ChakraCore.yaml/ChakraCore/test/Array/protoLookup.js.default
Created attachment 333826 [details] New patch, includes some refactoring
Comment on attachment 333826 [details] New patch, includes some refactoring Attachment 333826 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/6506695 New failing tests: stress/ftl-put-by-id-setter-exception-interesting-live-state.js.dfg-eager-no-cjit-validate
Comment on attachment 333826 [details] New patch, includes some refactoring I cannot reproduce this test failing. I suspect it might be flaky, especially considering this test does not seem to be calling concat. I would like a review while I check in more depth what could have made the bot fail.
I have confirmed that neither JSArray::appendMemcpy nor arrayProtoPrivateFuncConcatMemcpy are called by this test.
Created attachment 333923 [details] Patch
Comment on attachment 333923 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=333923&action=review > Source/JavaScriptCore/ChangeLog:19 > + That patch had revealed an underlying bug: arrayProtoPrivateFunConcatMemcpy forgot to initialize > + part of its result when given an array with undecided and an array with doubles. > + As a result it was rolled-out in r227435 > + Is there anywhere else that might depend on this behavior? > Source/JavaScriptCore/runtime/ArrayConventions.h:154 > + else if (type == ArrayWithInt32 || type == ArrayWithContiguous) What if these aren't arrays? Or are we guaranteed type here has the IsArray bit set? If so, it's worth an assert. e.g, do we see Int32Shape/ContigousShape here? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1206 > + } else if (UNLIKELY(source->indexingType() == ArrayWithUndecided)) { Do we not care about undecided shape? Or only ArrayWithUndecided? > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1213 > + JSValue value = source->tryGetIndexQuickly(i); This changes the code to no longer test if the result is empty. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1221 > +static bool moveElements(ExecState* exec, VM& vm, JSArray* target, unsigned targetOffset, JSArray* source) Maybe we should make this not return a boolean. Either it throws or it succeeds. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1267 > + if (UNLIKELY(!success)) > + return encodedJSValue(); It would be wrong to just return JSValue() to JS code. A javascript call should never return JSValue(). However, this code is not wrong, since moveElements *only* returns false if an exception happens. Let's just turn this into an assert then. I'm not sure why we had this check before given the behavior. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1320 > + if (type == NonArray) { I don't get how this is correct. mergeIndexingTypeForCopying returns NonArray for ArrayStorage. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1340 > + if (UNLIKELY(!success)) > + return encodedJSValue(); ditto here. This branch is never taken. > Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1344 > + if (UNLIKELY(!success)) > return encodedJSValue(); ditto here, and you don't reassign to success, so it would've done nothing regardless.