Regex breaking Firefox >3.6 #1514

Closed
epsd opened this Issue Apr 25, 2011 · 8 comments

Projects

None yet

4 participants

@epsd
epsd commented Apr 25, 2011

When using the desktop version of Firefox (versions below 4, i.e., 3.6) the following regex (from Alpha 4.1) is causing Firefox to stop working:

pageElemRegex = new RegExp(".(<[^>]+\bdata-" + $.mobile.ns + "role=["']?page["']?[^>]>).*"),

This only occurs on 'heavy' pages, i.e., pages that have a lot of elements to load, and script to work with. I tracked the problem back to the regex above, but I do not know what it does or how to fix it. Again, only seems to occur with Firefox 3.x.

@jblas
Contributor
jblas commented Apr 25, 2011

@epsd

Do you have a test case? That regex has been in for a while and I never noticed an error in FF 3.6.

When you say it stops working, is there an error thrown or printed in the console/error log?

Thanks,

  • Kin
@epsd epsd closed this Apr 25, 2011
@epsd
epsd commented Apr 25, 2011

Weird seems to only happen on my pre-intel mac, not on pc or on my intel mac... Only happens with firebug enabled too. I'll close for now unless I can replicate. Sorry for that.

@epsd epsd reopened this Apr 29, 2011
@epsd
epsd commented Apr 29, 2011

I'm gonna re-open this. I can confirm that the above regex absolutely kills ajax page load performance in Firefox 3.6. To test you need to have a pretty heavy page that you intend to load with ajax change page.

Try the code like this:

if(pageElemRegex.test(html) && RegExp.$1 && dataUrlRegex.test(RegExp.$1) && RegExp.$1) {

and then try the code like this:

if( RegExp.$1 && dataUrlRegex.test(RegExp.$1) && RegExp.$1) {

The 2nd one runs 10 times faster.

@pauln
Contributor
pauln commented May 1, 2011

If you change the declaration from:
pageElemRegex = new RegExp(".(<[^>]+\bdata-" + $.mobile.ns + "role=["']?page["']?[^>]>)."),
to:
pageElemRegex = new RegExp("(<[^>]+\bdata-" + $.mobile.ns + "role=["']?page["']?[^>]
>)"),

page loads speed up substantially (not just in Firefox, though that's where the difference seems most pronounced. Unless I'm missing something, I don't see any reason to be doing greedy matches for anything before and after the tag that's being looked for, as operations on the result only seem to use $1 (first matching group) - which doesn't change by removing the greedy matches.

A quick bit of Date()-based benchmarking showed an improvement from ~4.5 seconds to 3ms in Firefox 3.6. The stock Android browser on my Galaxy Tab went from ~3.6 seconds to 1ms.

Given that there appears to be no change in functionality (though perhaps somebody more familiar with that particular bit of the code could confirm this), this seems like a no-brainer - cutting out several seconds from the page load/render time can only be a good thing. If there's some particular reason for the greedy matching (such as some particular obscure mobile browser needing it there), perhaps the way forward is to detect that circumstance and only use the greedy matching if necessary?

@pauln pauln added a commit to pauln/jquery-mobile that referenced this issue May 2, 2011
@pauln pauln Issue #1514 (Regex breaking Firefox >3.6)
Remove greedy matches from start and end of regex - there's no need for them, and they cause immense slowdown (on the order of 3-4 seconds on medium-size pages loaded via ajax).
91126e4
@jblas
Contributor
jblas commented May 2, 2011

@MaxThrax

Thanks for the patch!

cff12a4

@jblas jblas closed this May 2, 2011
@Sumeruter

The patch is still not merged into the 1.0b3 or latest builds!

I just ran into this problem when I removed the surrounding <div data-role='page'> (according to the new simpler single page layout feature) and had to cancel the script execution after about 15 minutes.
I have read somewhere that a regex with greedy patterns will try to match every possible combination of chars before it fails, so every greedy part will multiply the problem. So my solution was too to remove the two greedy parts. After that, it took only seconds to finish.

I think there IS a difference with the changed pattern if the document contains multiple pages: the greedy one will match the last page, the less greedy (changed) one will match the first page. I think that would be also more conform with the docs.

@jblas
Contributor
jblas commented Sep 29, 2011

@Sumeruter

Hmmm, I did merge the pull request. Not sure what happened there. I'll re-merge it into the HEAD for 1.0rc1.

@jblas
Contributor
jblas commented Sep 29, 2011

@Sumeruter

Thanks for flagging this. I just re-merged it on to the HEAD:

972500b

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment