Yet another safari bug

written by Tobie on April 18th, 2007 @ 02:44 PM

This one's pretty annoying:

/(.)+/.test(string);

This crashes Safari when string's length is bigger than about 7000 characters (your mileage may vary, it looks like the exact size is machine dependent, but its in that area on an intel MacBook).

The cause of the crash seems to be the length of the grouping results, as this works fine:

/.+/.test(string);

UPDATE: Non-capturing parentheses do not solve this bug. So this:

/(?:.)+/.test(string);

will still crash Safari.

This bug has some really annoying consequences. While we managed to fix some of them in the Prototype framework, it still leaves the issue of JSON sanitizing.

Currently, this is done both in Prototype and in Douglas Crockford's original implementation, (on which Prototype is mapped) by the following regex:

/^("(\.|[^"\\n\r])*?"|[,:{}[]0-9.\-+Eaeflnr-u \n\r\t])+?$/

which unfortunately chokes on long strings.

The only solution I can see to this problem right now is to use Prototype's implementation without the sanitizing parameter, e.g.:

string.evalJSON(); // <-- this will work
string.evalJSON(true); // <-- this won't!

Any brilliant suggestion is of course welcomed.

UPDATE: I forgot to mention that this was fixed in WebKit.

UPDATE: I've added a small testcase.

UPDATE: There's a follow-up more directly targeted at the JSON issue here.

posted in:

Bytes & Pieces

tags:

bug, safari

7 comments

Comments

  • Andy Armstrong
    Andy Armstrong says:

    I can’t reproduce this on my PowerBook with a 20k+ string so I can’t test it to be sure but I wonder if using non-capturing grouping might fix the problem? I’m assuming that you’re not actually interested in capturing anything.

    Thu, April 19 at 09:08 AM

  • Andy Armstrong
    Andy Armstrong says:

    OK, managed to reproduce it now. Of course it only happens with valid JSON. Sorry about that.

    I think you can work around the problem by making the clause that checks for valid string characters loop over runs of valid characters like this:

    /^(?:"(?:\.|[^"\\n\r]+)*"|[,:{}[]0-9.\-+Eaeflnr-u \n\r\t])+$/

    The crucial change is

    [^"\\n\r] ---> [^"\\n\r]+

    The bug’s still there of course – this just makes it less likely to bite with typical JSON data.

    Here’s a simple test script that crashes Safari with the old version, works fine with the new version.

    Thu, April 19 at 09:29 AM

  • Ash Searle
    Ash Searle says:
    I wonder if this is the same bug that causes string.replace to fail in some of my own code. I ended up excluding Safari and leaving a snarky comment for myself: 
    // FIXME: Safari's throwing-up randomly during string.replace with
// a function... so we're not syntax-highlighting on Safari till
// WebKit's fixed.

    Anyone any idea of the average time between a WebKit fix and a Safari release containing it?

    Thu, April 19 at 10:38 AM

  • Andy Armstrong
    Andy Armstrong says:

    It may well be possible to refactor the RE to work round the problem Ash. From the stack backtrace Safari throws I’d say the problem is excessive recursion – something that afflicts most current RE implementations to some extent. Obviously the fact that the browser crashes is a major bug.

    If you can rewrite the RE so it involves less backtracking you may be able to at least avoid the problem.

    If you’re really stuck send me your RE and I’ll have a look at it.

    Thu, April 19 at 10:59 AM

  • Tobie Langel
    Tobie Langel says:

    Andy,

    Thanks for your work, I’ll look into it ASAP.

    And unfortunately, as you have noticed, using non-capturing parentheses like so:

    /(?:.)+/.test(string);

    does not change anything… i.e. Safari still crashes.

    Thu, April 19 at 13:38 PM

  • Tobie Langel
    Tobie Langel says:

    Ash:

    Your problem is probably related (if your regex uses capturing parentheses it definitely is).

    Regarding WebKit’s next release, I guess it’s targeted with the release of Leopard… i.e. this fall.

    Thu, April 19 at 13:43 PM

  • Ash Searle
    Ash Searle says:

    Thanks guys. My regex is built dynamically and it’s massive (bound to lead to lots of backtracking.) A single regex to do the work of a token parser (built on work done by Dean Edwards and Dan Webb)

    I wish Apple would update Safari a bit more frequently. Even if the Leopard release is fantastic, it’ll take ages waiting for people to upgrade before you can treat this bug as fixed. (in the meantime, we get 78GB QuickTime and iTunes fixes every few weeks. How useful.)

    Fri, April 20 at 06:20 AM

Comments are closed