Closed
Bug 979407
Opened 11 years ago
Closed 11 years ago
"ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute (and similar for negative markerWidth)
Categories
(Core :: SVG, defect)
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: jruderman, Assigned: longsonr)
References
Details
(Keywords: assertion, testcase)
Attachments
(4 files)
###!!! ASSERTION: viewport height must be nonnegative!: 'aViewportHeight >= 0', file content/svg/content/src/SVGContentUtils.cpp, line 295
Comment 1•11 years ago
|
||
This is likely because markerHeight is negative.
The SVG Spec says "A negative value is an error" for this attribute[1]. Maybe we're not honoring that?
[1] http://www.w3.org/TR/SVG11/painting.html#MarkerHeightAttribute
Summary: "ASSERTION: viewport height must be nonnegative" → "ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute
Comment 3•11 years ago
|
||
(Here's another testcase with simpler coordinates that also reproduces the bug.)
Comment 4•11 years ago
|
||
Note: Technically this puts the document "in error", which means we're only supposed to render the document up to the point of the error, per:
http://www.w3.org/TR/SVG11/implnote.html#ErrorProcessing
but I don't think we currently do that for other invalid attributes. I suspect it's probably OK to just reject it as a parse error.
Assuming we go that route:
SVGMarkerElement::ParseAttribute doesn't currently have any error checking for negative attributes:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGMarkerElement.cpp#208
It just ends up parsing the attribute using SVGMarkerElementBase::ParseAttribute -- which is really nsSVGElement::ParseAttribute -- which iterates over the entries in SVGMarkerElement::sLengthInfo and finds the attribute-name there and parses it as a length. (without any knowledge about the nonnegative requirement)
Maybe we should add a bitfield to "LengthInfo", with a flag that (if set) tells us that a given attribute explicitly excludes negative values?
Comment 5•11 years ago
|
||
Actually, disregard comment 4, I think -- documents can set this attribute from the DOM as well, and ParseAttribute tweaks wouldn't catch that. And the IDL shows that it's a SVGAnimatedLength:
http://www.w3.org/TR/SVG11/painting.html#InterfaceSVGMarkerElement
so it probably shouldn't have any extra magic parsing behavior beyond any other SVGAnimatedLength.
Maybe we just have to be graceful about handling (successfully-parsed) negative values internally.
Updated•11 years ago
|
Summary: "ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute → "ASSERTION: viewport height must be nonnegative", with negative 'markerHeight' attribute (and similar for negative markerWidth)
Comment 6•11 years ago
|
||
(This bug applies equally well to markerWidth. Here's a testcase, with markerWidth="-1px", which triggers:
###!!! ASSERTION: viewport width must be nonnegative!: 'aViewportWidth >= 0', file SVGContentUtils.cpp, line 294
)
Comment 7•11 years ago
|
||
This is happening in SVGMarkerElement::GetViewBoxTransform, BTW.
In the similar method for the <svg> element (SVGSVGElement::GetViewBoxTransform), we have this early-return:
> 657 if (viewportWidth <= 0.0f || viewportHeight <= 0.0f) {
> 658 return gfx::Matrix(0.0, 0.0, 0.0, 0.0, 0.0, 0.0); // singular
> 659 }
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/SVGSVGElement.cpp#657
We probably need the same early-return here.
Assignee | ||
Comment 8•11 years ago
|
||
Without an explicit viewBox we create a viewBox from the markerWidth and markerHeight. We then trap that in nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::GetMarkBBoxContribution like so...
const nsSVGViewBoxRect viewBox = content->GetViewBoxRect();
if (viewBox.width <= 0.0f || viewBox.height <= 0.0f) {
return bbox;
}
However if we do have a viewBox then we use that rather than the markerWidth and markerHeight and we don't check those things. What we need are calls to nsSVGMarkerElement::HasValidDimensions in nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::GetMarkBBoxContribution.
Assignee | ||
Comment 9•11 years ago
|
||
Assignee: nobody → longsonr
Attachment #8386021 -
Flags: review?(dholbert)
Comment 10•11 years ago
|
||
Comment on attachment 8386021 [details] [diff] [review]
marker.txt
Thanks!
Attachment #8386021 -
Flags: review?(dholbert) → review+
Comment 11•11 years ago
|
||
Comment on attachment 8386021 [details] [diff] [review]
marker.txt
Oh, one nit:
>+++ b/layout/svg/crashtests/crashtests.list
>@@ -175,8 +175,10 @@ load 893510-1.svg
> load 974746-1.svg
>+load 979407-1.svg
>+load 979407-2.svg
>\ No newline at end of file
Please add a newline at the end of this file.
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Flags: in-testsuite+
Comment 14•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•