Skip to main content

Bug Tracker

Side navigation

#1331 closed bug (fixed)

Opened June 28, 2007 02:42PM UTC

Closed July 06, 2007 01:39PM UTC

Last modified March 14, 2012 04:42AM UTC

Latest Nightlies Crash Safari 1.3.2

Reported by: jhenry Owned by:
Priority: critical Milestone: 1.1.4
Component: core Version: 1.1.3
Keywords: Cc:
Blocked by: Blocking:
Description

Version 1.1.2 loads correctly in Safari 1.3.2, but version 1.1.3a and the current nightly builds are crashing Safari 1.3.2 (browser exits) on load with a page as simple as

<html><head><script type="text/javascript" src="jquery.js"></script></head></html>
Attachments (2)
Change History (32)

Changed July 02, 2007 08:09PM UTC by jhenry comment:1

I know that this will be very hard to diagnose without an OS10.3 machine on which to test (since Apple doesn't allow Safari 1.3.2 to run on OS10.4). If no one has an installation of OS10.3, let me know and I'll see if I can come up with a way to test the engine.

Changed July 02, 2007 08:13PM UTC by john comment:2

version: → 1.1.3

I don't have a 10.3 machine - nor do I know anyone who has one. Anything that you can do to break it down will help us. The ideal situation would be one where you can say "I can comment out this line and the problem is now fixed."

Changed July 02, 2007 08:22PM UTC by jhenry comment:3

I'm not familiar with the jquery codebase, so I'm not sure where I'd start commenting out lines. Could you give me a few svn revision numbers to check out and build during the 1.1.2 to 1.1.3a timeline so I can determine which check-in broke the tree for Safari 1.3.2?

Changed July 02, 2007 09:49PM UTC by john comment:4

Well, 1.1.2 was 1465 and 1.1.3a was 1938 - I guess maybe taking a look at every 100 rev or so would be a start, we could drill down from there. You could also do a binary search - split the revisions in half (start at 1700 or so) and if it's broken, go down revision, if it's not broken, go up.

Changed July 02, 2007 10:54PM UTC by jhenry comment:5

I started doing some tests on this, and I found that using the uncompressed version of jquery does not crash safari 1.3.2. It's only the packed version that is causing the error. I will find the revision at which the packer starts causing safari to crash.

Changed July 02, 2007 11:08PM UTC by jhenry comment:6

This is the revision that causes the packed code to crash safari:

http://dev.jquery.com/changeset/1595

1594 works.

I'm taking a look at the packer to see if there's a good reason for this to happen on those strings.

Changed July 02, 2007 11:44PM UTC by jhenry comment:7

Applying this diff makes safari 1.3.2 not crash (however, nothing works correctly because all the selectors are broken):

Index: src/selector/selector.js
===================================================================
--- src/selector/selector.js    (revision 2214)
+++ src/selector/selector.js    (working copy)
@@ -61,9 +61,11 @@
                // Match: :contains('foo')
                /^(:)([\\w-]+)\\("?'?(.*?(\\(.*?\\))?[^(]*?)"?'?\\)/,
 
+               jQuery.chars = "(?:[\\\\w\\u0128-\\uFFFF*_-]|\\\\\\\\.)"
+
                // Match: :even, :last-chlid, #id, .class
                new RegExp("^([:.#]*)(" + 
-                       ( jQuery.chars = "(?:[\\\\w\\u0128-\\uFFFF*_-]|\\\\\\\\.)" ) + "+)")
+                       ( jQuery.chars ) + "+)")
        ],
 
        multiFilter: function( expr, elems, not ) {

If I simply replace all instances of jQuery.chars with the string that is assigned to it in the line above, jQuery loads in Firefox (and works) but crashes Safari 1.3.2.

Changed July 02, 2007 11:58PM UTC by jhenry comment:8

I accidentally omitted a semicolon in the above diff. Adding the semicolon causes safari to crash and everything to work again, so it isn't the placement of that assignment inside the string concatenation that is causing the crash. It is the packing of something in that string (or so it appears).

Changed July 03, 2007 12:07AM UTC by jhenry comment:9

I'm sorry for all the misleading comments on this ticket. I made a mistake during one of the first builds and copied the unpacked jquery.js from the source directory rather than the dist directory and thus assumed that the file was not crashing safari when it was unpacked in my simple test case of loading the library.

This is not the case. It crashes it compressed or uncompressed. The revision where the bug appears is still correct.

Changed July 03, 2007 12:20AM UTC by bcherne comment:10

@jhenry: Please confirm this on unpacked version (from jresig's original 1.1.3 release email)... on line 849, if you remove (?:[\\\\w\\u0128-\\uFFFF*_-]|\\\\\\\\.) then Safari 1.3.2 does not crash.

Changed July 03, 2007 12:25AM UTC by bcherne comment:11

More specifically, if I remove "\\u0128-\\uFFFF" from line 849, then Safari 1.3.2 does not crash when loading jquery-1.1.3.js. I don't understand enough about regular expressions or what this code does to recommend changes... hopefully this is enough...

Changed July 03, 2007 01:59AM UTC by bcherne comment:12

Possible solution!

@jhenry, please confirm....

If you escape the slashes (so it becomes "\\\\u0128-\\\\uFFFF") then Safari 1.3.2 does not crash, AND it successfully parses IDs and classes and what not. However, Safari 2.0.4 does not understand this... it requires the single slash.

\\\\u0128-\\\\uFFFF works in:

Safari 1.3.2

MacFF2, WinFF2

WinIE6, WinIE7

\\u0128-\\uFFFF works in:

Safari 2.0.4

MacFF2, WinFF2

WinIE6, WinIE7

I'll let you guys figure out how to implement it... I don't want to have all the fun. :) Also note that "\\u0128\\uFFFF" (sans hyphen) does not crash Safari 1.3.2... but obviously does not have the intended result either. :)

Changed July 03, 2007 03:00AM UTC by jhenry comment:13

The real problem is that Safari 2.0.4 doesn't understand \\\\u0128 as a unicode character. Safari 2.0.4 requires \\u0128 to match in a regular expression. This can be verified using the following test:

\\u0041 == A: <span id="jsUnicodeResults2">Test Not Run</span><script type="text/javascript">document.getElementById("jsUnicodeResults2").innerHTML = ("\\u0041" == "A");</script><br />
Regex "\\u0041" matches "A": <span id="jsUnicodeResults4">Test Not Run</span><script type="text/javascript">var jsReTest2 = new RegExp("\\u0041");document.getElementById("jsUnicodeResults4").innerHTML = (jsReTest2.exec("A") != null);

It would seem to me that doing browser version detection to slot in the appropriate character codes for Safari 2.0.4 would solve the problem, but it has to detect for Safari 2.0.4 rather than the others.

Changed July 03, 2007 03:08PM UTC by jhenry comment:14

On getting in to work this morning and checking on my 10.3 box, I found that using the escaped slash in the sequence \\\\u0128-\\\\u0FFF doesn't crash safari 1.3.2, but it doesn't really work either (none of the selectors seem to be working to allow jquery to operate).

The closest I can get to working in Safari 1.3.2 by changing that one line of code is to remove the unicode sequences.

So code for a workaround might be:


if(safari) {
  if(version < 2) {
    unicode = "";
  }
  else {
    unicode = "\\u0128-\\u0FFF";
  }
}
else {
  unicode = "\\\\u0128-\\\\u0FFF";
}

jQuery.chars = "(?:[\\\\w" + unicode + "*_-]|\\\\\\\\.)";

Changed July 03, 2007 03:30PM UTC by bcherne comment:15

I tested Safari 1.3.2 yesterday with some simple selectors (#foo, .bar, :even) and those seemed to be working with the \\\\u0128-\\\\u0FFF code. Is there a test suite I should be using instead?

Changed July 03, 2007 04:46PM UTC by jhenry comment:16

I was just doing quick tests by dropping the modified jquery into my development site and one of the applications on the page was a tabber. All the divs in the tabber were layered on top of each other in Safari 1.3.2 with the \\\\u0128-\\\\u0FFF code. With the unicode removed, the visibility on each of the divs was set correctly.

Changed July 03, 2007 06:37PM UTC by bcherne comment:17

Darn! I knew it was too good to be true. My simple tests were too simple... they work even when I remove unicode characters!

What do lines 848 and 849 do? The "Match :even, :last-child, #id, .class" comment doesn't seem to encapsulate what that line really does, or am I missing something? The other way of asking this is: if we remove \\u0128-\\u0FFF what should break?

Obviously getting Safari 1.3.2 to not crash is a priority (especially now that this is an official release and out in the wild). But breaking Safari 1.3.2 in some way (by removing the unicode) needs to be noted. .... and I'm still hoping we can find a work around ....

Changed July 03, 2007 07:08PM UTC by bcherne comment:18

These folks have run into the same problem: http://dev.rubyonrails.org/ticket/8332

Their solution was to replace "\\u0128-\\u0FFF" with "\\\\s\\\\S" ... the comment for their fix was "Replaced Unicode range with another any-character character class." I made the change in my local jquery-1.1.3.js, Safari 1.3.2 does not crash and jQuery seems to work.

Is "\\u0128-\\u0FFF" synonymous with "\\\\s\\\\S" ... are we losing/gaining anything? How can I test to make sure this pattern still works as intended?

Changed July 03, 2007 07:34PM UTC by jhenry comment:19

Browsing through other project repositories, I've found definitive information that unicode ranges in Safari 1.3.2 will never work.

http://webkit.org/blog/32/webkit-fixes-in-safari-202-mac-os-x-1043/

Another project was reporting a similar trouble with safari crashing. They wanted to match any character and they chose to use the combination of spaces and not spaces \\s\\S. We can use "not spaces" here to accomplish the same thing.

http://dev.rubyonrails.org/attachment/ticket/8332/scriptfragment-no-unicode.diff

The code will then read something similar to:


jQuery.chars = "(?:[\\\\S&&[^DO_NOT_MATCH_THIS]]|\\\\\\\\.)";

I've confirmed that new RegExp("\\\\S").exec("\\u0129") returns true in safari which means that \\u0129 is matching as not a space.

There are obviously characters that should explicitly not be matched, and those should be negated in the intersection of the regexp. John: can you weigh in here on what this pattern should *not* match so we can build an analogue without the unicode patterns? If we cannot do it without using unicode patterns, we should simply remove unicode support for any build less than Safari 2.0.2 as they're not supported in the regular expression engine for that browser.

Changed July 03, 2007 07:36PM UTC by jhenry comment:20

bcherne: it appears as though we were working on similar lines at the same time. I just need to know what in the unicode range \\u0001 to \\u0128 we cannot match in this regular expression so that it can go in the intersection with \\S.

Changed July 04, 2007 03:22PM UTC by john comment:21

Thanks for doing all the hard work on this, jhenry - I think the ideal solution is just going to be falling back to a less-powerful regexp in Safari 1.3. Something like:

jQuery.browser.safari && jQuery.browser.version < "2.0.0" ? "\\\\w" : "Unicode Regexp"

I'll look into this later today, to see if I can find a better solution.

Changed July 04, 2007 04:15PM UTC by john comment:22

resolution: → fixed
status: newclosed

Fixed in SVN rev [2233]. jhenry, let me know if this works for you, I plan on pushing it live later today.

Changed July 05, 2007 04:57AM UTC by john comment:23

milestone: 1.1.31.1.4

Changed July 05, 2007 10:59AM UTC by Ru comment:24

resolution: fixed
status: closedreopened

Hi, I've retested this on OS X 10.3.x with Safari 1.2 and 1.3 and this bug is still open, it still crashes the browser :( - please let me know if you have any more luck with this or if you'd like me to test.

Changed July 05, 2007 01:43PM UTC by john comment:25

@Ru, jhrenry - No one that I know has a copy of Safari 1.3 to test with, so please any help is appreciated. I committed the fix that jhenry recommended - if it's still crashing, then it's not related to what jhenry mentioned.

Changed July 05, 2007 01:56PM UTC by jhenry comment:26

Safari uses an internal build number for the version number instead of something sane like "2.0.2". The build number for 2.0.2 is 416.12, so the test string should be something like:

jQuery.browser.safari && jQuery.browser.version < "416.12" ? "\\\\w" : "Unicode Regexp"

I will patch the version in svn and test it as soon as I have access to my 1.3.2 box.

Changed July 05, 2007 02:11PM UTC by Ru comment:27

@ john - Hey, I'm currently testing with browsercam and it reports the error. I'm trying to get remote access or physical access to the particular Mac build but its taking time :(

Changed July 05, 2007 03:21PM UTC by jhenry comment:28

Attached is a patch against r2248 verified working in Safari 1.3.2. Note that "\\\\w" isn't enough as the character groups must be the same as the other half of the regexp for things to work correctly.

Changed July 05, 2007 07:17PM UTC by jhenry comment:29

Incidentally, you do not need Safari 1.3.2 (and OS10.3.9) to test for this bug. You can test in safari 2.0.0 available at http://www.michelf.com/projects/multi-safari/ on a machine that has OS10.4 installed. (This is because the bug was fixed in 2.0.2)

Changed July 05, 2007 08:44PM UTC by john comment:30

@jhenry - I applied your patch (SVN rev [2253]) and loaded it up in Safari 2.0.0, but it still crashes. Can you confirm this?

Changed July 05, 2007 11:05PM UTC by jhenry comment:31

It's a problem with the revision number comparison. It's not crashing for me in 1.3.2 because it can interpret the revision number as a float (eg. xyz.w). The reason that it's crashing in 2.0.0 is because the revision is not a valid floating point number. It is 412.2.2. If you add a parseInt(), it works.

Attached is an updated patch that works with 2.0.0.

Changed July 06, 2007 01:39PM UTC by john comment:32

resolution: → fixed
status: reopenedclosed

Committed in SVN rev [2256] - works in Safari 2.0.0 for me - great work jhenry. I wanna try and fix up a couple other bugs then I may do a 1.1.3.2 release - or just go straight to 1.1.4.

In the meantime, you can grab the code from the jQuery nightlies:

http://code.jquery.com/nightlies/jquery-nightly.js