Bug 155145 – RegExpObject::exec/match should handle errors gracefully

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

...

[Mark Lam]

<rdar://problem/27435934>

[Mark Lam]

Created attachment 294648 [details]
proposed patch.

[Keith Miller]

Comment on attachment 294648 [details]
proposed patch.

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

r=me with comments.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:799
> +    ASSERT(!scope.exception() || !input);

Don't you want ASSERT(scope.Exception() == !input)? Or is there some code path that returns nullptr but doesn't throw an exception?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:820
>      JSString* input = argument.toStringOrNull(exec);

Why not have the same assertion here you have above?

> Source/JavaScriptCore/dfg/DFGOperations.cpp:868
>      JSString* input = argument.toStringOrNull(exec);

Ditto.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:107
>      JSString* string = exec->argument(0).toStringOrNull(exec);

Ditto.

> Source/JavaScriptCore/runtime/RegExpPrototype.cpp:122
>      JSString* string = exec->argument(0).toStringOrNull(exec);

Ditto.

[Mark Lam]

Comment on attachment 294648 [details]
proposed patch.

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

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:799
>> +    ASSERT(!scope.exception() || !input);
> 
> Don't you want ASSERT(scope.Exception() == !input)? Or is there some code path that returns nullptr but doesn't throw an exception?

You're right.  I will change this to ASSERT(!!scope.exception() == !input).

>> Source/JavaScriptCore/dfg/DFGOperations.cpp:820
>>      JSString* input = argument.toStringOrNull(exec);
> 
> Why not have the same assertion here you have above?

I'll add the same assertion here and in the other similar places below.

[Mark Lam]

Created attachment 294712 [details]
patch for landing.

[Mark Lam]

Thanks for the review.  Landed in r208698: <http://trac.webkit.org/r208698>.