All users were logged out of Bugzilla on October 13th, 2018

Unresponsive script doing String.match - possible regression cause by irregexp

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: johnm, Assigned: bhackett)

Tracking

(4 keywords)

32 Branch
mozilla36
x86_64
Windows 8.1
dev-doc-complete, regression, site-compat, testcase
Points:
---

Firefox Tracking Flags

(firefox32 wontfix, firefox33+ wontfix, firefox34+ fixed, firefox35+ fixed, firefox36 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140923175406

Steps to reproduce:

I was seeing a problem when using Zimbra in FF32 but not in Chrome.

I have isolated the issue into a jsfiddle at http://jsfiddle.net/e1m4jy51/6/

The code there is:

var s1="a"
var ts
//
// Make a 1024-char string (2 to the power 10)
for (var i=0; i<10; i++) s1=s1+s1
alert(s1.length)
// Time the match
ts=Date.now()
alert(s1.match(/.*"(\w+Request)"/)+" "+(Date.now()-ts))
//
// Make one 32x larger (32=2 to the power 5)
var s2=s1
for (var i=0; i<5; i++) s2=s2+s2
alert(s2.length)
// Time the match
// On FF32 we get "unresponsive script" dialog, and repeated waiting doesn't get it completed
ts=Date.now()
alert(s2.match(/.*"(\w+Request)"/)+" "+(Date.now()-ts))



Actual results:

String.match(/.*"(\w+Request)"/) on a long (32k) string gives the "unresponsive script" dialog after 10 seconds. I tried continuing up to ten times but it didn't complete. During each 10-second wait my 4-core system showed FF using 25% CPU, i.e. an entire core.

I don't think this happened on previous FF versions.

See http://community.zimbra.com/zforums914/f/1867/t/1076315


Expected results:

match() should have returned, even if it took a while.

Updated

4 years ago
Component: Untriaged → JavaScript Engine
Keywords: regression, site-compat, testcase
Product: Firefox → Core
I just tried the testcase in the jsfiddle in Firefox 32 on Mac and it completes just fine; takes about 1s.  That's in both 64-bit and 32-bit mode.

However I can confirm the issue on Windows in Firefox 32 (using browserstack, not a local run).  Same thing in 34 aurora, on Windows...
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

4 years ago
Regression range on Win 7:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cbe4f69c2e9c&tochange=e017c15325ae

Suspected:
Brian Hackett — Bug 1015677 - Check for interrupts while backtracking during irregexp execution, r=jandem.
Blocks: 1015677
status-firefox32: --- → affected
tracking-firefox33: --- → ?
tracking-firefox34: --- → ?
tracking-firefox35: --- → ?

Updated

4 years ago
Flags: needinfo?(bhackett1024)
Did we use to actually _complete_ the match before that change?  Or just hang forever?
status-firefox32: affected → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → affected
tracking-firefox33: ? → +
tracking-firefox34: ? → +
tracking-firefox35: ? → +
(Assignee)

Comment 4

4 years ago
Created attachment 8501133 [details] [diff] [review]
patch

I haven't tried to reproduce this but the interrupt mechanism for regexps does have a design flaw when the regexp takes long enough to execute that the slow script dialog triggers but will eventually be able to complete.  When we interrupt regexp jitcode the regexp stops executing entirely, and after handling the interrupt we start execution over from the beginning.  So hitting 'continue' on the slow script dialog will never actually lead to the regexp completing as long as the dialog hits again later in execution.

Fixing this for regexp jitcode would be tricky as we would need to be able to call the interrupt handler from within irregexp jitcode, handling GC, reentrant script execution etc.  The attached patch uses a simpler approach of executing a regexp in the bytecode interpreter if the jitcode was interrupted.  The bytecode will execute more slowly but interrupts can be handled without the interpreter losing its place, so the regexp will eventually finish executing.
Assignee: nobody → bhackett1024
Attachment #8501133 - Flags: review?(jdemooij)
Flags: needinfo?(bhackett1024)
So why is there a difference between Mac and Windows here?  A 10x performance difference at that?
(Assignee)

Comment 6

4 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> So why is there a difference between Mac and Windows here?  A 10x
> performance difference at that?

Is this a Mac vs. Windows things, or an x64 vs. x86 thing?  We should generate the same jitcode on an architecture regardless of the platform, but irregexp can generate faster code on x64 than x86 by matching more characters at a time (though a 10x difference seems pretty extreme).
> Is this a Mac vs. Windows things, or an x64 vs. x86 thing?

I believe the former, given my results running a 32-bit Mac build; see comment 1.  Unless I messed up that testing somehow, of course...
Comparing yesterday's 32-bit Nightly to the 64-bit Nightly on Windows 7, the 32-bit Nightly gives me the unresponsive script dialogue after being unresponsive for a while, but the 64-bit Nightly finishes after a few seconds.
Hmm, this is odd - I can't reproduce the 64-bit Nightly finishing it now. It swear it worked the first time, but now it goes unresponsive just like the 32-bit version.
Ah, and now the 32-bit one finished without timing out. My PC is busy at the moment, so maybe it's just right on the edge of hitting the timeout; sorry about the noise.

Updated

4 years ago
Attachment #8501133 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ba5d59a26a47
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1084280
34 and 35 are marked as affected. This has now been on m-c for 9 days. What do you think about uplifting this fix to Beta and Aurora?
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 15

4 years ago
Comment on attachment 8501133 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 976446
[User impact if declined]: long running regexps might never terminate
[Risks and why]: none
Flags: needinfo?(bhackett1024)
Attachment #8501133 - Flags: approval-mozilla-beta?
Attachment #8501133 - Flags: approval-mozilla-aurora?
Comment on attachment 8501133 [details] [diff] [review]
patch

Beta+
Aurora+
Attachment #8501133 - Flags: approval-mozilla-beta?
Attachment #8501133 - Flags: approval-mozilla-beta+
Attachment #8501133 - Flags: approval-mozilla-aurora?
Attachment #8501133 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/29275e2e7e21

Needs major rebasing for Beta34 uplift.
status-firefox35: affected → fixed
status-firefox36: --- → fixed
Flags: needinfo?(bhackett1024)
Keywords: branch-patch-needed
Brian - We're going to need a branch specific patch to take this fix in Firefox 34. Can you get that ready this week in time for beta6, which builds on Monday, Nov 3?
status-firefox34: affected → fixed
Keywords: branch-patch-needed
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/5238acab8176
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
QA Whiteboard: [good first verify]

Updated

4 years ago
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.