Bug 155145 - RegExpObject::exec/match should handle errors gracefully
Summary: RegExpObject::exec/match should handle errors gracefully
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 164681 164695
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-07 15:48 PST by Filip Pizlo
Modified: 2016-11-14 11:30 PST (History)
8 users (show)

See Also:


Attachments
proposed patch. (9.16 KB, patch)
2016-11-12 20:08 PST, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff
patch for landing. (9.65 KB, patch)
2016-11-14 10:48 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-03-07 15:48:17 PST
...
Comment 1 Mark Lam 2016-11-11 16:14:37 PST
<rdar://problem/27435934>
Comment 2 Mark Lam 2016-11-12 20:08:42 PST
Created attachment 294648 [details]
proposed patch.
Comment 3 Keith Miller 2016-11-14 10:16:21 PST
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.
Comment 4 Mark Lam 2016-11-14 10:24:01 PST
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.
Comment 5 Mark Lam 2016-11-14 10:48:33 PST
Created attachment 294712 [details]
patch for landing.
Comment 6 Mark Lam 2016-11-14 11:30:07 PST
Thanks for the review.  Landed in r208698: <http://trac.webkit.org/r208698>.