Bug 26117 - REGRESSION (r37381-r37442) : Reproducible crash viewing an SVG
Summary: REGRESSION (r37381-r37442) : Reproducible crash viewing an SVG
Status: RESOLVED FIXED
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:
Keywords: Regression
: 24892 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-06-01 10:48 PDT by Gavin Sherlock
Modified: 2010-05-07 15:13 PDT (History)
8 users (show)

See Also:


Attachments
crash log from r44282 (26.71 KB, text/plain)
2009-06-01 10:49 PDT, Gavin Sherlock
no flags Details
Zip file containing required files (10.12 KB, application/zip)
2009-06-01 10:51 PDT, Gavin Sherlock
no flags Details
Reduction as far as I can get it (748 bytes, image/svg+xml)
2009-06-02 15:35 PDT, Gavin Sherlock
no flags Details
patch to fix this bug (4.94 KB, patch)
2009-09-22 00:32 PDT, Robin Qiu
no flags Details | Formatted Diff | Diff
Modified test case (5.65 KB, patch)
2009-09-23 01:18 PDT, Robin Qiu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Sherlock 2009-06-01 10:48:52 PDT
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
Comment 1 Gavin Sherlock 2009-06-01 10:49:31 PDT
Created attachment 30835 [details]
crash log from r44282
Comment 2 Gavin Sherlock 2009-06-01 10:51:01 PDT
Created attachment 30836 [details]
Zip file containing required files

Unzip the archive, and open "Assembly Viewer.svg"
Comment 3 Gavin Sherlock 2009-06-01 10:53:33 PDT
Note, this is a regression from Safari 3.2.1
Comment 4 Gavin Sherlock 2009-06-01 11:00:21 PDT
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
Comment 5 Gavin Sherlock 2009-06-01 11:01:18 PDT
Bisect-builds says:

Works: r37381  Fails: r37442
Comment 6 Gavin Sherlock 2009-06-01 11:09:35 PDT
Most suspicious change in that period is:

http://trac.webkit.org/changeset/37435
Comment 7 Gavin Sherlock 2009-06-01 17:20:18 PDT
Adding cc for reviewer of possible patch that caused regression
Comment 8 Gavin Sherlock 2009-06-02 15:35:25 PDT
Created attachment 30883 [details]
Reduction as far as I can get it

Mouse over the red dot for the crash
Comment 9 Robin Qiu 2009-09-22 00:32:52 PDT
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.
Comment 10 Oliver Hunt 2009-09-22 00:45:53 PDT
Eric, can you review this? -- i have no idea how <use> (or more or less any svg) works nowadays
Comment 11 Eric Seidel (no email) 2009-09-22 10:09:55 PDT
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.
Comment 12 Eric Seidel (no email) 2009-09-22 14:24:51 PDT
Comment on attachment 39906 [details]
patch to fix this bug

The change looks OK... but does the test need to be this complicated?
Comment 13 Eric Seidel (no email) 2009-09-22 14:25:18 PDT
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?
Comment 14 Robin Qiu 2009-09-22 18:53:42 PDT
(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.
Comment 15 Robin Qiu 2009-09-23 01:18:02 PDT
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?
Comment 16 Eric Seidel (no email) 2009-09-23 17:22:40 PDT
OK, I've looked again, I would really like Niko's comment here if possible. :)
Comment 17 Nikolas Zimmermann 2009-09-24 08:06:55 PDT
Thanks Eric, for checking the patch. I am going to have a look at this in the next hours.
Comment 18 Nikolas Zimmermann 2009-09-24 21:03:26 PDT
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.
Comment 19 Robin Qiu 2009-10-19 18:57:14 PDT
(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.
Comment 20 Nikolas Zimmermann 2009-10-19 19:37:08 PDT
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 :-)
Comment 21 WebKit Commit Bot 2009-10-19 20:19:29 PDT
Comment on attachment 39980 [details]
Modified test case

Clearing flags on attachment: 39980

Committed r49833: <http://trac.webkit.org/changeset/49833>
Comment 22 WebKit Commit Bot 2009-10-19 20:19:33 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Alexey Proskuryakov 2010-05-07 15:12:53 PDT
*** Bug 24892 has been marked as a duplicate of this bug. ***