452451 - enable relimit by default and find out if it breaks the web

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Crowder]

Bug 330569 introduced a feature that allows us to bail out of exponentially (worse than O(n^3)) complex regular expressions fairly quickly, depending on the length of the input string -- this might need more tweaking, see below.

We should turn on this feature by default in the browser (until now it has only been included as an "off" about:config feature), and see how much stuff breaks.  We may also want to install a hard upper-limit on the number of backtracks we're EVER going to do for an expression (another about:config option with a configurable number?) -- right now we base the logic only on the length of the input string.

Comment 1

[Brendan Eich [:brendan]]

There'a bug in nsJSEnvironment.cpp's GetOptionsProperty -- it censors updates to JSOPTION_RELIMIT. SetOptionsProperty was updated to allow JSOPTION_RELIMIT in addition to STRICT and WERROR, but GetOptionsProperty was not updated.

/be

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

This is making relimit 'on' by default in the browser and this is fixing the issue reported by Brendan on comment 2.

This patch isn't making relimit 'on' by default in the js engine AFAICT.

Comment 3

[Mounir Lamouri (:mounir)]

For the upper limit of backtracks, if you have a value to propose, I can try to implement it. Otherwise, I think a follow-up would be better.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 444402 [details] [diff] [review]
Patch v1

I'm not the right person to review this

Comment 5

[Jeff Walden [:Waldo]]

I haven't done much more than skim just yet, but at first glance this looks like a straightforward change that's probably fine.

The question is mostly one of policy and whether we *should* do this.  I think we should, to not spiral so far out of control on garbage input -- adding more people to hear potential arguments for why we shouldn't...

Comment 6

[Johnny Stenback (:jst)]

Seems fine with me fwiw.

Comment 7

[Brian Crowder]

I think PCRE was using a hard limit of something like 1,000,000 backtracks.  We might want to investigate the limits set by other engines on the web?

Comment 8

[Brendan Eich [:brendan]]

The issue here is whether we hide bugs. Sometimes we hear "hangs Firefox, does not hang (Safari|Chrome|...)". Reasons vary (including user-agent sniffing, evil) but I seem to recall occasionally there's a bug in our regexp code that leads to exponential blow-up where other engines avoid that. Or we just don't have a l33t optimization, where that optimization would mismatch quickly.

If we switch to YARR then no worries :-P.

/be

Comment 9

[Mike Shaver (:shaver emeritus)]

In all the cases I can remember, the Safari engine just errors out after some limit, while we keep on churning until the slow script timer breaks our face.

Comment 10

[Brendan Eich [:brendan]]

Ok, let's try this then. It's pre-beta, good time to turn it on and see if our face breaks less.

/be

Comment 11

[Mounir Lamouri (:mounir)]

I assume there is no sr needed. Marking checkin-needed.

Comment 12

[Justin Wood (:Callek)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/1711cfd31e7d

Comment 13

[Justin Wood (:Callek)]

Failed Jo/Jd on all platforms. (one debug log: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1274325551.1274326694.6306.gz&fulltext=1 )

Failing js1_5/extensions/regress-330569.js

Pushed backout: http://hg.mozilla.org/mozilla-central/rev/43e4cc9cb6a8
and merge: http://hg.mozilla.org/mozilla-central/rev/2e60eafd92ba

I have a gut feeling this can be fixed by fixing the failing test, but I held the tree too long, and needed to backout.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: Patch v1.1

Oups, I didn't know js tests where run from the browser in the tinderbox. I tried them but with jsDriver.pl. The relimit js tests were assuming the relimit was always disabled and |options('relimit')| was disabling relimit instead of enabling it because the patch is enabling relimit for the brower.
Anyway, this new patch should works correctly (tested locally at least).

Sorry for the annoyance, Justin.

Comment 15

[Justin Wood (:Callek)]

(In reply to comment #14)
> Created an attachment (id=446622) [details]
> Patch v1.1

Repushed as: http://hg.mozilla.org/mozilla-central/rev/12af2ba87d4c