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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[John Murray]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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...

Comment 2

[Loic]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Did we use to actually _complete_ the match before that change?  Or just hang forever?

Comment 4

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

So why is there a difference between Mac and Windows here?  A 10x performance difference at that?

Comment 6

[Brian Hackett [Laid off!]]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 9

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

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.

Comment 11

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5d59a26a47

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba5d59a26a47

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 15

[Brian Hackett [Laid off!]]

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

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8501133 [details] [diff] [review]
patch

Beta+
Aurora+

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/29275e2e7e21

Needs major rebasing for Beta34 uplift.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 19

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5238acab8176

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/5238acab8176

Comment 21

[Florian Scholz (Open Web Docs)]

Added to https://developer.mozilla.org/en-US/Firefox/Releases/34/Site_Compatibility#JavaScript