Created attachment 269478 [details] testcase This testcase produces one of the following assertions: ###!!! ASSERTION: Unknown value for meetOrSlice: 'Not Reached', file nsSVGUtils.cpp, line 893 ###!!! ASSERTION: XXX. We shouldn't get here. Viewbox width/height is set to 0. Need to disable display of element as per specs.: 'Error', file nsSVGSVGElement.cpp, line 1265 Which assertion it triggers is random. The latter assertion also shows up in bug 373960.
Created attachment 289860 [details] testcase2 I'm seeing this assertion in current debug trunk build with this testcase. An svg and foreignObject element inside a xul window.
Also, the viewbox assertions spew forth when running through reftests on the Mac with a debug trunk build.
I don't see the meetOrSlice assertion.
The XXX assertion fires when the <layout/svg/crashtests/348982-1.xhtml> crashtest is loaded.
Created attachment 304038 [details] [diff] [review] Patch Patch. Returning a zero matrix may cause compatibility problems. Should we return an identity matrix? # Returning a matrix at |GetViewboxToViewportTransform| may be better?
Assignee: nobody → taken.spc
Status: NEW → ASSIGNED
Attachment #304038 - Flags: review?(longsonr)
Created attachment 304040 [details] [diff] [review] Patch (typo fixed) Previous patch has a typo :-(.
This seems to be wallpapering over the cracks. Perhaps jwatt has some ideas on whether there is something better we can do to avoid calling this code in the first place if we have a viewBox with a 0 width or height.
I still have my 150 KB WIP patch for bug 374111, but that's too big this late in the game (and considerably bit rotted). I actually think patching things up along these lines is fine. The problem is that if someone uses getAttribute they need to get the viewBox value they set, so we have to carry these invalid values internally, meaning we have to check for validity when we access them. I would note a few things though. Returning a matrix full of zeros is too dangerous this close to a final release. E.g. cairo could go into an error state if it sees such a matrix. I think it would be better to have GetViewboxToViewportTransform return error codes for "no viewBox" and "invalid viewBox". Perhaps abuse some of the nsresult values: NS_ERROR_NOT_AVAILABLE - "no viewBox" NS_ERROR_INVALID_ARG - "invalid viewBox" You'd then need to put in checks at all six GetViewboxToViewportTransform call sites, not just the ones you have in the patch. The checks in nsSVGPatternFrame::ConstructCTM should be for a viewBox width and height that is greater than (not equal to) zero. In nsSVGUtils::GetViewBoxTransform I think we should just assert that aViewBoxWidth and aViewBoxHeight are both greater than zero. With the changes mentioned above there would be no existing code paths that can pass in such invalid values. We should check the viewBox width and height in: nsSVGSVGElement::GetCtxRect nsSVGSVGElement::GetLength nsSVGMarkerFrame::GetCanvasTM nsSVGMarkerFrame::PaintMark nsSVGOuterSVGFrame::GetIntrinsicRatio
It's fairly common in the code that we return NS_ERROR_FAILURE for out of range things. I'd suggest we return NS_ERROR_FAILURE for invalid viewbox i.e. with width/height <= 0 Do we need to know if there is no viewBox outside the GetViewboxToViewportTransform returning NS_OK would seem the most logical thing to do. > We should check the viewBox width and height in:... So we probably want a nsSVGUtils::IsRenderingDisabled or something which would return PR_TRUE if the element had a viewbox with a width/height <= 0.
Comment on attachment 304040 [details] [diff] [review] Patch (typo fixed) Hopefully you can adapt this based on comment 8 and comment 9
Attachment #304040 - Flags: review?(longsonr) → review-
Okay, yeah. Robert's suggestion of just returning NS_ERROR_FAILURE and having callers assume that means the viewBox is invalid seems fine now that I look closer. I guess we don't need to know whether there's no viewBox at all, as long as we return the identity matrix. As for nsSVGUtils::IsRenderingDisabled, I don't think that's necessary. The only place we don't actually already obtain the viewBox width and height is nsSVGSVGElement::GetCtxRect, so it seems overkill just for that one function. I think we can just have an |if| statement in the required locations.
(In reply to comment #11) > As for nsSVGUtils::IsRenderingDisabled, I don't think that's necessary. The > only place we don't actually already obtain the viewBox width and height is > nsSVGSVGElement::GetCtxRect, so it seems overkill just for that one function. I > think we can just have an |if| statement in the required locations. > Agreed.
taken, are you okay to do this soon?
(In reply to comment #13) > taken, are you okay to do this soon? > If you have a time, could you take this bug?
Assignee: taken.spc → nobody
Status: ASSIGNED → NEW
Okay, will do. Thanks for starting this off! I'll move this over to bug 418206 though since that's the real Firefox 3 blocker.
WFM (Mac trunk debug) jwatt, does anything here (such as comment 11, or the patch in comment 6) need to be spun off into a new bug?
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.