Last Comment Bug 691337 - 100% cpu svg file in tag <animateTransform> parameter "begin"
: 100% cpu svg file in tag <animateTransform> parameter "begin"
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: mozilla11
Assigned To: Brian Birtles (:birtles)
: Jet Villegas (:jet)
Depends on: 703992
  Show dependency treegraph
Reported: 2011-10-03 08:01 PDT by sec search
Modified: 2012-02-01 13:57 PST (History)
3 users (show)
bbirtles: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

svg0.svg (1.04 KB, image/svg+xml)
2011-10-03 08:01 PDT, sec search
no flags Details
Proposed patch v1a (14.54 KB, patch)
2011-11-13 19:14 PST, Brian Birtles (:birtles)
longsonr: review+
Details | Diff | Splinter Review

Description User image sec search 2011-10-03 08:01:25 PDT
Created attachment 564186 [details]

User Agent: Opera/9.80 (Windows NT 5.1; U; ru) Presto/2.9.168 Version/11.51

Steps to reproduce:

open .svg file in browser

Actual results:

100% cpu

Expected results:

browser should not load the CPU 100%
Comment 1 User image Robert Longson 2011-10-03 13:51:13 PDT
Looks like the begin time comes out as LL_MININT. This wraps around so that when you do 0 > LL_MININT and LL_MININT = 0 - LL_MININT. Perhaps SMIL needs to use LL_MININT + 1 in some places.
Comment 2 User image Brian Birtles (:birtles) 2011-11-13 19:14:48 PST
Created attachment 574220 [details] [diff] [review]
Proposed patch v1a

Here's a proposed patch to catch situations where the time we're generating overflows the range of nsSMILTime and just ignore the time.

I've also tweaked the parsing method to drop the isValid flag and just return early. (When I first wrote it I was working for a company whose coding standard discouraged early returns and I mistakenly applied that here.) I've also converted a few NS_ASSERTIONs to NS_ABORT_IF_FALSE.
Comment 3 User image Robert Longson 2011-11-13 23:47:12 PST
Comment on attachment 574220 [details] [diff] [review]
Proposed patch v1a

>-      if (sign != 0) {
>-        // sign has already been set
>-        isValid = false;
>-        break;
>-      }
>+      // check sign has not already been set
>+      if (sign != 0)
>+        return NS_ERROR_FAILURE;

I'd rather you kept the braces even for one line tests, and fortunately the coding style agrees with me :-)
Comment 4 User image Brian Birtles (:birtles) 2011-11-14 12:34:08 PST
(In reply to Robert Longson from comment #3)
> I'd rather you kept the braces even for one line tests, and fortunately the
> coding style agrees with me
> :-)

Fair enough. I was trying to match the existing style (which was written at a time where there was a specific exception to that rule for early returns. In fact, the coding style still gives an example of an un-braced early return under "Return from errors immediately" snippet #4).

I'm going to go ahead and break with the existing style and trust that we'll gradually add braces elsewhere (as well as replacing a number of uses of nsresult with bool).
Comment 5 User image Brian Birtles (:birtles) 2011-11-15 11:59:45 PST
Pushed to m-i:
Comment 6 User image Ed Morley [:emorley] 2011-11-16 03:14:38 PST

Note You need to log in before you can comment on or make changes to this bug.