709920 - "ASSERTION: viewBox width must be greater than zero!"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: 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]]

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]]

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]]

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]]

Attachment: fix v1 with crashtests

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: 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]]

(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]]

https://hg.mozilla.org/mozilla-central/rev/7604f82c29d0