Bug 206776

Summary: Incomplete braced quantifiers should be syntax errors in Unicode patterns only
Product: WebKit Reporter: Alexey Shvayka <ashvayka>
Component: JavaScriptCoreAssignee: Alexey Shvayka <ashvayka>
Status: RESOLVED FIXED    
Severity: Trivial CC: commit-queue, darin, ews-watchlist, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Alexey Shvayka
Reported 2020-01-24 15:50:18 PST
Test case: /{1/u Expected: SyntaxError thrown Actual: RegExp instance ECMA262: https://tc39.es/ecma262/#prod-annexB-Term (/u flag precludes the use of ExtendedAtom) Term[U, N] :: [~U] ExtendedAtom[?N] ExtendedAtom[N] :: InvalidBracedQuantifier Test262: https://test262.report/browse/annexB/built-ins/RegExp/prototype/compile/pattern-string-invalid-u.js https://test262.report/browse/built-ins/RegExp/unicode_restricted_incomple_quantifier.js https://test262.report/browse/language/literals/regexp/u-invalid-extended-pattern-char.js
Attachments
Patch (9.02 KB, patch)
2020-01-24 16:28 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (29.35 KB, patch)
2020-01-29 14:47 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Patch (29.47 KB, patch)
2020-01-30 10:16 PST, Alexey Shvayka 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Alexey Shvayka
Comment 1 2020-01-24 16:28:52 PST
Created attachment 388739 [details] Patch
Darin Adler
Comment 2 2020-01-25 09:02:52 PST
Comment on attachment 388739 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=388739&action=review > Source/JavaScriptCore/yarr/YarrErrorCode.cpp:43 > + REGEXP_ERROR_PREFIX "incomplete {} quantifier for unicode pattern", // QuantifierIncomplete I think Unicode needs a capital "U" > Source/JavaScriptCore/yarr/YarrParser.h:817 > + if (m_isUnicode) { > + m_errorCode = ErrorCode::QuantifierIncomplete; > + break; > + } Seems like we need at least one test case that checks that we get the expected error. Maybe that’s not good in WPT tests because the actual error messages will be different across platforms, but if so then we could add a WebKit-specific test. Would you consider adding one? > Source/JavaScriptCore/yarr/YarrParser.h:-817 > + // If we did not find a complete quantifer, fall through to the default case. > + FALLTHROUGH; > } > - // if we did not find a complete quantifer, fall through to the default case. > - FALLTHROUGH; This change seems to make the code a tiny bit worse. Having the "FALLTHROUGH" macro inside a block doesn’t quite seem like the best style, and there’s no need to make this change except for stylistic reasons.
Alexey Shvayka
Comment 3 2020-01-29 14:47:04 PST
Created attachment 389191 [details] Patch Capitalize Unicode in error messages and add tests.
Alexey Shvayka
Comment 4 2020-01-29 14:57:02 PST
(In reply to Darin Adler from comment #2) Thank you for review(s), Darin. I always appreciate your detailed comments. > This change seems to make the code a tiny bit worse. Having the > "FALLTHROUGH" macro inside a block doesn’t quite seem like the best style, > and there’s no need to make this change except for stylistic reasons. According to my "research" (grep), "FALLTHROUGH" outside block is relatively popular in Source/WebCore (5 inside, 3 outside), but it is the only occurrence in Source/JavaScriptCore (24 inside, 1 of which is in this file).
WebKit Commit Bot
Comment 5 2020-01-30 09:03:12 PST
Comment on attachment 389191 [details] Patch Rejecting attachment 389191 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 389191, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/JSTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: https://webkit-queues.webkit.org/results/13314000
Alexey Shvayka
Comment 6 2020-01-30 10:16:57 PST
Created attachment 389264 [details] Patch Set reviewer.
WebKit Commit Bot
Comment 7 2020-01-30 11:44:21 PST Comment hidden (obsolete)
WebKit Commit Bot
Comment 8 2020-01-30 11:44:34 PST
The commit-queue encountered the following flaky tests while processing attachment 389264 [details]: fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 9 2020-01-30 13:27:17 PST
Comment on attachment 389264 [details] Patch Clearing flags on attachment: 389264 Committed r255452: <https://trac.webkit.org/changeset/255452>
WebKit Commit Bot
Comment 10 2020-01-30 13:27:19 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 11 2020-01-30 13:28:16 PST
<rdar://problem/59039623>
Note You need to log in before you can comment on or make changes to this bug.