Bug 155139

Summary:

RegExp.prototype.exec() should call into Yarr at most once

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Filip Pizlo <fpizlo>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

barraclough, benjamin, commit-queue, ggaren, keith_miller, mark.lam, msaboff, saam

Priority:

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
work in progress	none
the patch	saam: review+
patch for landing	none
patch for landing	none

Description Filip Pizlo 2016-03-07 14:57:23 PST

Patch forthcoming.

Comment 1 Filip Pizlo 2016-03-07 14:57:47 PST

Created attachment 273218 [details]
work in progress

Comment 2 Filip Pizlo 2016-03-07 15:26:58 PST

Created attachment 273228 [details]
the patch

Comment 3 Saam Barati 2016-03-07 15:37:42 PST

Comment on attachment 273228 [details]
the patch

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

r=me w/ comments

> Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp:115
> +        array = JSArray::tryCreateUninitialized(vm, globalObject->regExpMatchesArrayStructure(), regExp->numSubpatterns() + 1);

I think this can fail.

> Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp:125
> +        RELEASE_ASSERT(array);

Is there ever a valid reason why this would fail? If so, I think we should throw an exception instead.

> Source/JavaScriptCore/runtime/RegExpObject.cpp:188
> +    String input = string->value(exec);

Can't this throw OOM?

Comment 4 Filip Pizlo 2016-03-07 15:43:31 PST

(In reply to comment #3)
> Comment on attachment 273228 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=273228&action=review
> 
> r=me w/ comments
> 
> > Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp:115
> > +        array = JSArray::tryCreateUninitialized(vm, globalObject->regExpMatchesArrayStructure(), regExp->numSubpatterns() + 1);
> 
> I think this can fail.
> 
> > Source/JavaScriptCore/runtime/RegExpMatchesArray.cpp:125
> > +        RELEASE_ASSERT(array);
> 
> Is there ever a valid reason why this would fail? If so, I think we should
> throw an exception instead.
> 
> > Source/JavaScriptCore/runtime/RegExpObject.cpp:188
> > +    String input = string->value(exec);
> 
> Can't this throw OOM?

I think you're right about all of these things.  For now, I'm just mirroring the behavior that the code previously had.

Comment 5 Filip Pizlo 2016-03-07 15:44:28 PST

I have a fix for the debug build.

Comment 6 Filip Pizlo 2016-03-07 15:51:01 PST

Created attachment 273231 [details]
patch for landing

Comment 7 Filip Pizlo 2016-03-07 15:56:57 PST

Created attachment 273233 [details]
patch for landing

Comment 8 Filip Pizlo 2016-03-07 16:34:57 PST

Landed in http://trac.webkit.org/changeset/197715