1317464 - 3,200 instances of "Image width or height is non-positive" emitted from layout/base/nsLayoutUtils.cpp during linux64 debug testing

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Eric Rahm [:erahm]]

> 2854 WARNING: Image width or height is non-positive: file layout/base/nsLayoutUtils.cpp, line 6527

This warning [1] shows up in the following test suites:

>    720 - desktop-test-linux64/debug-reftest-no-accel-4 Ru4
>    680 - desktop-test-linux64/debug-reftest-e10s-4 R4
>    680 - desktop-test-linux64/debug-reftest-4 R4
>    640 - desktop-test-linux64/debug-reftest-no-accel-e10s-4 Ru4
>     43 - desktop-test-linux64/debug-reftest-no-accel-7 Ru7
>     42 - desktop-test-linux64/debug-reftest-7 R7
>     22 - desktop-test-linux64/debug-reftest-e10s-7 R7
>     20 - desktop-test-linux64/debug-reftest-no-accel-e10s-7 Ru7
>      3 - desktop-test-linux64/debug-reftest-no-accel-6 Ru6
>      2 - desktop-test-linux64/debug-reftest-no-accel-e10s-6 Ru6
>      1 - desktop-test-linux64/debug-reftest-6 R6
>      1 - desktop-test-linux64/debug-reftest-e10s-6 R6

It shows up in 21 tests. A few of the most prevalent:

>    360 - [e10s] file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-005.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-005-ref.html
>    360 -        file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-004.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-004-ref.html
>    360 -        file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-005.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-005-ref.html
>    360 -        file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-007.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-007-ref.html
>    320 - [e10s] file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-006.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-006-ref.html
>    320 - [e10s] file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-004.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-004-ref.html
>    320 -        file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-006.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-006-ref.html
>    320 - [e10s] file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-007.html == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/css-grid/grid-item-intrinsic-ratio-stretch-007-ref.html
>     42 -        file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/image/image-preserveAspectRatio-03.svg == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/image/image-preserveAspectRatio-03-ref.svg
>     40 -        file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/image/image-preserveAspectRatio-01-svg.svg == file:///home/worker/workspace/build/tests/reftest/tests/layout/reftests/svg/image/image-preserveAspectRatio-01-ref.svg

[1] https://hg.mozilla.org/mozilla-central/annotate/71fd23fa0803/layout/base/nsLayoutUtils.cpp#l6527

Comment 1

[Eric Rahm [:erahm]]

Repros locally for me with:

> ./mach test layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-005.html

Comment 2

[Eric Rahm [:erahm]]

Mats, I've bisected down to the following range which are all seem to be changes you pushed:

> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=60031ad275cc7ffff4bbc0ba9b75bc6255ef230e&tochange=9d1b72a21a395e36cbfd9982243054e64a955bb3

Comment 3

[Eric Rahm [:erahm]]

This continues to be a top warning and has become more verbose.

Comment 4

[Eric Rahm [:erahm]]

This continues to be a top warning.

Comment 5

[Timothy Nikkel (:tnikkel)]

An image container can return 0 width/height if it's for an svg image with no specific size (only percentage).

Not clear if the warning is coming from nsLayoutUtils::DrawSingleImage or nsLayoutUtils::DrawSingleUnscaledImage. DrawSingleImage seems to do the right thing, using a fallback if it can't get size from the image. DrawSingleUnscaledImage doesn't have a fallback.

Comment 6

[Mats Palmgren (inactive)]

The patches in comment 2 intentionally added SVG images with zero width/height
as those triggered edge cases in our code that we previously had no tests for.
I'm guessing that's what made these warnings spike.

I think we should just remove the warning since there are legitimate cases
for images to have intrinsic zero size.  Or perhaps restrict it to cases
where it must not be zero, if there are such cases?

Comment 7

[Mats Palmgren (inactive)]

Personally, I see very little value in these warnings.
The warning should probably be in somewhere in ImageLib instead, for image types
that should never have zero intrinsic size (if there are such types).

Comment 8

[Timothy Nikkel (:tnikkel)]

Yeah, there are legimate cases for images to have 0 width/height. So the code should be written to handle them properly, and if it is then we don't need to warn about it. The only question I have is if DrawSingleUnscaledImage properly handles that case.

Comment 9

[Mats Palmgren (inactive)]

Attachment: wip

Let's see if this works:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=928d882b99d1d252669dd17a6a85a1b8be92bb1b

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix

Try run looks green.
I did a bit of code archeology to see where this code came from.
The behavior was changed in:
https://bugzilla.mozilla.org/attachment.cgi?id=390617&action=diff

Prior to that we did an early return (NS_OK) if the image size was zero,
but that patch changed it to an error with the motivation that
"imgContainer::Init() does not allow zero-sized images to begin with"
(bug 506409 comment 1), which was true at the time:
https://hg.mozilla.org/try/file/695b882e085a/modules/libpr0n/src/imgContainer.cpp#l119
(it's a bit odd that they added just a warning though, not an assertion,
but that's probably unintentional due to NS_ENSURE_TRUE *shrug*)

The check in Init() was removed in 2009:
https://hg.mozilla.org/mozilla-central/diff/9f856f094fea/modules/libpr0n/src/imgContainer.cpp#l1.212
when bholley rewrote much of that code (so I haven't checked if he added
it somewhere else instead, but I suspect not).

So returning SUCCESS for zero sized images seems correct to me.
Also, I think negative image sizes should never occur and we should assert that.
Seems like we should handle it gracefully though, just in case.

Comment 11

[Mats Palmgren (inactive)]

Do you have an ETA for this review? Or, let me know if you want me to ask someone else.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8825429 [details] [diff] [review]
fix

The DrawSingleImage looks good.

The DrawSingleUnscaledImage seems wrong though. Callers of DrawSingleUnscaledImage that pass a vector image with no fixed size should be fixed.

There are only two users of DrawSingleUnscaledImage: nsSVGImageFrame and nsTreeBodyFrame. nsSVGImageFrame uses DrawSingleImage for vector images for exactly this reason. nsTreeBodyFrame looks like it needs to be taught about how to handle vector image in this case.

I suspect that these warnings are not coming from drawing vector images in nsTreeBodyFrame, so your fix to DrawSingleImage should be enough to quiet the vast majority of these warnings. The warnings in DrawSingleUnscaledImage seem real and we should probably continue warning about them.

r+ if you want to land the DrawSingleImage change which should clear up this log spam.

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix

OK, GetWidth/GetHeight actually have an nsresult we could use for that.

Comment 14

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8828416 [details] [diff] [review]
fix

>@@ -6518,21 +6518,26 @@ nsLayoutUtils::DrawSingleUnscaledImage(g
>                                        imgIContainer*       aImage,
>                                        const SamplingFilter aSamplingFilter,
>                                        const nsPoint&       aDest,
>                                        const nsRect*        aDirty,
>                                        uint32_t             aImageFlags,
>                                        const nsRect*        aSourceArea)
> {
>   CSSIntSize imageSize;
>-  aImage->GetWidth(&imageSize.width);
>-  aImage->GetHeight(&imageSize.height);
>+  DebugOnly<nsresult> rv = aImage->GetWidth(&imageSize.width);
>+  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
>+                       "attempt to draw image with no width");
>+  rv = aImage->GetHeight(&imageSize.height);
>+  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
>+                       "attempt to draw image with no height");
>   if (imageSize.width < 1 || imageSize.height < 1) {
>-    NS_WARNING("Image width or height is non-positive");
>-    return DrawResult::TEMPORARY_ERROR;
>+    NS_ASSERTION(imageSize.width >= 0 && imageSize.height >= 0,
>+                 "Image width or height is negative");
>+    return DrawResult::SUCCESS;  // no point in drawing a zero size image
>   }

I think this still needs to return DrawResult::TEMPORARY_ERROR and not SUCCESS because raster images that don't know their size yet will return 0 and NS_OK.

Returning SUCCESS in DrawSingleImage seems okay though because we only get zero-size from ComputeSizeForDrawingWithFallback if both the image size and the destination size is zero. And drawing to a empty destination is a no-op, so successful.

Comment 15

[Mats Palmgren (inactive)]

> raster images that don't know their size yet will return 0 and NS_OK.

... and vector images return 0 and a failure?  That seems like a bug to me,
the caller shouldn't have to deal with such API inconsistencies.  I think
we should change all GetWidth/Height implementations to return a failure
when the size is unknown.

Comment 16

[Timothy Nikkel (:tnikkel)]

(In reply to Mats Palmgren (:mats) from comment #15)
> > raster images that don't know their size yet will return 0 and NS_OK.
> 
> ... and vector images return 0 and a failure?

Vector images that will _never_ know their size return 0 and a failure (because their size is specified in percent, so they cannot respond with a concrete size, it's up to the caller to do the right thing in that situation, like use the size that the image will be drawn at). Note that this is quite different from when raster images return 0 and NS_OK.

> That seems like a bug to me,
> the caller shouldn't have to deal with such API inconsistencies.  I think
> we should change all GetWidth/Height implementations to return a failure
> when the size is unknown.

It's not an ideal API and improvements are welcome but there is a way to make sense of it:

1) return 0 and NS_OK -> size information is not known yet
2) return 0 and failure -> all size information is known but no concrete size can be provided for this image
2) non-zero -> concrete size available.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e66dbc63ed5
Zero size images are valid and shouldn't trigger warnings.  Assert that the image size isn't negative though.  r=tn

Comment 18

[Mats Palmgren (inactive)]

> r+ if you want to land the DrawSingleImage change which should clear up this log spam.

Yeah, I took that option and left DrawSingleUnscaledImage unchanged for now,
as that's likely not the source of these warning messages.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5e66dbc63ed5