1112533 - Get rid of SVGDocumentWrapper::GetWidthOrHeight

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

As requested by dholbert at the end of bug 1086284 comment 3.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8537745 [details] [diff] [review]
patch

>+++ b/image/src/VectorImage.cpp
>@@ -480,26 +480,20 @@ VectorImage::SetAnimationStartTime(const
> 
> //******************************************************************************
> /* readonly attribute int32_t width; */
> NS_IMETHODIMP
> VectorImage::GetWidth(int32_t* aWidth)
> {
>   if (mError || !mIsFullyLoaded) {
>     *aWidth = 0;
>-    return NS_ERROR_FAILURE;
>+  } else {
>+    *aWidth = mSVGDocumentWrapper->GetRootSVGElem()->GetIntrinsicWidth();
>   }

GetRootSVGElem() is allowed to return null, though I think it only can do so during loading, or if there's an error, or during destruction -- so we should probably be fine. (since we've already returned for error/loading cases, and this function shouldn't be called during destruction) Still -- we should assert that it's non-null here before using it, to document/justify our "non-null" assumption here.   (fatally, w/ MOZ_ASSERT, since we'll crash if the assertion fails anyway)

So: please capture GetRootSVGElem() in a local-var here and assert that it's non-null before using it. (with assertion-message being something like "should have a root SVG elem, since we finished loading without errors").

Same goes for VectorImage::GetHeight().

>@@ -675,22 +663,23 @@ VectorImage::GetFrame(uint32_t aWhichFra
>   if (mError)
>     return nullptr;
> 
>   // Look up height & width
>   // ----------------------
[...]
>+  SVGSVGElement* svgElem = mSVGDocumentWrapper->GetRootSVGElem();
>+  nsIntSize imageIntSize(svgElem->GetIntrinsicWidth(),
>+                         svgElem->GetIntrinsicHeight());

Two things:
 (1) as above, assert that svgElem is non-null before using it here.
 (2) I think we can't actually be sure of that assertion, *unless* we expand the "if (mError)" early-return to be (mError || !mIsFullyLoaded). So, please do that -- expand the check on that early-return to match the one in GetWidth/GetHeight.

(Issue (2) there should already make us able to crash in this code, I think -- I'll bet we've just gotten lucky and haven't hit that as a problem yet because GetFrame() callers are rare, and the ones that exist only call GetFrame() after receiving a "image fully loaded" status.  So mIsFullyLoaded turns out to always be true here, in practice, with existing callers. Anyway, we shouldn't depend on that.)

r=me with above addressed.

Comment 3

[Jonathan Watt [:jwatt]]

It seems this (now? pretty sure this passed before) causes the W3C Web Platform test 2dcontext/drawing-images-to-the-canvas/2d.drawImage.zerosource.image.html to fail because the line:

  assert_throws("INDEX_SIZE_ERR", function() { ctx.drawImage(document.getElementById('red-zerosize.svg'), 0, 0, 100, 50); });

Throws "Component is not available" instead of INDEX_SIZE_ERR.

This turns out to be because making VectorImage::GetWidth/GetHeight return NS_ERROR_FAILURE means that when we return the SurfaceFromElementResult without a SourceSurface or imgIContainer from this stack:

  VectorImage::GetWidth/GetHeight
  nsLayoutUtils::SurfaceFromElement
  nsLayoutUtils::SurfaceFromElement
  mozilla::dom::CanvasRenderingContext2D::DrawImage

DrawImage throws NS_ERROR_NOT_AVAILABLE:

https://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp?rev=a0f3db72a62c&mark=4307-4312#4306

It seems to me that this code path should be changed to allow the DrawImage code to tell that the issue is due to the image being zero sized so it can throw INDEX_SIZE_ERR. For now though I've changed my patch to use < 0 to indicate percentage dimensions so that only that causes an error (basically maintaining the current behavior). I'll file a follow-up to change the DrawImage code.

Comment 4

[Jonathan Watt [:jwatt]]

Updated to address review comments and to use -1 instead of 0 to flag percentages:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6390ede276ae

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6390ede276ae