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

RESOLVED FIXED in mozilla11

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: dholbert)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla11
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 581015 [details]
testcase

###!!! ASSERTION: viewBox width must be greater than zero!: 'aViewboxWidth > 0', file layout/svg/base/src/nsSVGUtils.cpp, line 766
(Assignee)

Comment 1

6 years ago
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)
(Assignee)

Comment 2

6 years ago
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
(Assignee)

Comment 3

6 years ago
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 "||".
Blocks: 633337
(Assignee)

Comment 4

6 years ago
Created attachment 581105 [details] [diff] [review]
fix v1 with crashtests
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #581105 - Flags: review?(longsonr)
(Assignee)

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
(Assignee)

Comment 5

6 years ago
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.
Attachment #581105 - Attachment is obsolete: true
Attachment #581105 - Flags: review?(longsonr)
Attachment #581136 - Flags: review?
(Assignee)

Comment 6

6 years ago
(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.)

Updated

6 years ago
Attachment #581136 - Flags: review? → review+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/mozilla-central/rev/7604f82c29d0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.