Closed Bug 1081175 Opened 5 years ago Closed 5 years ago

(latin1strings) Degraded regular expression performance (infinite loop?)

Categories

(Core :: JavaScript Engine, defect)

33 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox32 --- unaffected
firefox33 + wontfix
firefox34 + fixed
firefox35 + fixed

People

(Reporter: james.keane, Assigned: jandem)

Details

(Keywords: dev-doc-complete, regression, site-compat)

Attachments

(3 files, 1 obsolete file)

Attached file Degraded regular expression (obsolete) —
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/38.0.2125.101 Safari/537.36

Steps to reproduce:

Google Caja CSS lexer causes a browser lock up on FIrefox 33 for sufficiently large css files.

See related Caja issue: https://code.google.com/p/google-caja/issues/detail?id=1941


Actual results:

Using the provided example, the time required to lex the bootstrap css (on my machine):

Chrome 38: ~50ms
Firefox 32: ~4ms
Firefox 33 forced unicode: ~8ms
Firefox 33 without forced unicode: _never completes_.


Expected results:

Firefox 33 should have similar performance as Firefox 32.
Summary: (latin1strings) Degraded regular expression performance → (latin1strings) Degraded regular expression performance (infinite loop?)
[Tracking Requested - why for this release]: Serious web compat regression...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jdemooij)
Keywords: regression
Version: unspecified → 33 Branch
I have reduced the test case to the absolute minimum.

It is definitely a performance regression as increasing the input size shows exponential increase in time.
Attached file Reduced example
Attachment #8503234 - Attachment is obsolete: true
Attached file Shell testcase
Thanks for the bug report and the great testcase!

It looks like this is an issue in the regex engine, will investigate more.
Attached patch PatchSplinter Review
One-line fix for a bug introduced when irregexp was imported (bug 976446). We were appending to a vector instead of replacing the old elements, like the original code does.

The problem is in Latin1-only code though, so it was not a problem until Latin1 strings were added to SpiderMonkey.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8503597 - Flags: review?(bhackett1024)
Flags: needinfo?(jdemooij)
[Tracking Requested - why for this release]:
OS: Linux → All
Hardware: x86_64 → All
Attachment #8503597 - Flags: review?(bhackett1024) → review+
Comment on attachment 8503597 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Older bug exposed by bug 998392.
[User impact if declined]: Very slow websites/apps, hangs.
[Describe test coverage new/current, TBPL]: Patch adds a testcase.
[Risks and why]: Low risk - fix is a one-liner and it matches the code upstream.
[String/UUID change made/needed]: None.
Attachment #8503597 - Flags: approval-mozilla-beta?
Attachment #8503597 - Flags: approval-mozilla-aurora?
Comment on attachment 8503597 [details] [diff] [review]
Patch

We're too lage for Firefox 33, which ships on Tuesday. Let's take this in Firefox 34 after it lands on m-c.
Attachment #8503597 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/dc10fcd30554
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment on attachment 8503597 [details] [diff] [review]
Patch

Aurora+
Attachment #8503597 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Nominating for 33.1 as the patch is very small and low risk.
Sorry but we only take critical issues for 33.1...
This will wait for 34.
You need to log in before you can comment on or make changes to this bug.