bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 54

Status

()

Core
Layout
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: mats)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

> 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
Repros locally for me with:

> ./mach test layout/reftests/css-grid/grid-item-intrinsic-ratio-normal-005.html
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
Flags: needinfo?(mats)
This continues to be a top warning and has become more verbose.
Summary: 2,900 instances of "Image width or height is non-positive" emitted from layout/base/nsLayoutUtils.cpp during linux64 debug testing → 3,200 instances of "Image width or height is non-positive" emitted from layout/base/nsLayoutUtils.cpp during linux64 debug testing
This continues to be a top warning.
Flags: needinfo?(mats)
Flags: needinfo?(mats)
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.
(Assignee)

Comment 6

2 years ago
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?
Flags: needinfo?(mats)
(Assignee)

Comment 7

2 years ago
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).
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.
(Assignee)

Comment 10

2 years ago
Created attachment 8825429 [details] [diff] [review]
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.
Assignee: nobody → mats
Attachment #8825206 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8825429 - Flags: review?(tnikkel)
(Assignee)

Comment 11

2 years ago
Do you have an ETA for this review? Or, let me know if you want me to ask someone else.
Flags: needinfo?(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.
Flags: needinfo?(tnikkel)
Attachment #8825429 - Flags: review?(tnikkel)
(Assignee)

Comment 13

2 years ago
Created attachment 8828416 [details] [diff] [review]
fix

OK, GetWidth/GetHeight actually have an nsresult we could use for that.
Attachment #8825429 - Attachment is obsolete: true
Attachment #8828416 - Flags: review?(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.
Attachment #8828416 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 15

2 years ago
> 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.
(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

2 years ago
Pushed by mpalmgren@mozilla.com:
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
(Assignee)

Comment 18

2 years ago
> 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5e66dbc63ed5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.