Status

()

Core
JavaScript: Standard Library
RESOLVED DUPLICATE of bug 998785
4 years ago
3 years ago

People

(Reporter: zpao, Unassigned)

Tracking

unspecified
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 affected, firefox30 affected, firefox31 affected)

Details

(Whiteboard: [js:p1])

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
Till is on PTO, I should be able to take a look later this week.
Flags: needinfo?(jdemooij)
Whiteboard: [js:p1]
Hannes can you look into this? It could be a regression from bug 953013 but unfortunately Till is on PTO.
Flags: needinfo?(jdemooij) → needinfo?(hv1989)
(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.
Depends on: 953013
Flags: needinfo?(hv1989)
status-firefox29: --- → affected
status-firefox30: --- → affected
status-firefox31: --- → affected
@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)?
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.
(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.
(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"?
(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.
Any update? I just hit this again today.
(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.
Depends on: 976446
Bug 998785 has more details, so marking as duplicate of that. (With a fix forthcoming in that bug.)
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 998785
You need to log in before you can comment on or make changes to this bug.