Ticket #11137 (closed bug: invalid)
Jquery doesn't parse a rare selector correctly
| Reported by: | fam.lam | Owned by: | fam.lam |
|---|---|---|---|
| Priority: | low | Milestone: | None |
| Component: | selector | Version: | 1.7.1 |
| Keywords: | Cc: | ||
| Blocking: | Blocked by: |
Description
If you execute this code (in Chrome, you can just paste it in the console), JQuery will hang for a very long time. (If you leave out the 'window.open', then you'll just get a hang for about 5-10 seconds).
var selector = "[onclick^*=\"window.open('http://www.firstload.de/\"]"
$(selector)
Change History
comment:2 Changed 17 months ago by fam.lam
Oh... sorry... although it is (I think?) a bug that such an invalid selector hangs jQuery so long, I hadn't noticed I introduced a bug in the selector myself...
Corrected reproduction steps:
- go to battle.net (uses an old version of jQuery, but also with a newer version (which you can insert by content scripts if you like) it fails the test
- in Chrome's console, insert
var selector = "[onclick^=\"window.open('http://www.firstload.de/\"]" jQuery(selector) - wait forever...
I've created a content script for you: Install http://solidfiles.com/d/b03088d9ff/ in Chrome and visit battle.net It'll first alert the jQuery version it uses (to prove it's not the old version), then run the code from step 2.
comment:3 Changed 17 months ago by fam.lam
It seems to be this code that hangs:
/((?:\((?:\([^()]+\)|[^()]+)+\)|\[(?:\[[^\[\]]*\]|['"][^'"]*['"]|[^\[\]'"]+)+\]|\\.|[^ >+~,(\[\\]+)+|[>+~])(\s*,\s*)?((?:.|\r|\n)*)/g.exec("[onclick^=\"window.open('http://www.firstload.de/\"]");
The actual line where this happens is
m = chunker.exec( soFar );
comment:4 Changed 17 months ago by sindresorhus
- Owner set to fam.lam
- Status changed from new to pending
Thanks for taking the time to contribute to the jQuery project! Please provide a complete reduced test case on jsFiddle to help us assess your ticket.
Additionally, be sure to test against the jQuery Edge version to ensure the issue still exists. To get you started, use this boilerplate: http://jsfiddle.net/FrKyN/ Open the link and click to "Fork" (in the top menu) to get started.
comment:5 follow-up: ↓ 7 Changed 17 months ago by fam.lam
- Status changed from pending to new
Even if I just copy-paste the full web page and required reproduction steps into jsfiddle (removed the comments and made the links absolute: http://jsfiddle.net/EWPLb/5/ and used the latest jQuery or the 1.7.1 release), I do not get any hangs. (Don't ask me why, the steps above cause it 100% of the time)
However, you might want to have a look at http://code.google.com/p/v8/issues/detail?id=1888 (The issue tracker for Chromes javascript kit V8). It explains exactly why it can hang in Chrome, but doesn't in Safari or FireFox: the regex too many repeating quantifiers, which may hang for quite a while. The regex proposed by that V8 programmer
/((?:\((?:\([^()]+\)|[^()])+\)|\[(?:\[[^\[\]]*\]|['"][^'"]*['"]|[^\[\]'"])+\]|\\.|[^ >+~,(\[\\])+|[>+~])(\s*,\s*)?((?:.|\r|\n)*)/g
indeed does the same thing. (Verified manually and I've also ran it through the jQuery test suite which told me there were no errors: Tests completed in 45812 milliseconds. 4919 tests of 4919 passed, 0 failed.
The only change is in the three +)+ blocks, which are (as explained by that programmer) not necessarily (the blocks are all non-capturing).
comment:6 Changed 17 months ago by fam.lam
Or do I have to report this to Sizzle?
This code seems to reproduce it 100% of the time (if Sizzle is in your current directory)?
<html><head><script src="sizzle.js"></script><script>
Sizzle("[onclick^*=\"window.open('http://www.firstload.de/\"]");
alert('Done');
</script></head><body></body></html>
comment:7 in reply to: ↑ 5 Changed 17 months ago by rwaldron
indeed does the same thing. (Verified manually and I've also ran it through the jQuery test suite which told me there were no errors: Tests completed in 45812 milliseconds. 4919 tests of 4919 passed, 0 failed.
Something isn't right, because jQuery should run:
Tests completed in 49876 milliseconds. 5876 tests of 5876 passed, 0 failed.
comment:8 Changed 17 months ago by fam.lam
Hmmm... Can't get it higher than 4919. I ran the index.html file in /test/ after
- downloading the full repository from https://github.com/jquery/jquery/downloads
- adding the missing files that showed up in index.html's console (qunit.js/css from http://docs.jquery.com/QUnit, the two sizzle files from sizzles repository)
- replace the incorrect regexes with the proposed fix
- run index.html -> 4919 tests...
What do I do wrong?
Also, should I report it to sizzle as it is reproducable 100% of the time for sizzle itself?
comment:10 Changed 17 months ago by fam.lam
Yes, I am. I do not have some server to put them on.
comment:11 Changed 17 months ago by rwaldron
You can easily run a localhost server, but that's irrelevant to this issue
comment:12 Changed 17 months ago by timmywil
- Priority changed from undecided to low
- Resolution set to invalid
- Status changed from new to closed
- Component changed from unfiled to selector
A few things here:
- If the bug cannot be reproduced on jsfiddle, it is usually not a jQuery issue but an issue with user's environment.
- We shouldn't spend too much time verifying selectors that use on* attributes, especially the onclick attribute.
- onclick^*=... is invalid. Either use ^= or *=, but not both. This could be the cause of the loop.
comment:13 Changed 17 months ago by fam.lam
You didn't read comment 2... Filed it to Sizzle itself, as it's 100% reproducible for them.
comment:14 Changed 17 months ago by fam.lam
Ok... have to apologize. Copied the wrong one again :(
comment:15 Changed 17 months ago by timmywil
Neither selector is valid, but the reason is less obvious for the selector in comment two. To escape any character, one must insert two backslashes before the character that needs to be escaped (such as parenthesis and periods):
var selector = '[onclick^="window\\.open\\(\'http:\\/\\/www\\.firstload\\.de\\/"]'
These are the same escaping rules that apply to querySelectorAll:
Please follow the bug reporting guidlines and use jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.
*in the console of a jQuery using page* of course, otherwise the console uses it's own $ selector, which does return null. Or replace the '$' with 'jQuery' if you want to be sure.