Bug 26117 – REGRESSION (r37381-r37442) : Reproducible crash viewing an SVG

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Gavin Sherlock]

Steps to reproduce:

1.  Open AssemblyViewer.svg (see attachment)
2.  Click on test.svg
3.  Wait a few seconds

Expected Results:

You can load, pan, and zoom in

Actual results:

Crash.

Top of the crash log is:

0   com.apple.WebCore             	0x00f5f4a7 WebCore::EventHandler::updateMouseEventTargetNode(WebCore::Node*, WebCore::PlatformMouseEvent const&, bool) + 279
1   com.apple.WebCore             	0x00f5f883 WebCore::EventHandler::dispatchMouseEvent(WebCore::AtomicString const&, WebCore::Node*, bool, int, WebCore::PlatformMouseEvent const&, bool) + 67
2   com.apple.WebCore             	0x00f6431c WebCore::EventHandler::handleMouseMoveEvent(WebCore::PlatformMouseEvent const&, WebCore::HitTestResult*) + 1052
3   com.apple.WebCore             	0x00f64555 WebCore::EventHandler::mouseMoved(WebCore::PlatformMouseEvent const&) + 69
4   com.apple.WebCore             	0x00f68874 WebCore::EventHandler::mouseMoved(NSEvent*) + 228

[Gavin Sherlock]

Created attachment 30835 [details]
crash log from r44282

[Gavin Sherlock]

Created attachment 30836 [details]
Zip file containing required files

Unzip the archive, and open "Assembly Viewer.svg"

[Gavin Sherlock]

Note, this is a regression from Safari 3.2.1

[Gavin Sherlock]

My instructions to reproduce are slightly incorrect.  They should be:

1.  Open "Assembly Viewer.svg" (see attachment)
2.  Click on test.svg
3.  Mouse over the "-" button for zooming

It will crash a few seconds later

[Gavin Sherlock]

Bisect-builds says:

Works: r37381  Fails: r37442

[Gavin Sherlock]

Most suspicious change in that period is:

http://trac.webkit.org/changeset/37435

[Gavin Sherlock]

Adding cc for reviewer of possible patch that caused regression

[Gavin Sherlock]

Created attachment 30883 [details]
Reduction as far as I can get it

Mouse over the red dot for the crash

[Robin Qiu]

Created attachment 39906 [details]
patch to fix this bug

If there is a structure like this:
<g id="G">
 <use id="A" ... >
  <set> ... </set>
 </use>
</g>

<use id="B" xlink:href="#G"> 
</use>

In SVGUseElement.cpp:builtInstanceTree(), when building instance tree for <use>B, the <use>A will be handled twice, the result is that the instance tree is incorrect (more nodes than expected). In later process, on these unwanted nodes, associations with shadow tree is broken and this causes crash when they are referred.

[Oliver Hunt]

Eric, can you review this? -- i have no idea how <use> (or more or less any svg) works nowadays

[Eric Seidel (no email)]

Yes, I'll take a look later today.  Thanks for pointing it out to me.  Niko wrote this code, but I think I understand it well enough to review this.

[Eric Seidel (no email)]

Comment on attachment 39906 [details]
patch to fix this bug

The change looks OK... but does the test need to be this complicated?

[Eric Seidel (no email)]

Rather, why does the test need to use svg animation?  And even then why would it need to use a mouse-click to start the animation?

[Robin Qiu]

(In reply to comment #13)
> Rather, why does the test need to use svg animation?  And even then why would
> it need to use a mouse-click to start the animation?

Yes, The test case is not simplified, it's just copied from the test case uploaded by Gavin Sherlock. if need, I'll changed it.

[Robin Qiu]

Created attachment 39980 [details]
Modified test case

Eric, I updated the patch. the animation is not necessary, we only need a child for <use>, so, an empty <set> is OK.

In this new test case:
<1> If move mouse on it, it will crash.
<2> If dump the instance tree, it will show an unwanted <cycle> element (last line):
The instance tree of use element zoomplus:

     id = loupePlus  this = [object SVGElementInstance]  correspondingElement = [object SVGGElement]
          id = useRim  this = [object SVGElementInstance]  correspondingElement = [object SVGUseElement]
               id = rim  this = [object SVGElementInstance]  correspondingElement = [object SVGCircleElement]
               id = rim  this = [object SVGElementInstance]  correspondingElement = [object SVGCircleElement]  //this one is unwanted.

Which way do you think is better?

[Eric Seidel (no email)]

OK, I've looked again, I would really like Niko's comment here if possible. :)

[Nikolas Zimmermann]

Thanks Eric, for checking the patch. I am going to have a look at this in the next hours.

[Nikolas Zimmermann]

Comment on attachment 39980 [details]
Modified test case

Robin, thanks for getting started on fixing that bug.
Though the approach is not correct, I do wonder if you re-ran layout tests? No new tests failing?

The SVGElementInstance tree should be a 1:1 map of the normalized, referenced tree (aka. you expand the use elements to the content that actually get rendered -- our shadow tree).
I agree that the current behaviour is wrong, leading to the second wrong SVGCircleElement instance, though your new approach can't be right as well - where is the <set> element?
The SVGSetElement has to show up in the SVGElmeentInstance tree as child of the SVGCircleElement, that's not the case at the moment.

I suggest you look deeper - enable DUMP_INSTANCE_TREE and DUMP_SHADOW_TREE, and try to find out the real cause why this gets out of sync.
That said, r- for the patch, as the handleDeepUseReferencing case has to stay (see the Spec comment! :-).

If you need any further advice, feel free to contact me in private.

[Robin Qiu]

(In reply to comment #18)

I had checked the patch again and had not found any regression bugs. 
The <set> element is SMIL element and is ignored by the original code. 
Before I submit this patch, I had enabled DUMP_INSTANCE_TREE and DUMP_SHADOW_TREE to locate the bug, at first it will crash in dump function. After changing dump functions, I found that the instance tree and shadow is incorrect.
It's just a misktake in depth-first recursive call, not hard to fix.

[Nikolas Zimmermann]

Comment on attachment 39980 [details]
Modified test case

I've tested the patch, and indeed the problem is real and the fix is sound. Got on the wrong track because of some interessting (maybe dangerous) refcounting issues:

I hope Bugzilla displays the following paste correctly, I guess not, better copy to a texteditor and view it there :-)

<quote>
Dumping <use> instance tree:
SVGElementInstance this=0x1a3af180, (parentNode=defs, firstChild=#text, correspondingElement=g (0x1a3ab5e0), shadowTreeElement=0x1a3b0830, id=loupePlus)
Corresponding element is associated with 1 instance(s):
-> SVGElementInstance this=0x1a3af180, (refCount: 1, shadowTreeElement in document? 1)
 SVGElementInstance this=0x1a3b07f0, (parentNode=g, firstChild=#text, correspondingElement=use (0x8986000), shadowTreeElement=0x1a3af680, id=useRim)
 Corresponding element is associated with 1 instance(s):
  -> SVGElementInstance this=0x1a3b07f0, (refCount: 0, shadowTreeElement in document? 1)
   SVGElementInstance this=0x1a3b0550, (parentNode=defs, firstChild=null, correspondingElement=circle (0x1a3ab0c0), shadowTreeElement=0x1a3afdc0, id=rim)
   Corresponding element is associated with 2 instance(s):
    -> SVGElementInstance this=0x1a3aba10, (refCount: 1, shadowTreeElement in document? 1) <-------------------------------------------- HERE!
    -> SVGElementInstance this=0x1a3b0550, (refCount: 0, shadowTreeElement in document? 1)

Dumping <use> shadow tree markup:
<g xmlns="http://www.w3.org/2000/svg" transform="translate(300.000000, 300.000000)"><g id="loupePlus">
           <g id="useRim" fill="#e33c31"><circle id="rim" cx="0" cy="0" r="70"/></g>
       </g></g>
</quote>

I saw these different refcounts, and thought Robins patch may be the cause, though it's just like this in trunk. Someone needs to investigate who's holding the refcounts, etc. We definately have to check wheter we leak around SVGElementInstance objects and/or (even worse) shadow tree elements. I don't trust leak bots :-)

[WebKit Commit Bot]

Comment on attachment 39980 [details]
Modified test case

Clearing flags on attachment: 39980

Committed r49833: <http://trac.webkit.org/changeset/49833>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

*** Bug 24892 has been marked as a duplicate of this bug. ***