Last Comment Bug 691337 - 100% cpu svg file in tag <animateTransform> parameter "begin"
: 100% cpu svg file in tag <animateTransform> parameter "begin"
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 7 Branch
: All All
: -- normal (vote)
: mozilla11
Assigned To: Brian Birtles (:birtles)
:
Mentors:
Depends on: 703992
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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 | Review

Description sec search 2011-10-03 08:01:25 PDT
Created attachment 564186 [details]
svg0.svg

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 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 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 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 https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide :-)
Comment 4 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
> https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide :-)

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 Brian Birtles (:birtles) 2011-11-15 11:59:45 PST
Pushed to m-i:
https://hg.mozilla.org/integration/mozilla-inbound/rev/36b19dca7f91
Comment 6 Ed Morley [:emorley] 2011-11-16 03:14:38 PST
https://hg.mozilla.org/mozilla-central/rev/36b19dca7f91

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