Bug 155145

Summary:

RegExpObject::exec/match should handle errors gracefully

Product:

WebKit

Reporter:

Filip Pizlo <fpizlo>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ggaren, jfbastien, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Bug Depends on:

164681, 164695

Bug Blocks:

Attachments:

Description	Flags
proposed patch.	keith_miller: review+
patch for landing.	none

Filip Pizlo

Reported 2016-03-07 15:48:17 PST

...

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Mark Lam

Comment 1 2016-11-11 16:14:37 PST

<rdar://problem/27435934>

Mark Lam

Comment 2 2016-11-12 20:08:42 PST

Created attachment 294648 [details] proposed patch.

Keith Miller

Comment 3 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.

Mark Lam

Comment 4 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.

Mark Lam

Comment 5 2016-11-14 10:48:33 PST

Created attachment 294712 [details] patch for landing.

Mark Lam

Comment 6 2016-11-14 11:30:07 PST

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

Note You need to log in before you can comment on or make changes to this bug.