Bug 18971 – Crash on sierpinski svg

• Component: SVG
• Status: RESOLVED WORKSFORME

WebKit beachballs and then crashes, after clicking the link above.

Created attachment 21040 [details]
Crash log

[Rob Buis]

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.

[Darin Adler]

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.

[Rob Buis]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 21482


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

Rob.

[Darin Adler]

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.

[eddie svärd]

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.
 

[Eric Seidel (no email)]

Looks like this crash fix got abandoned. :(

[Stephen Chenney]

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.