Last Comment Bug 709920 - "ASSERTION: viewBox width must be greater than zero!"
: "ASSERTION: viewBox width must be greater than zero!"
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on:
Blocks: 344905 633337
  Show dependency treegraph
 
Reported: 2011-12-12 12:37 PST by Jesse Ruderman
Modified: 2011-12-13 12:14 PST (History)
2 users (show)
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (170 bytes, image/svg+xml)
2011-12-12 12:37 PST, Jesse Ruderman
no flags Details
fix v1 with crashtests (2.16 KB, patch)
2011-12-12 16:54 PST, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
fix v2 with crashtests (3.30 KB, patch)
2011-12-12 18:15 PST, Daniel Holbert [:dholbert]
longsonr: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-12-12 12:37:32 PST
Created attachment 581015 [details]
testcase

###!!! ASSERTION: viewBox width must be greater than zero!: 'aViewboxWidth > 0', file layout/svg/base/src/nsSVGUtils.cpp, line 766
Comment 1 Daniel Holbert [:dholbert] 2011-12-12 16:15:37 PST
This testcase also triggers the following assertion (seems to be tied to the other one):
###!!! ASSERTION: Unknown value for meetOrSlice: 'Not Reached', file layout/svg/base/src/nsSVGUtils.cpp, line 835

From tweaking the attached testcase -- the assertions don't fire for...
>  <pattern id="test" viewBox="0 0 0 0">
but they do fire for...
>  <pattern id="test" viewBox="0 0 0 1">
and...
>  <pattern id="test" viewBox="0 0 1 0">
(with s/width/height/ in the assertion from comment 0)
Comment 2 Daniel Holbert [:dholbert] 2011-12-12 16:42:51 PST
so we're fine in the 0 0 0 0 case because we have this up one level, a few lines before the call to GetViewBoxTransform() (the function with the failing assertion):
>584   if (viewBox.height <= 0.0f && viewBox.width <= 0.0f) {
>585     return tCTM;
>586   }
http://mxr.mozilla.org/mozilla-central/source/layout/svg/base/src/nsSVGPatternFrame.cpp#584

I'm pretty sure we want an "||" there instead of "&&".  Note that in the equivalent spot in nsSVGSVGElement.cpp, we use "||":
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#1014
...and nsSVGMarkerFrame has an assertion to the same effect (that both the viewbox width & height must be positive):
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGMarkerElement.cpp#384
Comment 3 Daniel Holbert [:dholbert] 2011-12-12 16:49:56 PST
Line 584 quoted above is from http://hg.mozilla.org/mozilla-central/rev/4ff3dfaadd48 , and I think this is effectively a regression from that. (bug 633337)

It looks like that cset replaced this structure:

>  if (viewBox.height > 0.0f && viewBox.width > 0.0f) {
>    // [do the stuff that assumes nonzero size]
>  }

with this structure:

>   if (viewBox.height <= 0.0f && viewBox.width <= 0.0f) {
>     return tCTM;
>   }
>   // do the stuff that assumes nonzero size

and in flipping the logic, we forgot to flip the "&&" to an "||".
Comment 4 Daniel Holbert [:dholbert] 2011-12-12 16:54:07 PST
Created attachment 581105 [details] [diff] [review]
fix v1 with crashtests
Comment 5 Daniel Holbert [:dholbert] 2011-12-12 18:15:50 PST
Created attachment 581136 [details] [diff] [review]
fix v2 with crashtests

Turns out the crashtests in the previous patch wouldn't reliably fail, because we skip over the test before the problem code (PaintPattern / ConstructCTM) is ever triggered.

I added reftest-wait MozReftestInvalidate, but even then, we'd still skip over the test without ever triggering the bad code.

So -- this new patch uses MozReftestInvalidate *plus* a 100ms timeout.  With that, the tests reliably fail for me.
Comment 6 Daniel Holbert [:dholbert] 2011-12-12 18:17:06 PST
(In reply to Daniel Holbert [:dholbert] from comment #5)
> I added reftest-wait MozReftestInvalidate, but even then, we'd still skip
> over the test without ever triggering the bad code.

(btw, I verified this by adding debugging printf's in PaintPattern / ConstructCTM, and those printfs were never triggered during crashtest-runs until I added the setTimeouts.)
Comment 7 Daniel Holbert [:dholbert] 2011-12-13 12:14:01 PST
https://hg.mozilla.org/mozilla-central/rev/7604f82c29d0

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