Bug 173867

Summary: RegExp.prototype[@@replace] relies on globals and doesn't perform ToLength
Product: WebKit Reporter: Saam Barati <saam>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Minor CC: ashvayka, benjamin, ews-watchlist, fpizlo, ggaren, gskachkov, jfbastien, joepeck, keith_miller, mark.lam, msaboff, ross.kirsling, ticaiolima, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=209323
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

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>