Bug 209375 - RegExp.prototype.exec must always access lastIndex
Summary: RegExp.prototype.exec must always access lastIndex
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ross Kirsling
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-20 22:55 PDT by Ross Kirsling
Modified: 2020-03-31 11:49 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.49 KB, patch)
2020-03-20 23:06 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2020-03-21 00:34 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (10.72 KB, patch)
2020-03-21 12:34 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (12.24 KB, patch)
2020-03-21 14:06 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (14.61 KB, patch)
2020-03-21 15:45 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2020-03-24 16:01 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff
Patch (15.16 KB, patch)
2020-03-30 14:48 PDT, Ross Kirsling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ross Kirsling 2020-03-20 22:55:11 PDT
RegExp.prototype.exec must always access lastIndex
Comment 1 Ross Kirsling 2020-03-20 23:06:52 PDT
Created attachment 394161 [details]
Patch
Comment 2 Ross Kirsling 2020-03-20 23:22:39 PDT
Comment on attachment 394161 [details]
Patch

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

A couple of notes:

1. I thought about proposing we change the spec here, but apparently somebody already tried that and it had to be reverted:
   https://github.com/tc39/ecma262/issues/777

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:803
> +            if (!hasIndeterminateLastIndex && convertToStatic())

2. I believe this does the trick, but please educate me if there's a better way.

(Near the top of this case block, we find the RegExpObject when the op is RegExpExec, so my original idea was to hang onto that so that I could check regExpObject->getLastIndex().isUInt32() in convertToStatic, but it seemed like regExpObject was disappearing on me. I suppose that explains the complexity of the existing lastIndex-verifying procedure. As such, I just added a boolean so that we don't do the verification twice -- i.e., we should only convert to RegExpExecNonGlobalOrSticky when lastIndex is determinate but the RegExp as a whole is non-constant.)
Comment 3 Ross Kirsling 2020-03-21 00:34:10 PDT
Created attachment 394162 [details]
Patch
Comment 4 Ross Kirsling 2020-03-21 12:34:50 PDT
Created attachment 394173 [details]
Patch
Comment 5 Ross Kirsling 2020-03-21 12:40:25 PDT
(In reply to Ross Kirsling from comment #2)
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:803
> > +            if (!hasIndeterminateLastIndex && convertToStatic())
> 
> 2. I believe this does the trick, but please educate me if there's a better
> way.

Looks like I can just raise the lastIndex verification step up and out of foldConstant instead.
Comment 6 Ross Kirsling 2020-03-21 14:06:47 PDT
Created attachment 394176 [details]
Patch
Comment 7 Ross Kirsling 2020-03-21 15:45:54 PDT
Created attachment 394180 [details]
Patch
Comment 8 Caio Lima 2020-03-24 14:52:16 PDT
Comment on attachment 394180 [details]
Patch

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

Informal review: Overall LGTM, but I left some comments.

> Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:567
> +                lastIndex = 0;

Nice catch here. Is there a specific reason to move this out of `foldToConstant`? This will make us always run `executeInsertionSet()` and iterate over instructions increasing compile time, even if we endup giving up on folding due to other reasons.

> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:49
>          RETURN_IF_EXCEPTION(scope, UINT_MAX);

Do we have a test case like the one you added, but it throws an exception instead of counting execution? I would be nice to test such code path as well.
Comment 9 Ross Kirsling 2020-03-24 15:08:39 PDT
(In reply to Caio Lima from comment #8)
> > Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:567
> > +                lastIndex = 0;
> 
> Nice catch here. Is there a specific reason to move this out of
> `foldToConstant`? This will make us always run `executeInsertionSet()` and
> iterate over instructions increasing compile time, even if we endup giving
> up on folding due to other reasons.

It needs block *both* strength reductions that follow. At first I just had foldToConstant set a boolean which would prevent entering convertToStatic, but beyond being weird in style, this wouldn't've been valid in the case where foldToConstant didn't even reach that check (e.g. due to massive string size or number of subpatterns).

My only concern was whether the ordering was too fragile to allow for raising this check above, but this doesn't appear to be the case.

> > Source/JavaScriptCore/runtime/RegExpObjectInlines.h:49
> >          RETURN_IF_EXCEPTION(scope, UINT_MAX);
> 
> Do we have a test case like the one you added, but it throws an exception
> instead of counting execution? I would be nice to test such code path as
> well.

I was going to add one where lastIndex is a Symbol and we catch the error, but I couldn't get it to tier up that way. I guess I can add it regardless though.
Comment 10 Caio Lima 2020-03-24 15:17:43 PDT
Comment on attachment 394180 [details]
Patch

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

>> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:49
>>          RETURN_IF_EXCEPTION(scope, UINT_MAX);
> 
> Do we have a test case like the one you added, but it throws an exception instead of counting execution? I would be nice to test such code path as well.

I think it is fine if we don't tier up, but if you would like to do it, I think this could potentially work:

```
...
let throws = false;
r.lastIndex = {
  valueOf() {
     if(throws) 
        //throws here
     return 0;
   }
 };
...
// set throws to true after we tier up
```
Comment 11 Ross Kirsling 2020-03-24 15:53:25 PDT
(In reply to Caio Lima from comment #10)
> I think it is fine if we don't tier up, but if you would like to do it, I
> think this could potentially work:
> 
> ```
> ...
> let throws = false;
> r.lastIndex = {
>   valueOf() {
>      if(throws) 
>         //throws here
>      return 0;
>    }
>  };
> ...
> // set throws to true after we tier up
> ```

That's a neat suggestion! I think it won't help though, if DFG compiles with the assumption that there won't be a TypeError? But it seems like we can tier up as long as we catch the error *outside* of the function itself, so I suppose I'll do it that way.
Comment 12 Ross Kirsling 2020-03-24 16:01:25 PDT
Created attachment 394430 [details]
Patch
Comment 13 Ross Kirsling 2020-03-26 14:41:46 PDT
Ping?
Comment 14 Saam Barati 2020-03-30 13:28:32 PDT
Comment on attachment 394430 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:15
> +        That is, we're always obliged to verify lastIndex, even when we don't use the value.

what does verify mean?

> Source/JavaScriptCore/ChangeLog:16
> +        DFG, in particular, must make sure strength reductions don't apply when lastIndex isn't an unsigned integer.

why?

> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:71
> +        setLastIndex(globalObject, 0);

this seems like it could throw too, and override previously thrown exception?

> Source/JavaScriptCore/runtime/RegExpObjectInlines.h:116
> +        setLastIndex(globalObject, 0);

ditto
Comment 15 Ross Kirsling 2020-03-30 14:48:07 PDT
Created attachment 394967 [details]
Patch
Comment 16 Saam Barati 2020-03-30 16:50:25 PDT
Comment on attachment 394967 [details]
Patch

r=me
Comment 17 EWS 2020-03-30 16:57:27 PDT
Committed r259246: <https://trac.webkit.org/changeset/259246>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394967 [details].
Comment 18 Radar WebKit Bug Importer 2020-03-30 16:58:16 PDT
<rdar://problem/61080650>