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

VERIFIED FIXED in Firefox 47

Status

()

VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: erahm, Assigned: dholbert)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

This is currently the most verbose layout warning during testing.

> 551 [NNNNN] WARNING: Image width or height is non-positive: file layout/base/nsLayoutUtils.cpp, line 6233

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

> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm116-tests1-linux64-build52.txt:208
> mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm123-tests1-linux64-build16.txt:69
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm117-tests1-linux64-build23.txt:42
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm117-tests1-linux64-build3.txt:37
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm118-tests1-linux64-build12.txt:35
> mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-2-bm67-tests1-linux64-build4.txt:32
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm54-tests1-linux64-build30.txt:31
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm115-tests1-linux64-build34.txt:25
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm120-tests1-linux64-build7.txt:24
> mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm115-tests1-linux64-build12.txt:12
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm54-tests1-linux64-build15.txt:11
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm52-tests1-linux64-build16.txt:10
> mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm121-tests1-linux64-build13.txt:5
> mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm123-tests1-linux64-build11.txt:2
> mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm115-tests1-linux64-build34.txt:2
> mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm115-tests1-linux64-build14.txt:1
> mozilla-central_ubuntu64_vm-debug_test-mochitest-gl-bm54-tests1-linux64-build4.txt:1
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm117-tests1-linux64-build2.txt:1
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm121-tests1-linux64-build21.txt:1
> mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm120-tests1-linux64-build46.txt:1
> mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm51-tests1-linux64-build4.txt:1

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

> 29 - browser/base/content/test/general/browser_bug735471.js
> 27 - browser/components/customizableui/test/browser_1087303_button_preferences.js
> 24 - browser/components/translation/test/browser_translation_yandex.js
> 19 - docshell/test/test_bug669671.html
> 18 - dom/manifest/test/browser_ManifestObtainer_obtain.js
> 13 - toolkit/devtools/server/tests/browser/browser_canvasframe_helper_02.js
> 12 - dom/animation/test/css-transitions/test_csstransition-transitionproperty.html
> 9 - docshell/test/navigation/test_sibling-off-domain.html
> 9 - docshell/test/navigation/test_not-opener.html
> 9 - browser/components/feeds/test/test_registerHandler.html

[1] https://hg.mozilla.org/mozilla-central/annotate/b12a261ee32e/layout/base/nsLayoutUtils.cpp#l6233
(Assignee)

Updated

3 years ago
Flags: needinfo?(dholbert)
(Assignee)

Comment 1

3 years ago
IIRC, this means we think we're laying out an image which has a *negative* width or height.

It's likely that there's something broken going on, except maybe in cases where tests use images with bizarre (huge?) sizes. (and it doesn't look like these tests would be to do that, based on their names/paths)  So, probably something's broken.
(Assignee)

Comment 2

3 years ago
Circled back to this when looking at my stale needinfo requests.  Sorry for the delay here.

I can reproduce this locally by running:
  ./mach mochitest browser/base/content/test/general/browser_bug735471.js
(though I only get 14 instances of the warning -- not 29 as noted in comment 0)

It looks like the image in question for at least one of the warnings is:
  chrome://browser/skin/preferences/in-content/icons.svg#search
and it ends up with 0 size because the image is not fully loaded, which means GetWidth & GetHeight & GetIntrinsicRatio all return false.  (So ComputeSizeForDrawing returns (0,0) size up to ComputeSizeForDrawingWithFallback, which returns that up to DrawSingleImage, which spams this warning.)

So: to the extent that there's a bug here, it's that we're trying to draw this VectorImage before it's ready. I thought we had notifications set up which should prevent this.
Flags: needinfo?(dholbert)
(Assignee)

Comment 3

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #2)
> and it ends up with 0 size because the image is not fully loaded, which
> means GetWidth & GetHeight & GetIntrinsicRatio all return false.

(Those are all functions on VectorImage, btw, and by "return false" I meant "return NS_ERROR_FAILURE".)

Also: this is inside of nsDisplayXULImage::Paint()'s call to nsImageBoxFrame::PaintImage(). I wouldn't be surprised if XUL played more fast & loose with image notifications (trying to draw images prematurely) than HTML images do...
(Assignee)

Comment 4

3 years ago
Created attachment 8695417 [details] [diff] [review]
fix v1: check if image size is available before painting, in nsImageBoxFrame

This seems to fix the problem (at least in browser_bug735471.js) -- just check the image status before trying to paint.

Seth, do you if there's a more elegant solution here / if we should be preventing this at some other level?

(One other semi-equivalent option would be to add a bool & set it when OnSizeAvailable is called, and bail from PaintImage if the bool is unset. That saves us from needing the status lookup, but it bloats our frame by an extra bool. Not sure about the tradeoff there.)
Attachment #8695417 - Flags: review?(seth)
Thanks for getting back to this!

(In reply to Daniel Holbert [:dholbert] from comment #2)
> Circled back to this when looking at my stale needinfo requests.  Sorry for
> the delay here.
> 
> I can reproduce this locally by running:
>   ./mach mochitest browser/base/content/test/general/browser_bug735471.js
> (though I only get 14 instances of the warning -- not 29 as noted in comment
> 0)

FWIW those stats probably include both e10s and non-e10s which seems to fit with what you're seeing.
Comment on attachment 8695417 [details] [diff] [review]
fix v1: check if image size is available before painting, in nsImageBoxFrame

Review of attachment 8695417 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! I think this approach is the better one; I'd rather not duplicate state by storing a bool and then discover bugs later where the state gets out of sync.

::: layout/xul/nsImageBoxFrame.cpp
@@ +345,5 @@
>      return DrawResult::TEMPORARY_ERROR;
>    }
>  
> +  // Don't draw if the image's size isn't available.
> +  uint32_t imgStatus;

Just for my sanity, please initialize this to 0.

@@ +348,5 @@
> +  // Don't draw if the image's size isn't available.
> +  uint32_t imgStatus;
> +  if (!NS_SUCCEEDED(mImageRequest->GetImageStatus(&imgStatus)) ||
> +      !(imgStatus & imgIRequest::STATUS_SIZE_AVAILABLE)) {
> +    return DrawResult::TEMPORARY_ERROR;

Probably NOT_READY is more semantically appropriate here.
Attachment #8695417 - Flags: review?(seth) → review+
(Assignee)

Comment 8

3 years ago
 (In reply to Seth Fowler [:seth] [:s2h] from comment #7)
> > +  // Don't draw if the image's size isn't available.
> > +  uint32_t imgStatus;
> > +  if (!NS_SUCCEEDED(mImageRequest->GetImageStatus(&imgStatus)) ||
> > +      !(imgStatus & imgIRequest::STATUS_SIZE_AVAILABLE)) {
> > +    return DrawResult::TEMPORARY_ERROR;
> 
> Probably NOT_READY is more semantically appropriate here.

I agree that, just based on the naming, NOT_READY sounds like a better choice.

But, based on the documentation, I think TEMPORARY_ERROR might be better.  See these descriptions of the two values:
> 35  * NOT_READY: We failed to draw because no decoded version of the image was
> 36  * available. Drawing again with FLAG_SYNC_DECODE would improve the result.
> 37  * (Though FLAG_SYNC_DECODE will not necessarily work until after the image's
> 38  * load event!)
> 39  *
> 40  * TEMPORARY_ERROR: We failed to draw due to a temporary error. Drawing may
> 41  * succeed at a later time.
http://mxr.mozilla.org/mozilla-central/source/image/DrawResult.h#35

None of the extra details about NOT_READY (RE decoded versions & FLAG_SYNC_DECODE) seem to be applicable here -- see comment 2 for what's going on. (We're waiting on our SVG document to be ready, or something like that -- I can't quite remember.)

So, the generic description of TEMPORARY_ERROR seemed more in line with what's actually going on here (aside from the implied-wrongness of the word "error").

So: to the extent that code depends on these documented details (e.g. assumes that it should try again with FLAG_SYNC_DECODE as documented), TEMPORARY_ERROR seems like a better choice... But maybe we don't depend on these details that much.  What do you think?
Flags: needinfo?(seth)
(Assignee)

Updated

3 years ago
Assignee: nobody → dholbert
(In reply to Daniel Holbert [:dholbert] from comment #8)
> So: to the extent that code depends on these documented details (e.g.
> assumes that it should try again with FLAG_SYNC_DECODE as documented),
> TEMPORARY_ERROR seems like a better choice... But maybe we don't depend on
> these details that much.  What do you think?

The documentation might not be ideally clear on this point, but NOT_READY is definitely intended for the case where we haven't finished loading or decoding the image. It's true that the additional information about NOT_READY is somewhat slanted towards raster images, though.

TEMPORARY_ERROR is really much more intended as a catchall for unlikely scenarios that might temporarily prevent us from drawing - for example, being unable to allocate a buffer larger enough to store a temporary surface that we need for some reason. In this bug, there's really no "error" at all, so I think it's better to avoid TEMPORARY_ERROR.
Flags: needinfo?(seth)
(Assignee)

Comment 10

3 years ago
OK, thanks for clarifying. I'll use NOT_READY then.

RE your other review comment:
> Just for my sanity, please initialize this to 0.

I'm happy to do this if you'd really like, but I think it's somewhat counterproductive... I assume you're worried about cases where GetImageStatus() forgets to initialize the variable, and the variable gets used without being initialized. But that's something that ASAN and valgrind can help us discover -- and initializing the variable to 0 will *prevent them* from helping us discover that sort of thing.  (Since they'll see the variable as having been initialized.)

See bug 1245104 comment 1 and bug 1225428 comment 18 for more of my thoughts on this general issue (initialing variables that we shouldn't have to initialize, just for safety's sake).

Please let me know if this changes your mind at all... if not, it's not too big of a deal in this one spot and I'm happy to add in " = 0" in this one spot if it helps your sanity. :)
(Assignee)

Updated

3 years ago
Flags: needinfo?(seth)
(In reply to Daniel Holbert [:dholbert] from comment #10)
> I'm happy to do this if you'd really like, but I think it's somewhat
> counterproductive... I assume you're worried about cases where
> GetImageStatus() forgets to initialize the variable, and the variable gets
> used without being initialized. But that's something that ASAN and valgrind
> can help us discover -- and initializing the variable to 0 will *prevent
> them* from helping us discover that sort of thing.  (Since they'll see the
> variable as having been initialized.)

Well, ASAN and valgrind are dynamic tests - in other words, they *may* catch an uninitialized variable, but are not guaranteed to. Simply initializing the variable is obviously a static proof that the variable is initialized, and if the compiler can statically prove that the subsequent code will always initialize it, it can remove the initialization, so this is effectively zero cost in cases where the compiler can be confident it's safe.

My intuition is therefore in the opposite direction of yours - I prefer static checks to dynamic checks wherever possible.

However, I don't feel strongly in this particular case; I'm sure we hit this code path plenty of times in our tests, so in practice ASAN/valgrind should catch any issue. Do whatever makes you happy. =)
Flags: needinfo?(seth)
(Assignee)

Comment 12

3 years ago
(In reply to Seth Fowler [:seth] [:s2h] from comment #11)
> Well, ASAN and valgrind are dynamic tests - in other words, they *may* catch
> an uninitialized variable, but are not guaranteed to.

Right, they're not guaranteed, but at least they have a chance of catching this sort of thing. (And if any of our tests exercise this codepath in such a way as to leave the variable uninitialized, I think ASAN is smart enough that it *would* catch this sort of thing.)

If we dummy-initialize a variable, we'll cut off that possibility entirely -- we're guaranteed that they *will not* be able to catch cases where we forgot to provide the variable with a meaningful value.

> Simply initializing
> the variable is obviously a static proof that the variable is initialized

[with a dummy value, yes.]

> and if the compiler can statically prove that the subsequent code will
> always initialize it, it can remove the initialization

Agreed, but such a proof is non-trivial when the expected-initialization happens in a virtual function. So I doubt that in practice the compiler can actually remove the initialization.

> so this is
> effectively zero cost in cases where the compiler can be confident it's safe.

True; but:
 * I doubt the compiler can be confident about this in this case, since there's a virtual function call.  I can conceive of a compiler checking all known implementations of a function in all subclasses to see which ones do/don't set the outparam and under what circumstances, and how those circumstances map to its usages in the caller -- but I strongly doubt that compilers go to that trouble, in practice.
 * Even if the compiler *were* confident about this and optimized away the initialization: suppose we introduce a bug where GetImageStatus neglects to set its outparam in some case where it should. The compiler will realize this, and it'll stop optimizing away the dummy-initialization.  So, we'll be left with the initialization cost after all, and our tools will be unable to tell us that something's going wrong here. (because they won't be able to detect uninitialized usage)
 * Also: if we do introduce such a bug & do use the outparam without initializing it here, I'm pretty sure nothing catastrophic would happen -- so there's no strong reason to initialize for safety reasons here. (If this were a pointer value, I'd be more concerned & would be open to initializing it to be on the safe side.)

> However, I don't feel strongly in this particular case; I'm sure we hit this
> code path plenty of times in our tests, so in practice ASAN/valgrind should
> catch any issue. Do whatever makes you happy. =)

OK, thanks! I will leave this as-is, then, for reasons expressed above.

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/25b27835c872
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 15

3 years ago
erahm, if you're still running stats on warning counts, it'd be great if you could verify that this warning has disappeared or been greatly reduced in the logs.

(As a quick spot-check, I opened Linux64 Debug mochitest bc1, bc2, and bc3 logs from a recent inbound push, and I couldn't find any instances of the phrase "Image width" in those logs. So I'm pretty sure we're good here.)
Flags: needinfo?(erahm)
I no longer see the warning.
Flags: needinfo?(erahm)
(Assignee)

Comment 17

3 years ago
Hooray, thanks! I'll consider this VERIFIED then.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.