980781 - Error executing regex

Status: RESOLVED DUPLICATE of bug 998785

(Core :: JavaScript: Standard Library, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

This is a fairly complicated regex and test string, but it works in beta, release, and other browsers. Aurora and Nightly both throw an error. I haven't tried to find a regression range beyond that.

This is extracted from some code we have http://jsbin.com/qowelovu/1/edit?js,console

Comment 1

[Jan de Mooij [:jandem]]

Till is on PTO, I should be able to take a look later this week.

Comment 2

[Jan de Mooij [:jandem]]

Hannes can you look into this? It could be a regression from bug 953013 but unfortunately Till is on PTO.

Comment 3

[Hannes Verschore [:h4writer]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> Hannes can you look into this? It could be a regression from bug 953013 but
> unfortunately Till is on PTO.

Right on the money. Bug 953013 caused this. Investigating.

Comment 4

[Hannes Verschore [:h4writer]]

@Paul, so yarr (regexp engine) limits the amount of matches that can happen. (Mostly to not run into bad exponential performance behaviour). So after X matches (2500000) it will throw an error. Previous behaviour was that it would always return there was no match. So your example said there were no matches without actually testing all cases.

I don't think we can do much, except maybe if the limit is only for performance (which I think), we can ditch the limit and add code that checks if it needs to stop executing (i.e. the unresponsive dialog)?

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Throwing an error into userland that didn't happen before and must now be managed is not great. If it's an implementation detail, then I don't think this should be exposed or it should be exposed in a meaningful way. This error message doesn't give insight into why this code started throwing when it's been working without issue for a long time and continues to work in other engines.

Is there no spec behavior here? Would be great to have a behavior documented and standardized across engines. Or even not standardized but documented. I've never heard of a valid regex throwing.

Comment 6

[Till Schneidereit [:till]]

(In reply to Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) from comment #5)
> Throwing an error into userland that didn't happen before and must now be
> managed is not great. If it's an implementation detail, then I don't think
> this should be exposed or it should be exposed in a meaningful way. This
> error message doesn't give insight into why this code started throwing when
> it's been working without issue for a long time and continues to work in
> other engines.

While I agree that the new behavior isn't ideal, note that the last part isn't true: the regexp *didn't* work before. Instead, it silently swallowed an error inside the regexp engine and pretended that the operation had finished successfully, with the string not matching the expression. Since we found expressions that would return true if it weren't for the internal abort, we decided that throwing an exception is strictly better than having a silent error, with potentially security-relevant consequences.

Ideally, we'd just not have the restriction in the first place, as clearly it is only required because of an implementation detail in Yarr. That, however, is much more difficult than the band-aid fix we have in place now.

Comment 7

[Jason Orendorff [:jorendorff]]

(In reply to Hannes Verschore [:h4writer] from comment #4)
> @Paul, so yarr (regexp engine) limits the amount of matches that can happen.
> (Mostly to not run into bad exponential performance behaviour). So after X
> matches (2500000) it will throw an error.

We obviously need to change this. Don't break the web.

Can we replace this hard limit with "after every X matches we CheckForInterrupt"?

Comment 8

[Till Schneidereit [:till]]

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> Can we replace this hard limit with "after every X matches we
> CheckForInterrupt"?

This should be possible, yes. Short of replacing Yarr - i.e., the long-term solution, this is probably the best option.

I'll take a look at what's entailed over the next couple of days.

Comment 9

[Rob Arnold [:robarnold]]

Any update? I just hit this again today.

Comment 10

[Till Schneidereit [:till]]

(In reply to Rob Arnold [:robarnold] from comment #9)
> Any update? I just hit this again today.

Yes, the replacement for Yarr is very close to landing in bug 976446.

Comment 11

[Till Schneidereit [:till]]

Bug 998785 has more details, so marking as duplicate of that. (With a fix forthcoming in that bug.)