418206 - Some SVG files that appear to hang Mozilla

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

David Dailey reports on svg-developers that some of his SVG files cause FF3 beta 3 to crash (I assume he means hang). See:

http://www.mail-archive.com/[e-mail]/msg12783.html

These include:

http://srufaculty.sru.edu/david.dailey/svg/newstuff/path5.svg
http://srufaculty.sru.edu/david.dailey/svg/newstuff/path6.svg
http://srufaculty.sru.edu/david.dailey/svg/newstuff/path7.svg
(8 , 8a, 9 and 10 too)

Comment 1

[Takeshi Kurosawa :takenspc]

(In reply to comment #0)
> http://srufaculty.sru.edu/david.dailey/svg/newstuff/path5.svg

doesn't crash but script are very slow.
Its |svg| element specifies |viewBox="0,0,0,600"|.

http://www.w3.org/TR/SVG11/coords.html#ViewBoxAttribute says
> A negative value for <width> or <height> is an error (see Error processing). A value of zero disables rendering of the element.

but we renders that element as a huge box.
# Should I file a bug of this?

Comment 2

[Robert Longson [:longsonr]]

The viewBox issue is bug 385554 I think.

On top of that the testcase has the same problems as bug 374037. I.e. using appendChild rather than insertBefore.

The appendChild slowness is bug 233463. The testcases could be speeded up replacing appendChild by insertBefore and rewriting the loops to go backwards if necessary.

Also using (un)suspendRedraw in the javascript code would presumably help.

Comment 3

[Jonathan Watt [:jwatt]]

Actually, depending on how it's done, disabling rendering isn't enough. I think the main problem is that getCanvasTM returns bogus results and causes getBBox to return a huge rect. As a result the script iterates over a huge area, and hence the "hang". If it wasn't iterating over such a large area I think the other issues would have much, much less of an impact.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 5

[Robert Longson [:longsonr]]

Comment on attachment 304490 [details] [diff] [review]
patch

>-    NS_NewSVGRect(getter_AddRefs(vb), 0, 0, w, h);
>+  }
>+
>+  if (!vb || w < 0.0f || h < 0.0f) {
>+    NS_NewSVGRect(getter_AddRefs(vb), 0, 0, PR_MAX(w,0.0f), PR_MAX(h,0.0f));
>   }

Nit: space after commas.

>+    } else {
>+      NS_WARNING("we should propogate the fact that the viewBox is invalid");

That's not how you spell propagate ;-)

Note that this misspelling occurs 3 times within this patch.

r=longsonr with nits fixed.

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch addressing review comments

I should get an editor with a built-in spell checker. :-)

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 304500 [details] [diff] [review]
patch addressing review comments

Can we have a mochitest for this?

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

or a reftest.

Comment 9

[Jonathan Watt [:jwatt]]

Comment on attachment 304500 [details] [diff] [review]
patch addressing review comments

A crashtest would probably be best. Possibly some a Tsvg test since this is really a massive perf hit.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 304500 [details] [diff] [review]
patch addressing review comments

Please make sure to file those tests. a=beltzner

Comment 11

[Joe Drew (not getting mail)]

Closing this to clean up bugzilla. Please file follow-on bugs for tests needed.

Comment 12

[Jonathan Watt [:jwatt]]

Please don't go around closing bugs and suggesting someone files something about file up work. Things will fall through the cracks. If you're going to file this, file the follow up bug first.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

Filed bug 433063 for the tests.