Bug 18971 - Crash on sierpinski svg
Summary: Crash on sierpinski svg
Status: RESOLVED WORKSFORME
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Normal
Assignee: Nobody
URL: http://www.maa.org/joma/volume7/lane/...
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-09 10:56 PDT by jonathanjohnsson
Modified: 2012-01-18 12:25 PST (History)
4 users (show)

See Also:


Attachments
Crash log (86.87 KB, text/plain)
2008-05-09 10:58 PDT, jonathanjohnsson
no flags Details
First attempt (3.43 KB, patch)
2008-05-09 23:06 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff
Now with testcase (6.41 KB, patch)
2008-06-03 11:50 PDT, Rob Buis
darin: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jonathanjohnsson 2008-05-09 10:56:41 PDT
WebKit beachballs and then crashes, after clicking the link above.
Comment 1 jonathanjohnsson 2008-05-09 10:58:08 PDT
Created attachment 21040 [details]
Crash log
Comment 2 Rob Buis 2008-05-09 23:06:42 PDT
Created attachment 21053 [details]
First attempt

This is a quick preview patch. I think the RefPtr stuff could be improved, but the patch does not regress and works for the testcase. I am thinking of adding the
mentioned testcase as is to svg/custom, not sure if that is allowed.
Cheers,

Rob.
Comment 3 Darin Adler 2008-05-24 23:08:12 PDT
Comment on attachment 21053 [details]
First attempt

+            Node *cloneParentPtr = cloneParent.get();

There's no guaranteed that this node won't go away due to DOM mutation code. The local variable needs to be a RefPtr. And the result needs to be a PassRefPtr<Node>, not a raw Node* pointer.

The test case should be included in the patch, too.
Comment 4 Rob Buis 2008-05-24 23:35:36 PDT
Hi Darin,

(In reply to comment #3)
> (From update of attachment 21053 [details] [edit])
> +            Node *cloneParentPtr = cloneParent.get();
> 
> There's no guaranteed that this node won't go away due to DOM mutation code.
> The local variable needs to be a RefPtr. And the result needs to be a
> PassRefPtr<Node>, not a raw Node* pointer.
> 
> The test case should be included in the patch, too.

I would love to include it in the patch, but am worried about copyrights.
I mailed the owner, but got no reply. I'll create a similar testcase that also
causes a crash due to the broad and deep use hierarchy but represent some random
image instead of sierpinsky. I'll include that in the patch along with a fix for the above.
Cheers,

Rob.
Comment 5 Rob Buis 2008-06-03 11:50:38 PDT
Created attachment 21481 [details]
Now with testcase

I reworked the original svg a bit into a testcase. Also now PassRefPtr<Node> is used for the return value.
Cheers,

Rob.
Comment 6 Rob Buis 2008-06-03 11:53:35 PDT
Created attachment 21482


The test results were to big for the previous patch, so I add them here seperately.
Cheers,

Rob.
Comment 7 Darin Adler 2008-06-03 12:20:23 PDT
Comment on attachment 21481 [details]
Now with testcase

 647     Node *child = element->firstChild();
 648     while (child) {
 649         PassRefPtr<Node> newChild = expandUseElementsInShadowTree(child);
 650         if (newChild)
 651             child = newChild.get();
 652         else
 653             child = child->nextSibling();
 654     }

Why is it OK for child to not be a RefPtr? That looks wrong to me. It seems like it could be deallocated before calling expandUseElementsInShadowTree again.

Also newChild should just be a RefPtr, not a PassRefPtr.
Comment 8 eddie svärd 2008-10-23 22:17:48 PDT
The Sierpinski.svg site also make my Safari 4 developer preview 2 crash (run on Windows XP on a PC). The  browser closes unexpectedly at load up attempt.
 
Comment 9 Eric Seidel (no email) 2009-10-06 11:54:32 PDT
Looks like this crash fix got abandoned. :(
Comment 10 Stephen Chenney 2012-01-18 12:25:41 PST
The code in question has been modified since the patch was generated in ways that change the previous behavior. The test case, and indeed a recursive tree with three more levels, does not crash existing browsers. Marking it RESOLVED.