Bug 178175 – Invalid numeric and named references should be early syntax errors

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

[Michael Saboff]

This Test262 test is failing - JSTests/test262/test/built-ins/RegExp/named-groups/unicode-references.js
Exception: SyntaxError: Invalid regular expression: invalid backreference for unicode pattern
at JSTests/test262/test/built-ins/RegExp/named-groups/unicode-references.js:33

The line in question is:
  assert(compareArray(["bab", "b"], "bab".match(/\k<a>(?<a>b)\w\k<a>/u)));

Here the named group “a” is being referenced before if is defined.  Seems like we’re doing the right thing, but we need to check.

[Michael Saboff]

<rdar://problem/34844068>

[Alexey Shvayka]

Created attachment 394228 [details]
Patch

[Alexey Shvayka]

I will add ChangeLog entry in a minute; just making sure we are not tackling the same issue separately.

[Alexey Shvayka]

Created attachment 394256 [details]
Patch

Add ChangeLog and fix build.

[Alexey Shvayka]

Created attachment 394257 [details]
Patch

Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.

[Ross Kirsling]

(In reply to Alexey Shvayka from comment #5)
> Created attachment 394257 [details]
> Patch
> 
> Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.

I think we intentionally avoid this? I think for a release assert you may need to use NO_RETURN_DUE_TO_CRASH.

[Alexey Shvayka]

> > Use [[noreturn]] instead of NO_RETURN_DUE_TO_ASSERT.
> 
> I think we intentionally avoid this? I think for a release assert you may
> need to use NO_RETURN_DUE_TO_CRASH.

Thank you, there are indeed no usages of [[noreturn]] in WebKit code base.

API tests fail because named and numeric references should be disabled for URLFilterParser. I might have need to bring back isValidNamedForwardReference().

[Alexey Shvayka]

Created attachment 394400 [details]
Patch

Use NO_RETURN_DUE_TO_CRASH, fix API tests, improve error messages, and tweak naming.

[Ross Kirsling]

Comment on attachment 394400 [details]
Patch

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

Nice work!
(Looks like you need a rebase though.)

> Source/JavaScriptCore/yarr/YarrErrorCode.h:55
> +    InvalidNamedBackreference,

Interesting how it's `Backreference` here and `BackReference` elsewhere...though this isn't your fault. :P

[Alexey Shvayka]

Created attachment 394553 [details]
Patch

Rebase patch, set reviewer, add second m_kIdentityEscapeSeen,  check and add a comment explaining m_isUnicode check in handleIllegalReferences().

[Alexey Shvayka]

Created attachment 394555 [details]
Patch

Camelcase "backreference".

[Alexey Shvayka]

Thank you for taking time to review this patch, Ross.
It is quite tricky.

[Ross Kirsling]

Comment on attachment 394555 [details]
Patch

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

> Source/JavaScriptCore/yarr/YarrParser.h:919
> +        if (m_kIdentityEscapeSeen && !m_captureGroupNames.isEmpty()) {
> +            m_errorCode = ErrorCode::InvalidNamedBackReference;
> +            return;
> +        }

Do we need a test for this path, if nothing was failing without it?

[Alexey Shvayka]

(In reply to Ross Kirsling from comment #13)
> Do we need a test for this path, if nothing was failing without it?

We have this covered by
  test/language/literals/regexp/named-groups/invalid-incomplete-groupname.js
  test/language/literals/regexp/named-groups/invalid-incomplete-groupname-2.js
  test/language/literals/regexp/named-groups/invalid-incomplete-groupname-3.js

I just didn't run the tests properly.

[Ross Kirsling]

(In reply to Alexey Shvayka from comment #14)
> (In reply to Ross Kirsling from comment #13)
> > Do we need a test for this path, if nothing was failing without it?
> 
> We have this covered by
>   test/language/literals/regexp/named-groups/invalid-incomplete-groupname.js
>  
> test/language/literals/regexp/named-groups/invalid-incomplete-groupname-2.js
>  
> test/language/literals/regexp/named-groups/invalid-incomplete-groupname-3.js
> 
> I just didn't run the tests properly.

Oh okay, thanks for confirming.

[EWS]

Committed r259026: <https://trac.webkit.org/changeset/259026>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394555 [details].