Bug 63646 - Crash in SVGUseElement::updateContainerOffsets on <use> with no parent
Summary: Crash in SVGUseElement::updateContainerOffsets on <use> with no parent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-06-29 12:23 PDT by Tim Horton
Modified: 2011-07-24 19:17 PDT (History)
3 users (show)

See Also:


Attachments
test case (341 bytes, image/svg+xml)
2011-06-29 12:23 PDT, Tim Horton
no flags Details
Patch (3.18 KB, patch)
2011-07-04 14:48 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2011-07-24 18:33 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 Tim Horton 2011-06-29 12:23:45 PDT
Created attachment 99126 [details]
test case

The attached SVG crashes WebKit in SVGUseElement::updateContainerOffsets.

In a debug build, we get an ASSERTion failure saying that the <use> element doesn't have a parentNode.
Comment 1 Tim Horton 2011-06-29 12:24:15 PDT
<rdar://problem/9630764>
Comment 2 Nikolas Zimmermann 2011-06-30 01:31:00 PDT
> Created an attachment (id=99126) [details]
> test case
> 
> The attached SVG crashes WebKit in SVGUseElement::updateContainerOffsets.
Excellent catch. Tim, where have you been all the years? :-) Your work is highly appreciated.

> 
> In a debug build, we get an ASSERTion failure saying that the <use> element doesn't have a parentNode.
That's not correct, the <use> has a parentNode - it's shadow tree root element (living in the <use> renderer) lost its parent.

Do you feel like debugging? :-)
Comment 3 Tim Horton 2011-06-30 10:10:48 PDT
(In reply to comment #2)
> > Created an attachment (id=99126) [details] [details]
> > test case
> > 
> > The attached SVG crashes WebKit in SVGUseElement::updateContainerOffsets.
> Excellent catch. Tim, where have you been all the years? :-) Your work is highly appreciated.
> 
> > 
> > In a debug build, we get an ASSERTion failure saying that the <use> element doesn't have a parentNode.
> That's not correct, the <use> has a parentNode - it's shadow tree root element (living in the <use> renderer) lost its parent.

Alright, that makes a lot more sense.

> Do you feel like debugging? :-)

Now that you've explained it correctly, I think it'll be easier to figure it out, so sure! (I couldn't make my explanation make any sense given what was actually going on in the document, but I figured someone else would have some insight, and you did!)
Comment 4 Tim Horton 2011-06-30 17:19:46 PDT
Nope, I think this one is beyond me for now.

It's clear that it's because we're unlinking the middle use in the chain from its child content, but I can't follow exactly where we diverge from the case where the first use in the chain (the last one in the file) doesn't exist.

I don't know enough about the shadow tree and friends to track this one down yet.

Feels like we're just failing to detachInstance() or something, but I'm not sure where.
Comment 5 Rob Buis 2011-07-04 14:48:56 PDT
Created attachment 99658 [details]
Patch
Comment 6 Tim Horton 2011-07-05 09:55:19 PDT
(In reply to comment #5)
> Created an attachment (id=99658) [details]
> Patch

Rob's patch is certainly sufficient to fix the symptom; should we at least take this for now and figure out the root cause later?
Comment 7 Nikolas Zimmermann 2011-07-05 11:06:28 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=99658) [details] [details]
> > Patch
> 
> Rob's patch is certainly sufficient to fix the symptom; should we at least take this for now and figure out the root cause later?

Are we in hurry with a fix for this? If so yes, but I don't think so. We should rather find out if the null parentNode is "okay", or not. Rob?
Comment 8 Rob Buis 2011-07-24 18:33:03 PDT
Created attachment 101844 [details]
Patch
Comment 9 Rob Buis 2011-07-24 18:34:04 PDT
Hi Niko,

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > Created an attachment (id=99658) [details] [details] [details]
> > > Patch
> > 
> > Rob's patch is certainly sufficient to fix the symptom; should we at least take this for now and figure out the root cause later?
> 
> Are we in hurry with a fix for this? If so yes, but I don't think so. We should rather find out if the null parentNode is "okay", or not. Rob?

I uploaded a new patch, hopefully this fix is better and the ChangeLog should explain the problem.
Cheers,

Rob.
Comment 10 Rob Buis 2011-07-24 19:17:41 PDT
Landed in r91653.