Bug 197228 - TypedArrays should not store properties that are canonical numeric indices
Summary: TypedArrays should not store properties that are canonical numeric indices
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tadeu Zagallo
URL:
Keywords: InRadar
: 197353 (view as bug list)
Depends on: 197446
Blocks:
  Show dependency treegraph
 
Reported: 2019-04-24 02:44 PDT by Tadeu Zagallo
Modified: 2019-05-07 17:40 PDT (History)
11 users (show)

See Also:


Attachments
Patch (7.55 KB, patch)
2019-04-24 02:48 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (12.46 KB, patch)
2019-04-24 08:33 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (9.95 KB, patch)
2019-04-24 10:44 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (3.06 MB, application/zip)
2019-04-24 11:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.71 MB, application/zip)
2019-04-24 12:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (3.05 MB, application/zip)
2019-04-24 12:27 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews121 for ios-simulator-wk2 (3.42 MB, application/zip)
2019-04-24 12:37 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.99 MB, application/zip)
2019-04-24 12:52 PDT, Build Bot
no flags Details
Patch (12.71 KB, patch)
2019-04-25 10:27 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (12.36 KB, patch)
2019-04-25 13:42 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (12.62 KB, patch)
2019-04-27 10:59 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (14.93 KB, patch)
2019-04-29 00:30 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch for landing (14.93 KB, patch)
2019-04-30 14:34 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (29.14 KB, patch)
2019-05-01 11:39 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-highsierra (3.16 MB, application/zip)
2019-05-01 13:28 PDT, Build Bot
no flags Details
Patch (29.42 KB, patch)
2019-05-03 08:40 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.48 MB, application/zip)
2019-05-03 11:52 PDT, Build Bot
no flags Details
Patch for landing (32.28 KB, patch)
2019-05-03 13:35 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Patch (33.05 KB, patch)
2019-05-03 15:09 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews212 for win-future (13.45 MB, application/zip)
2019-05-04 00:51 PDT, Build Bot
no flags Details
Patch for landing (33.02 KB, patch)
2019-05-04 11:33 PDT, Tadeu Zagallo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tadeu Zagallo 2019-04-24 02:44:48 PDT
<rdar://problem/49557381>
Comment 1 Tadeu Zagallo 2019-04-24 02:48:49 PDT
Created attachment 368115 [details]
Patch
Comment 2 Tadeu Zagallo 2019-04-24 08:33:29 PDT
Created attachment 368126 [details]
Patch

Fix build
Comment 3 Tadeu Zagallo 2019-04-24 10:44:24 PDT
Created attachment 368140 [details]
Patch

Rebase
Comment 4 Build Bot 2019-04-24 11:57:29 PDT
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
Comment 5 Build Bot 2019-04-24 11:57:31 PDT
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 6 Build Bot 2019-04-24 12:09:41 PDT
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
Comment 7 Build Bot 2019-04-24 12:09:43 PDT
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 8 Build Bot 2019-04-24 12:27:10 PDT
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
Comment 9 Build Bot 2019-04-24 12:27:12 PDT
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 10 Build Bot 2019-04-24 12:37:33 PDT
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
Comment 11 Build Bot 2019-04-24 12:37:35 PDT
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 12 Build Bot 2019-04-24 12:52:21 PDT
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
Comment 13 Build Bot 2019-04-24 12:52:33 PDT
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 14 Darin Adler 2019-04-24 14:04:23 PDT
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.
Comment 15 Tadeu Zagallo 2019-04-25 10:27:37 PDT
Created attachment 368245 [details]
Patch
Comment 16 Saam Barati 2019-04-25 11:00:08 PDT
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 17 Tadeu Zagallo 2019-04-25 13:31:27 PDT
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.
Comment 18 Tadeu Zagallo 2019-04-25 13:42:47 PDT
Created attachment 368262 [details]
Patch
Comment 19 Darin Adler 2019-04-26 15:03:19 PDT
(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?
Comment 20 Tadeu Zagallo 2019-04-27 10:59:29 PDT
Created attachment 368404 [details]
Patch
Comment 21 Tadeu Zagallo 2019-04-27 11:03:16 PDT
(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 22 Darin Adler 2019-04-27 14:32:30 PDT
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.
Comment 23 Tadeu Zagallo 2019-04-29 00:30:57 PDT
Created attachment 368444 [details]
Patch
Comment 24 Tadeu Zagallo 2019-04-29 00:55:53 PDT
(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 25 WebKit Commit Bot 2019-04-30 14:19:30 PDT
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
Comment 26 Tadeu Zagallo 2019-04-30 14:34:32 PDT
Created attachment 368615 [details]
Patch for landing
Comment 27 WebKit Commit Bot 2019-04-30 15:24:27 PDT
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 28 WebKit Commit Bot 2019-04-30 15:25:15 PDT
Comment on attachment 368615 [details]
Patch for landing

Clearing flags on attachment: 368615

Committed r244806: <https://trac.webkit.org/changeset/244806>
Comment 29 WebKit Commit Bot 2019-04-30 15:25:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 WebKit Commit Bot 2019-04-30 17:28:48 PDT
Re-opened since this is blocked by bug 197446
Comment 32 Tadeu Zagallo 2019-05-01 05:34:05 PDT
Hum, I did not handle symbols correctly. I will upload a new patch soon.
Comment 33 Tadeu Zagallo 2019-05-01 11:39:04 PDT
Created attachment 368687 [details]
Patch
Comment 34 Build Bot 2019-05-01 13:28:12 PDT
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
Comment 35 Build Bot 2019-05-01 13:28:14 PDT
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 36 Saam Barati 2019-05-02 15:01:21 PDT
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 37 Saam Barati 2019-05-02 15:03:48 PDT
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 38 Tadeu Zagallo 2019-05-03 07:32:42 PDT
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.
Comment 39 Tadeu Zagallo 2019-05-03 08:40:20 PDT
Created attachment 368929 [details]
Patch
Comment 40 Saam Barati 2019-05-03 10:40:48 PDT
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 41 Build Bot 2019-05-03 11:52:35 PDT
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
Comment 42 Build Bot 2019-05-03 11:52:42 PDT
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
Comment 43 Tadeu Zagallo 2019-05-03 13:35:31 PDT
Created attachment 368977 [details]
Patch for landing
Comment 44 Tadeu Zagallo 2019-05-03 15:09:14 PDT
Created attachment 368999 [details]
Patch
Comment 45 Darin Adler 2019-05-03 19:09:08 PDT
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 46 Build Bot 2019-05-04 00:51:28 PDT
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
Comment 47 Build Bot 2019-05-04 00:51:31 PDT
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
Comment 48 Tadeu Zagallo 2019-05-04 11:33:48 PDT
Created attachment 369072 [details]
Patch for landing
Comment 49 Tadeu Zagallo 2019-05-04 11:52:08 PDT
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 50 WebKit Commit Bot 2019-05-04 12:12:37 PDT
Comment on attachment 369072 [details]
Patch for landing

Clearing flags on attachment: 369072

Committed r244950: <https://trac.webkit.org/changeset/244950>
Comment 51 WebKit Commit Bot 2019-05-04 12:12:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 52 Saam Barati 2019-05-07 17:40:12 PDT
*** Bug 197353 has been marked as a duplicate of this bug. ***