Skip to main content

Bug Tracker

Side navigation

#4150 closed bug (fixed)

Opened February 14, 2009 04:55AM UTC

Closed December 20, 2009 05:45AM UTC

Last modified March 14, 2012 03:03PM UTC

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

Reported by: JosephPecoraro Owned by:
Priority: major Milestone: 1.4
Component: core Version: 1.4a2
Keywords: right pad spaces Cc: joepeck02@gmail.com
Blocked by: Blocking:
Description

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!

Attachments (7)
Change History (10)

Changed February 14, 2009 05:03AM UTC by JosephPecoraro comment:1

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.

Changed February 14, 2009 05:05AM UTC by JosephPecoraro comment:2

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

Changed February 14, 2009 02:44PM UTC by Rwhitbeck comment:3

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.

Changed February 14, 2009 07:02PM UTC by dmethvin comment:4

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.

Changed February 14, 2009 08:02PM UTC by JosephPecoraro comment:5

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.

Changed February 14, 2009 08:59PM UTC by JosephPecoraro comment:6

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.

Changed February 26, 2009 05:47PM UTC by JosephPecoraro comment:7

Any news?

Changed April 07, 2009 05:16AM UTC by JosephPecoraro comment:8

Take 2. Any news?

Changed August 09, 2009 01:14AM UTC by dmethvin comment:9

component: unfilledcore
milestone: 1.3.21.3.3
version: 1.3.11.3.2

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

Changed December 20, 2009 05:45AM UTC by john comment:10

resolution: → fixed
status: newclosed
version: 1.3.21.4a2

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.