976369 - Firefox does not fetch additional segments of dzone web page

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jacques Lemire]

Attachment: Screenshot from 2014-02-24 18:58:03.png

User Agent: Mozilla/5.0 (X11; Linux i686; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140224004003

Steps to reproduce:

Go to http://www.dzone.com/links/queue.html
Scroll down to the bottom to see more posts.



Actual results:

The fetch indicator shows NO additional segments of web page. It is struck at 50.


Expected results:

Additional segments of dzone web page are loaded. Just as they are loaded correctly and quickly with Google/Chrome and Seamonkey.

Comment 1

[Alice0775 White]

An error shown in Error Console:

Error: an error occurred while executing regular expression
Source File: http://www.dzone.com/links/combined.js.h-1448902423.pack
Line: 445

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/441af05d123b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140110055749
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/9ecbd1ca14e2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140110062749
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=441af05d123b&tochange=9ecbd1ca14e2

Suspect:
9ecbd1ca14e2	Till Schneidereit — Bug 953013 - throw exceptions for uncorrectly-interpreted regular expressions instead of treating them as non-matching. r=jandem

Comment 2

[Matthieu Rivaud]

I'd be willing to place a bet on this RegExp (found on line 445), which seems to be a (poorly expressed) trim expression :

/^(\s|\u00A0)+|(\s|\u00A0)+$/

We could be reaching the 999999 pattern matches limit Till explained in Bug 953013 comment 5 ...

Comment 3

[Till Schneidereit [:till]]

Attachment: Shell test case

Yup, that's caused by my change. It works in v8, though, and gives correct results in jsc. Whether the latter is by accident or because Yarr's unpatched behavior is actually correct here, I don't know.

Attached is a shell test case, which I'm investigating.

Comment 4

[Till Schneidereit [:till]]

Attachment: Increase Yarr's match limit to unregress dzone website.

So, the regexp Matthieu identified in comment 2 (thanks!) hits the match limit indeed: for the current input (which is ~99k chars of html source), it creates about 1.8M matches. That's not an ideal implementation of String#trim ...

Anyway, I think we should just bump the match limit and be done with this: the alternatives are to change back to the broken behavior of just returning false when the matching actually failed, or to drastically change Yarr.

Jandem, this simply bumps the limit from 1M to 2.5M. The test still completes in a couple of seconds (on my fairly high-end machine, yes.)

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8381893 [details] [diff] [review]
Increase Yarr's match limit to unregress dzone website.

Review of attachment 8381893 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

Comment 6

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52577b51029a

Thanks for the quick review!

Comment 7

[Jan de Mooij [:jandem]]

Follow-up to update the regexp-match-limit.js jit-test, to fix the jit-test failures on TBPL:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5103fc7d8c39

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8381893 [details] [diff] [review]
Increase Yarr's match limit to unregress dzone website.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 953013
User impact if declined: some websites keep being broken
Testing completed (on m-c, etc.): not yet, about to land on m-c
Risk to taking this patch (and alternatives if risky): extremely low, just increases an execution limit
String or IDL/UUID changes made by this patch: none

Requesting this now because I'm going on vacation tomorrow.
An uplift needs to include the follow-up from comment 7.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/52577b51029a
https://hg.mozilla.org/mozilla-central/rev/5103fc7d8c39

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f939511edee

Comment 11

[moses]

No issues on 29/30 here.

Comment 12

[Gabriela [:gaby2300]]

Verified fixed using Windows 7 64 bit and Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0. Verified in latest Nightly as well.