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

RESOLVED FIXED in Firefox 34

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: James Keane, Assigned: jandem)

Tracking

({dev-doc-complete, regression, site-compat})

33 Branch
mozilla35
dev-doc-complete, regression, site-compat
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ wontfix, firefox34+ fixed, firefox35+ fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8503234 [details]
Degraded regular expression

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.
(Reporter)

Updated

4 years ago
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
tracking-firefox33: --- → ?
Ever confirmed: true
Flags: needinfo?(jdemooij)
Keywords: regression

Updated

4 years ago
Version: unspecified → 33 Branch
(Reporter)

Comment 2

4 years ago
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.
(Reporter)

Comment 3

4 years ago
Created attachment 8503542 [details]
Reduced example
Attachment #8503234 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
Created attachment 8503586 [details]
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.
(Assignee)

Comment 5

4 years ago
Created attachment 8503597 [details] [diff] [review]
Patch

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)
(Assignee)

Comment 6

4 years ago
[Tracking Requested - why for this release]:
status-firefox32: --- → unaffected
status-firefox33: --- → affected
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?
(Assignee)

Updated

4 years ago
OS: Linux → All
Hardware: x86_64 → All
Attachment #8503597 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 8

4 years ago
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-
status-firefox33: affected → wontfix
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
https://hg.mozilla.org/mozilla-central/rev/dc10fcd30554
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox35: affected → fixed
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.
status-firefox33: wontfix → ?
Keywords: site-compat
Sorry but we only take critical issues for 33.1...
This will wait for 34.
status-firefox33: ? → wontfix
You need to log in before you can comment on or make changes to this bug.