Closed Bug 1077514 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

32 Branch
x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox32 --- wontfix
firefox33 + wontfix
firefox34 + fixed
firefox35 + fixed
firefox36 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: johnm, Assigned: bhackett1024)

References

Details

(4 keywords)

Attachments

(1 file)

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.
Component: Untriaged → JavaScript Engine
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
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.
Flags: needinfo?(bhackett1024)
Did we use to actually _complete_ the match before that change?  Or just hang forever?
Attached patch patchSplinter Review
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?
(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.
Attachment #8501133 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ba5d59a26a47
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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)
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.
Flags: needinfo?(bhackett1024)
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?
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.