Bug 173867 - RegExp.prototype[@@replace] relies on globals and doesn't perform ToLength
Summary: RegExp.prototype[@@replace] relies on globals and doesn't perform ToLength
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Minor
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-06-26 23:38 PDT by Saam Barati
Modified: 2020-03-25 19:25 PDT (History)
16 users (show)

See Also:


Attachments
Patch (20.93 KB, patch)
2020-03-25 13:07 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (20.96 KB, patch)
2020-03-25 13:21 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (20.96 KB, patch)
2020-03-25 13:38 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2020-03-25 16:28 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2017-06-26 23:38:25 PDT
...
Comment 1 Saam Barati 2017-06-27 14:23:15 PDT
I think this is observable.
Comment 2 Alexey Shvayka 2020-03-25 13:07:39 PDT
Created attachment 394532 [details]
Patch
Comment 3 Alexey Shvayka 2020-03-25 13:21:53 PDT
Created attachment 394535 [details]
Patch

Add a few UNUSED_PARAM, drop extra JSC_HOST_CALL. I wonder if we need to ALWAYS_INLINE string*Impl()?
Comment 4 Ross Kirsling 2020-03-25 13:27:45 PDT
Comment on attachment 394535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394535&action=review

> Source/JavaScriptCore/ChangeLog:14
> +        b) Removes `lastPosition` checks/updates, as there are none in the spec, and it was
> +           equivalent to checking `nextSourcePosition`.

Nice catch. I wondered about this in my earlier patch but evidently didn't think about it long enough to realize it wasn't helping at all.
Comment 5 Alexey Shvayka 2020-03-25 13:38:08 PDT
Created attachment 394541 [details]
Patch

Set correct 'length' of builtinStringSubstringInternal's JSFunction.
Comment 6 Ross Kirsling 2020-03-25 13:54:45 PDT
Comment on attachment 394541 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394541&action=review

> Source/JavaScriptCore/runtime/StringPrototype.cpp:1136
> +    JSValue position = callFrame->argument(1);
> +    UNUSED_PARAM(position);
> +    ASSERT(position.isUndefined() || position.isNumber());

Here and below, if this is just for a non-release ASSERT, I suppose it'd be okay get the argument twice instead?
Comment 7 Alexey Shvayka 2020-03-25 16:28:54 PDT
Created attachment 394561 [details]
Patch

Drop UNUSED_PARAM.
Comment 8 EWS 2020-03-25 19:24:34 PDT
Committed r259029: <https://trac.webkit.org/changeset/259029>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394561 [details].
Comment 9 Radar WebKit Bug Importer 2020-03-25 19:25:12 PDT
<rdar://problem/60900492>