Bug 11221 - REGRESSION: iExploder crash due to style="cursor: url()"
Summary: REGRESSION: iExploder crash due to style="cursor: url()"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P1 Normal
Assignee: Nobody
URL:
Keywords: HasReduction, Regression
Depends on:
Blocks:
 
Reported: 2006-10-08 09:43 PDT by jonathanjohnsson
Modified: 2006-10-11 12:45 PDT (History)
1 user (show)

See Also:


Attachments
iExploder crasher (110.25 KB, text/html)
2006-10-08 09:44 PDT, jonathanjohnsson
no flags Details
Crash log (20.05 KB, text/plain)
2006-10-08 09:46 PDT, jonathanjohnsson
no flags Details
Reduction (will crash) (27 bytes, text/html)
2006-10-08 10:05 PDT, mitz
no flags Details
Potential fix (2.63 KB, patch)
2006-10-10 13:20 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Now with testcase (4.54 KB, patch)
2006-10-11 02:24 PDT, Rob Buis
mitz: review-
Details | Formatted Diff | Diff
Improved patch (5.32 KB, patch)
2006-10-11 10:58 PDT, Rob Buis
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jonathanjohnsson 2006-10-08 09:43:09 PDT
WebKit crashes when opening the attached page from disk. Bug 11202 is related to this bug, as its testcase was created by reducing this one. However, that reduction unfortunately doesn't show the same behaviour as this original page, so I create this other bug.
Comment 1 jonathanjohnsson 2006-10-08 09:44:18 PDT
Created attachment 10983 [details]
iExploder crasher

Download and open from disk to crash WebKit. It does here quite reliably on nightly r16878.
Comment 2 jonathanjohnsson 2006-10-08 09:46:45 PDT
Created attachment 10984 [details]
Crash log
Comment 3 mitz 2006-10-08 10:05:53 PDT
Created attachment 10985 [details]
Reduction (will crash)

<br style="cursor: url()">
Comment 4 Rob Buis 2006-10-10 13:20:29 PDT
Created attachment 11021 [details]
Potential fix

This patch changes the logic slightly so any cursor value with a cursor list (even if
it amounts to an empty list like in the reduction) gets processed inside the if (list) {}.
Should fix the bug and still passes the webkits tests.
Cheers,

Rob.
Comment 5 Rob Buis 2006-10-11 02:24:20 PDT
Created attachment 11031 [details]
Now with testcase

Added a testcase as suggested by Mitz.
Cheers,

Rob.
Comment 6 mitz 2006-10-11 05:28:45 PDT
Comment on attachment 11031 [details]
Now with testcase

+            list = new CSSValueList; 

This will allocate a new CSSValueList on every iteration through the loop, throwing away the previously allocated one.

I noticed that the current code leaks a CSSValueList in a couple of places. One of them this patch fixes, another one is here:

            if ((strict && !value) || (value && !(value->unit == Value::Operator && value->iValue == ',')))
                return false;

(So for example, "cursor: url(cursor.png) ex" will leak a CSSValueList).

             if (strict || coords.size() == 0) {

This code after this 'if' is insufficiently indented, please clean it up.

In WebCore/ChangeLog, please add a line noting the test that goes with your patch. The usual format is "Test: fast/css/invalid-cursor-property-crash.html", right after the bug summary.
Comment 7 Rob Buis 2006-10-11 10:30:52 PDT
Hi Mitz,

(In reply to comment #6)
> (From update of attachment 11031 [details] [edit])
> +            list = new CSSValueList; 

I clearly wasnt thinking :}
 
> This will allocate a new CSSValueList on every iteration through the loop,
> throwing away the previously allocated one.

See above :}
 
> I noticed that the current code leaks a CSSValueList in a couple of places. One
> of them this patch fixes, another one is here:
> 
>             if ((strict && !value) || (value && !(value->unit ==
> Value::Operator && value->iValue == ',')))
>                 return false;

Yep, the state after bug 6002 shows that there were some issues left in the code. I guess the review should have been better, and I should have also studied this (tricky) code better.

> (So for example, "cursor: url(cursor.png) ex" will leak a CSSValueList).
> 
>              if (strict || coords.size() == 0) {
> 
> This code after this 'if' is insufficiently indented, please clean it up.

Ok.

> In WebCore/ChangeLog, please add a line noting the test that goes with your
> patch. The usual format is "Test: fast/css/invalid-cursor-property-crash.html",
> right after the bug summary.

Ok. Will try to make a new patch asap.
Cheers,

Rob.
Comment 8 Rob Buis 2006-10-11 10:58:39 PDT
Created attachment 11032 [details]
Improved patch

This one should address Mitz' issues with the patch before this one.
Cheers,

Rob.
Comment 9 mitz 2006-10-11 11:14:23 PDT
Comment on attachment 11032 [details]
Improved patch

r=me
Comment 10 Rob Buis 2006-10-11 12:45:00 PDT
Landed in r16991.