Bug 1205923: Make VectorImage::GetWidth/GetHeight set outparam to 0 (not -1) on failure, to accomodate callers that don't check error codes. r=seth
authorDaniel Holbert <dholbert@cs.stanford.edu>
Fri, 18 Sep 2015 15:33:43 -0700
changeset 264141 d57165a9b3360fb5009184dc90c1cd361782f4bc
parent 264140 4f0530d695b6898a865e61b0bfaa39761a815f6e
child 264142 f0894b47de2d689402f9dd481c0f211f48762e23
push id65535
push userdholbert@mozilla.com
push dateThu, 24 Sep 2015 04:10:38 +0000
treeherdermozilla-inbound@d57165a9b336 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersseth
bugs1205923
milestone44.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1205923: Make VectorImage::GetWidth/GetHeight set outparam to 0 (not -1) on failure, to accomodate callers that don't check error codes. r=seth
image/VectorImage.cpp
image/test/crashtests/1205923-1.html
image/test/crashtests/crashtests.list
image/test/crashtests/unsized-svg.svg
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -492,24 +492,33 @@ VectorImage::SetAnimationStartTime(const
 //------------------------------------------------------------------------------
 // imgIContainer methods
 
 //******************************************************************************
 NS_IMETHODIMP
 VectorImage::GetWidth(int32_t* aWidth)
 {
   if (mError || !mIsFullyLoaded) {
-    *aWidth = -1;
-  } else {
-    SVGSVGElement* rootElem = mSVGDocumentWrapper->GetRootSVGElem();
-    MOZ_ASSERT(rootElem, "Should have a root SVG elem, since we finished "
-                         "loading without errors");
-    *aWidth = rootElem->GetIntrinsicWidth();
+    // XXXdholbert Technically we should leave outparam untouched when we
+    // fail. But since many callers don't check for failure, we set it to 0 on
+    // failure, for sane/predictable results.
+    *aWidth = 0;
+    return NS_ERROR_FAILURE;
   }
-  return *aWidth >= 0 ? NS_OK : NS_ERROR_FAILURE;
+
+  SVGSVGElement* rootElem = mSVGDocumentWrapper->GetRootSVGElem();
+  MOZ_ASSERT(rootElem, "Should have a root SVG elem, since we finished "
+             "loading without errors");
+  int32_t rootElemWidth = rootElem->GetIntrinsicWidth();
+  if (rootElemWidth < 0) {
+    *aWidth = 0;
+    return NS_ERROR_FAILURE;
+  }
+  *aWidth = rootElemWidth;
+  return NS_OK;
 }
 
 //******************************************************************************
 NS_IMETHODIMP_(void)
 VectorImage::RequestRefresh(const TimeStamp& aTime)
 {
   if (HadRecentRefresh(aTime)) {
     return;
@@ -556,24 +565,33 @@ VectorImage::GetImageSpaceInvalidationRe
   return aRect;
 }
 
 //******************************************************************************
 NS_IMETHODIMP
 VectorImage::GetHeight(int32_t* aHeight)
 {
   if (mError || !mIsFullyLoaded) {
-    *aHeight = -1;
-  } else {
-    SVGSVGElement* rootElem = mSVGDocumentWrapper->GetRootSVGElem();
-    MOZ_ASSERT(rootElem, "Should have a root SVG elem, since we finished "
-                         "loading without errors");
-    *aHeight = rootElem->GetIntrinsicHeight();
+    // XXXdholbert Technically we should leave outparam untouched when we
+    // fail. But since many callers don't check for failure, we set it to 0 on
+    // failure, for sane/predictable results.
+    *aHeight = 0;
+    return NS_ERROR_FAILURE;
   }
-  return *aHeight >= 0 ? NS_OK : NS_ERROR_FAILURE;
+
+  SVGSVGElement* rootElem = mSVGDocumentWrapper->GetRootSVGElem();
+  MOZ_ASSERT(rootElem, "Should have a root SVG elem, since we finished "
+             "loading without errors");
+  int32_t rootElemHeight = rootElem->GetIntrinsicHeight();
+  if (rootElemHeight < 0) {
+    *aHeight = 0;
+    return NS_ERROR_FAILURE;
+  }
+  *aHeight = rootElemHeight;
+  return NS_OK;
 }
 
 //******************************************************************************
 NS_IMETHODIMP
 VectorImage::GetIntrinsicSize(nsSize* aSize)
 {
   if (mError || !mIsFullyLoaded) {
     return NS_ERROR_FAILURE;
new file mode 100644
--- /dev/null
+++ b/image/test/crashtests/1205923-1.html
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<body>
+</body>
+<script>
+  function createImage(loadHandler) {
+    var newImage = new Image;
+    newImage.id = "thepreviewimage";
+    newImage.setAttribute("src", "unsized-svg.svg");
+    if (loadHandler) {
+      newImage.onload = loadHandler;
+    }
+
+    // Query width & height, and display them in document:
+    physWidth = newImage.width;
+    physHeight = newImage.height;
+    document.documentElement.innerHTML +=
+      physWidth + " x " + physHeight + "<br>";
+  }
+
+  function part2() {
+    // Load image again:
+    createImage();
+
+    // End the crashtest.
+    document.documentElement.removeAttribute("class");
+  }
+
+  function startTest() {
+    // Trigger image load, and call part2() when it's loaded:
+    createImage(part2);
+  }
+
+  startTest();
+</script>
+</html>
--- a/image/test/crashtests/crashtests.list
+++ b/image/test/crashtests/crashtests.list
@@ -48,12 +48,14 @@ load 856616.gif
 
 skip-if(AddressSanitizer) skip-if(B2G) load 944353.jpg
 
 # Bug 1160801: Ensure that we handle invalid disposal types.
 load invalid-disposal-method-1.gif
 load invalid-disposal-method-2.gif
 load invalid-disposal-method-3.gif
 
+load 1205923-1.html
+
 # Ensure we handle ICO directory entries which specify the wrong size for the
 # contained resource.
 load invalid_ico_height.ico
 load invalid_ico_width.ico
new file mode 100644
--- /dev/null
+++ b/image/test/crashtests/unsized-svg.svg
@@ -0,0 +1,1 @@
+<svg xmlns="http://www.w3.org/2000/svg"></svg>