Bug 197228

Created attachment 368115 [details] Patch

Created attachment 368126 [details] Patch Fix build

Created attachment 368140 [details] Patch Rebase

Comment on attachment 368140 [details] Patch Attachment 368140 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11985384 New failing tests: fast/canvas/canvas-ImageData-behaviour.html

Created attachment 368153 [details] Archive of layout-test-results from ews103 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 368140 [details] Patch Attachment 368140 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11985417 New failing tests: fast/canvas/canvas-ImageData-behaviour.html

Created attachment 368156 [details] Archive of layout-test-results from ews107 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 368140 [details] Patch Attachment 368140 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11985400 New failing tests: fast/canvas/canvas-ImageData-behaviour.html

Created attachment 368157 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 368140 [details] Patch Attachment 368140 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11985418 New failing tests: fast/canvas/canvas-ImageData-behaviour.html

Created attachment 368160 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6

Comment on attachment 368140 [details] Patch Attachment 368140 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11985713 New failing tests: fast/canvas/canvas-ImageData-behaviour.html

Created attachment 368163 [details] Archive of layout-test-results from ews202 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit

Comment on attachment 368140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368140&action=review > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:47 > + if (property != String::numberToStringECMAScript(index)) This seems is a rather expensive way to do this check. We could avoid a single memory allocation by using the underlying function that String::numberToStringECMAScript that puts things into a char buffer. But there may be other ways to do this significantly more efficiently. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:52 > +static inline bool isValidIndex(double index) If this is used only in assertions then: 1) No reason to mark it inline. 2) Needs to be inside an #if !ASSERT_DISABLED or something like that. Also seems this could be a constexpr function. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:54 > + if (fabs(index) == std::numeric_limits<double>::infinity()) Please use std::abs instead of fabs. Can we use std::isfinite here instead? > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:56 > + if (floor(fabs(index)) != fabs(index)) Please use std::floor instead of floor. Also, is the compiler going to optimize the three calls to std::abs or should we use a local variable? > JSTests/stress/typed-array-canonical-numeric-index-string.js:44 > + test('-0'); > + test('-1'); > + test(-1); > + test(1); > + test('Infinity'); > + test('-Infinity'); > + test('NaN'); > + test('0.1'); Should also cover the values right at the limits of array indices, last valid array index, one higher, etc.

Created attachment 368245 [details] Patch

Comment on attachment 368245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368245&action=review > Source/JavaScriptCore/ChangeLog:12 > + According to the spec, TypedArrays should not perform an ordinary GetOwnProperty/SetOwnProperty > + if the index is a CanonicalNumericIndexString, but invalid according toIntegerIndexedElementGet > + and similar functions. I.e., there are a few properties that should not be set in a TypedArray, > + like NaN, Infinity and -0. Can you link to the spec? It really distinguishes between zero and negative zero here? > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:-430 > - revert please. > Source/JavaScriptCore/runtime/JSTypedArrays.cpp:60 > +Optional<double> canonicalNumericIndexString(const PropertyName& propertyName) Can you link to the spec? > Source/JavaScriptCore/runtime/JSTypedArrays.cpp:63 > + if (equal(property, "-0")) really? What about other forms of "-0"? "-0.0", etc?

Comment on attachment 368245 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368245&action=review >> Source/JavaScriptCore/ChangeLog:12 >> + like NaN, Infinity and -0. > > Can you link to the spec? > > It really distinguishes between zero and negative zero here? Yes, it has a specific step in the spec for it. I'll update the changelog, but you can see it here: https://www.ecma-international.org/ecma-262/9.0/index.html#sec-canonicalnumericindexstring >> Source/JavaScriptCore/runtime/JSTypedArrays.cpp:63 >> + if (equal(property, "-0")) > > really? What about other forms of "-0"? "-0.0", etc? "-0.0" is not a canonical numeric index, because of the trailing decimal.

Created attachment 368262 [details] Patch

(In reply to Tadeu Zagallo from comment #17) > "-0.0" is not a canonical numeric index, because of the trailing decimal. Makes sense of course. I suggest we emphasize that point by including it as one of the test cases. Were we able to use our test with other JavaScript engines to confirm their implementations agree with what we are switching to?

Created attachment 368404 [details] Patch

(In reply to Darin Adler from comment #19) > (In reply to Tadeu Zagallo from comment #17) > > "-0.0" is not a canonical numeric index, because of the trailing decimal. > > Makes sense of course. I suggest we emphasize that point by including it as > one of the test cases. Done! > Were we able to use our test with other JavaScript engines to confirm their > implementations agree with what we are switching to? I had checked V8 and it matches the new behavior, but SpiderMonkey matches the old behavior.

Comment on attachment 368404 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368404&action=review I am still a bit confused about what the standard calls for. I would add these test cases too: '4294967294' // maximum array index '4294967295' // maximum array index + 1 '4294967296' // maximum array index + 2 I understand the desire to not confuse array indices with other property keys, but I don’t get how the people who wrote the standard chose to divide things. I would have thought they’d make it illegal to have other properties if the keys are valid array indices, but instead they defined a seemingly arbitrary superset of valid array indices. > Source/JavaScriptCore/runtime/JSTypedArrays.h:49 > +JS_EXPORT_PRIVATE Optional<double> canonicalNumericIndexString(const PropertyName&); > + > +#if !ASSERT_DISABLED > +JS_EXPORT_PRIVATE bool isValidIndex(double); > +#endif I think it’s OK to land it like this, but I think it would be cleaner to include only a function that returns a boolean: bool isCanonicalNumericIndeString(const PropertyName&); We can put the consistency checks and assertions inside the function. Also not entirely sure this function belongs in JSTypedArrays.h. I know that typed array implementations need to do this check, but the check is a more basic language property.

Created attachment 368444 [details] Patch

(In reply to Darin Adler from comment #22) > I am still a bit confused about what the standard calls for. I would add > these test cases too: > > '4294967294' // maximum array index > '4294967295' // maximum array index + 1 > '4294967296' // maximum array index + 2 > > I understand the desire to not confuse array indices with other property > keys, but I don’t get how the people who wrote the standard chose to divide > things. I would have thought they’d make it illegal to have other properties > if the keys are valid array indices, but instead they defined a seemingly > arbitrary superset of valid array indices. Thanks, I've included the test cases. I had misunderstood it before as testing beyond the array's length. Indeed, it did catch an issue, since it's canonical index, it's a valid index, but we shouldn't store it since it will always be out of bounds. After fixing this, we no longer need the `isValidIndex` function, since the assertion doesn't always hold. I also moved the `canonicalNumericIndexString` into PropertyName.h. I'm going to hold off on landing it, in case you have any comments about these changes.

Comment on attachment 368444 [details] Patch Rejecting attachment 368444 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 368444, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Darin Alder found in /Volumes/Data/EWS/WebKit/JSTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/12044506

Created attachment 368615 [details] Patch for landing

The commit-queue encountered the following flaky tests while processing attachment 368615 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.

Comment on attachment 368615 [details] Patch for landing Clearing flags on attachment: 368615 Committed r244806: <https://trac.webkit.org/changeset/244806>

All reviewed patches have been landed. Closing bug.

It looks like changes in https://trac.webkit.org/changeset/244806 Are causing 16 Test262 failures on Release and Debug https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20Test262%20%28Tests%29/builds/2775/steps/test262-test/logs/stdio https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20Test262%20%28Tests%29/builds/2099/steps/test262-test/logs/stdio Also appears to be causing 2 failures on Release JSC https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20JSC%20%28Tests%29/builds/9176/steps/jscore-test/logs/stdio

Re-opened since this is blocked by bug 197446

Hum, I did not handle symbols correctly. I will upload a new patch soon.

Created attachment 368687 [details] Patch

Comment on attachment 368687 [details] Patch Attachment 368687 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12052978 New failing tests: jquery/offset.html

Created attachment 368696 [details] Archive of layout-test-results from ews117 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6

Comment on attachment 368687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368687&action=review > Source/JavaScriptCore/ChangeLog:12 > + According to the spec[1], TypedArrays should not perform an ordinary GetOwnProperty/SetOwnProperty > + if the index is a CanonicalNumericIndexString, but invalid according toIntegerIndexedElementGet > + and similar functions. I.e., there are a few properties that should not be set in a TypedArray, > + like NaN, Infinity and -0. It seems like this patch is doing more than this. Can you add what else it's doing to this changelog? > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:418 > + if (index.value() >= thisObject->m_length) > + return false; This seems like a behavior change. > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:470 > + if (propertyName > MAX_ARRAY_INDEX) > + return false; why is this check needed? Wouldn't canGetIndexQuickly below fail? (I know you're just changing the code slightly, but it seems like it might be an unnecessary branch.) > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:-466 > - if (propertyName > MAX_ARRAY_INDEX) { > - PutPropertySlot slot(JSValue(thisObject), shouldThrow); > - return thisObject->methodTable(exec->vm())->put(thisObject, exec, Identifier::from(exec, propertyName), value, slot); > - } Why is this ok? This seems like a real behavior change as well. Do you have a test that the put fails? > Source/JavaScriptCore/runtime/PropertyName.h:140 > + if (!property) > + return WTF::nullopt; why would this happen?

Comment on attachment 368687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368687&action=review >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:418 >> + return false; > > This seems like a behavior change. I left an unfinished thought here. It would be good to highlight this in the changeling.

Comment on attachment 368687 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368687&action=review >> Source/JavaScriptCore/ChangeLog:12 >> + like NaN, Infinity and -0. > > It seems like this patch is doing more than this. Can you add what else it's doing to this changelog? I will update it. >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:470 >> + return false; > > why is this check needed? Wouldn't canGetIndexQuickly below fail? (I know you're just changing the code slightly, but it seems like it might be an unnecessary branch.) You're correct. I will remove it. >> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:-466 >> - } > > Why is this ok? This seems like a real behavior change as well. Do you have a test that the put fails? This is handled by `setIndex` after the `isNeutered` check, which is the correct order according to the spec. I do have tests for this. >> Source/JavaScriptCore/runtime/PropertyName.h:140 >> + return WTF::nullopt; > > why would this happen? To be honest, I'm not sure. It's how `parseIndex` handled it, so I thought I should handle this case here too.

Created attachment 368929 [details] Patch

Comment on attachment 368929 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368929&action=review r=me > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:370 > + slot.setValue(thisObject, PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly, jsUndefined()); > + return false; so Object.hasOwnProperty(typedArray, "-0") returns true? and Object.getOwnPropertyDescriptor(typedArray, "-0").configurable == false and writable == false? Do we have tests for this? (and other canonical numeric index strings)

Comment on attachment 368929 [details] Patch Attachment 368929 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12089599 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star.html

Created attachment 368959 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 368977 [details] Patch for landing

Created attachment 368999 [details] Patch

Comment on attachment 368999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368999&action=review > Source/JavaScriptCore/runtime/PropertyName.h:136 > +// https://www.ecma-international.org/ecma-262/9.0/index.html#sec-canonicalnumericindexstring > +ALWAYS_INLINE Optional<double> canonicalNumericIndexString(const PropertyName& propertyName) All the call sites are using this function simply to answer the boolean question, "is this a canonical numeric index string?" This function instead replicates the algorithm in the specification, which returns a numeric index, and the callers ignore it. It’s considerably more expensive to implement that operation. We should not need to actually do the jsToNumber and numberToString round trip just to answer the question of whether the string is a canonical numeric index, although it might be tricky to write an alternative bit of code correctly. For example, the canonical numeric strings have only a possible leading "-" and then the characters "0-9", and we’d need a few checks like making sure there are no leading zeroes and the number isn’t too large or too small (some kind of clever length check may do the trick), we would not have to actually convert to a double and then back. I suggest that: 1) we name this function isCanonicalNumericIndexString 2) over time we explore a much more efficient implementation that doesn’t involve the round trip conversion to double and back

Comment on attachment 368999 [details] Patch Attachment 368999 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12097576 New failing tests: security/contentSecurityPolicy/video-with-file-url-allowed-by-media-src-star-with-AllowContentSecurityPolicySourceStarToMatchAnyProtocol-enabled.html

Created attachment 369064 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

Created attachment 369072 [details] Patch for landing

Comment on attachment 368999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368999&action=review >> Source/JavaScriptCore/runtime/PropertyName.h:136 >> +ALWAYS_INLINE Optional<double> canonicalNumericIndexString(const PropertyName& propertyName) > > All the call sites are using this function simply to answer the boolean question, "is this a canonical numeric index string?" This function instead replicates the algorithm in the specification, which returns a numeric index, and the callers ignore it. It’s considerably more expensive to implement that operation. We should not need to actually do the jsToNumber and numberToString round trip just to answer the question of whether the string is a canonical numeric index, although it might be tricky to write an alternative bit of code correctly. For example, the canonical numeric strings have only a possible leading "-" and then the characters "0-9", and we’d need a few checks like making sure there are no leading zeroes and the number isn’t too large or too small (some kind of clever length check may do the trick), we would not have to actually convert to a double and then back. > > I suggest that: > > 1) we name this function isCanonicalNumericIndexString > 2) over time we explore a much more efficient implementation that doesn’t involve the round trip conversion to double and back Thanks, I've renamed the function. For 2, we'd also need to consider Infinity and NaN, but I agree it's doable. The reason I was ok with this code is that I think this is a pretty rare code path for typed arrays, and the clever implementation was much more bug-prone. A half way alternative could be checking if it's a valid double with jsToNumber and then doing simple checks on the strings (does it have a leading +, leading 0, trailing 0 or trailing dot).

Comment on attachment 369072 [details] Patch for landing Clearing flags on attachment: 369072 Committed r244950: <https://trac.webkit.org/changeset/244950>

All reviewed patches have been landed. Closing bug.

*** Bug 197353 has been marked as a duplicate of this bug. ***

*** Bug 188792 has been marked as a duplicate of this bug. ***

*** Bug 171718 has been marked as a duplicate of this bug. ***