Summary: | Crash on sierpinski svg | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jonathanjohnsson | ||||||||
Component: | SVG | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED WORKSFORME | ||||||||||
Severity: | Normal | CC: | eric, rwlbuis, schenney, zimmermann | ||||||||
Priority: | P1 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
URL: | http://www.maa.org/joma/volume7/lane/Sierpinski.svg | ||||||||||
Attachments: |
|
Description
jonathanjohnsson
2008-05-09 10:56:41 PDT
Created attachment 21040 [details]
Crash log
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 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.
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. 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.
Created attachment 21482 The test results were to big for the previous patch, so I add them here seperately. Cheers, Rob. 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.
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. Looks like this crash fix got abandoned. :( 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. |