jQuery(str) has Performance/Crash issues with space right padded strings.

[JosephPecoraro]

General:

---

Passing a string into jQuery() containing html/xml that is right padded with spaces causes errors in Safari/Webkit and severe performance impacts on all other browsers I could test. As the number of spaces increases, the parse time grows exponentially, 2s 4s, 8s, 16s, etc. The string input must contain a <tag>. All browsers appear fine with left-padded text but fail with right-padded text.

Unknown and untested to see if this was a problem in past version of jQuery. I spent the time to write this report because I was really irked that Firefox kept crashing on me. I eventually figured it out and saw the problems all over.

Browsers Tested:

---

OS X Safari / Webkit:

• Throws "Syntax error, unrecognized expression: >" warning and stops script execution.

OS X Firefox 3:

• Poor performance on right pad.
• Hangs before displaying the page. May take 30s to display.

OS X Opera 9:

• Poor performance on right pad.
• Successfully loads the page then runs the scripts.

XP Firefox 2:

• Poor performance on right pad
• Hangs before displaying the page. May get a "long script" warning.

XP IE 6:

• Poor performance on right pad. Although better then Firefox!

Suggestions:

---

I don't know what to say here. I don't know enough about how jQuery works and the underling browser iefficiencies. I imagine this could be turned around into browser bug reports if that is the case. jQuery could force the developer to trim() text, but then again maybe the jQuery(str) function itself should trim its input. Would that have any serious impact on performance? How about just right-trimming? I'm out of my league here, so I'm passing it on to you.

Attached:

---

• Unit Test to show the problem
• Version of jQuery emitting the problem
• Pictures of my results on current browsers (listed above)

Please keep me updated. Thanks!

[JosephPecoraro]

Unit Test

[JosephPecoraro]

I have been a fan of jQuery for a while, but there has been some serious criticism lately that has peeked my interest. I spent a lot of time submitting this report, including wrestling with this trac system by writing and rewriting my bug report multiple times... then needing to uploading multiple files because of asinine limits...

I would appreciate it if this bug was handled in a way that can restore some of my confidence in the jQuery team and community. I hope this helps or gets moved in the right direction. Thank You.

[JosephPecoraro]

The Component is "core." It appears as though that got lost when I had to rewrite the report.

[Rwhitbeck]

JosephPecoraro, thanks for your effort in submitting this bug, it is very thorough. Well take a look at it and hopefully restore your confidence in jQuery again.

[dmethvin]

If every bug report had a thorough test case like this one does, they would be a lot easier to find and fix! Firebug said it was spending most of its time in jQuery.init. A little investigation from there showed it was this line:

   var match = quickExpr.exec( selector );

This is quickExpr:

quickExpr = /^[^<]*(<(.|\s)+>)[^>]*$|^#([\w-]+)$/,

Something about the regexp is probably causing the JS matcher to backtrack a LOT. The problem with the regexp isn't obvious to me, but at least we have it narrowed down to one line.

[JosephPecoraro]

Very cool guys. I checked it out and here are my thoughts:

Change the quickExpr regex to the following and see how the rest of jQuery's tests run:

 quickExpr = /^[^<]*(<(.|\n)+>)[^>]*$|^#([\w-]+)$/,

I changed the white-space matching portion on the optimized tag search portion.

 (.|\s) => (.|\n)

This resource says that "." matches every character except a new line:  http://www.evolt.org/article/Regular_Expressions_in_JavaScript/17/36435/

I don't know wether or not that is true across "all browsers". So clearly this change should be tested to see if it works everywhere... but check out the results:

1. Perfect Results and Excellent performance in Firefox (3 and 2), Opera 9, IE6
2. It corrects Webkit's random error... Although Safari 3.2 still throws the error...

Thats all positive!

I've uploaded a new version 2 of the UnitTest to test spaces and newlines and desired output.

[JosephPecoraro]

Newer tests to test newlines and desired output.

[JosephPecoraro]

Hmm, I'm not typically a Window's user (I just open VMWare to test IE6), but a thought occurred to me that "(.|\n)" wouldn't match "\r". I tested this in WebKit and Firefox and both returned null.

js> /(.|\n)/.exec("\r")       
null

So, if this is a viable change then it might be necessary to make the regex:

(.|[\n\r]) or (.|\n|\r)

Aside: Further tests showed that \r inside the content will throw errors in Webkit if the regex doesn't explicitly match \r. Note, this is specific to being "in the html content" because when you run it from the Developer Console it will work fine. It only throws the error if it was in the actual content that Webkit/Safari loaded. That just keeps getting more and more confusing! If I find enough time I'll submit a bug report to the Webkit team on that.

[JosephPecoraro]

Any news?

[JosephPecoraro]

Take 2. Any news?

[dmethvin]

This ticket got lost because it was lacking a component. Ressurecting for 1.3.3.

[john]

I just re-tested the attached test case and it's no longer a problem. We've since changed the quickExpr so I bet that helped. We now do [\w\W]+.

Going to mark this as resolved.