825519 - Add telemetry counting max(# times this image was decoded)

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Justin Lebar (not reading bugmail)]

We've now had two bugs where we enter into a loop decoding an image over and over again.

So it seems like it would be worthwhile to add telemetry which would let us see how many people are affected by this bug.  That way we can verify our eventual fix, and we can verify that it doesn't regress.

I'd probably want to report both

 1) # times this image was decoded (for all images), and
 2) max_over_all_images(# times this image was decoded).

(2) is what we need to see if we've hit this decode-infinitely bug, and (1) is simple to do once we have (2) and is interesting for tuning discard heuristics.

Comment 1

[Justin Lebar (not reading bugmail)]

Oh, we already have (2).  So this is even easier.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 3

[Joe Drew (not getting mail)]

Comment on attachment 697107 [details] [diff] [review]
Patch, v1

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

::: image/src/RasterImage.cpp
@@ +2581,5 @@
> +    if (mDecodeCount > sMaxDecodeCount) {
> +      // Don't subtract out 0 from the histogram, because that causes its count
> +      // to go negative, which is not kosher.
> +      if (sMaxDecodeCount > 0) {
> +        Telemetry::GetHistogramById(Telemetry::IMAGE_MAX_DECODE_COUNT)->Subtract(sMaxDecodeCount);

It'd be nice if this underlying bug could be fixed.

Comment 4

[Justin Lebar (not reading bugmail)]

> It'd be nice if this underlying bug could be fixed.

It's merely an assertion, I think.  I guess to fix the bug we could have a "remove and add" function, but we'd still need to call the vanilla "add" function the first time...  :-/

Comment 5

[Nathan Froyd [:froydnj]]

I think from a telemetry perspective, you might be better off exposing this as a single number in the ping's simpleMeasurements or similar, since that's really what it is.  Slightly more complicated, though. :(  I'm not sure if the data collection portion will DTRT with a histogram whose buckets are not monotonically increasing over time...

Comment 6

[Justin Lebar (not reading bugmail)]

> you might be better off exposing this as a single number in the ping's simpleMeasurements 
> or similar, since that's really what it is. 

I agree that would be better, but it's complicated, like you say.  :)

I was told that the data collection back-end deletes all old data for a session when a new ping for that session arrives.  If it doesn't DTRT with this patch, it's basically impossible to use Histogram::Subtract correctly, which is going to be a much more general problem...

Comment 7

[Justin Lebar (not reading bugmail)]

I went ahead and landed this.  If the data collection is broked, that would be useful to discover!

Comment 8

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d51d7f54e5d

Comment 9

[(no longer active)]

Backed out since this broke the build: https://hg.mozilla.org/integration/mozilla-inbound/rev/e57fcdedf527

Comment 10

[Justin Lebar (not reading bugmail)]

cc1plus: warnings being treated as errors
../../../image/src/RasterImage.cpp: In member function 'nsresult mozilla::image::RasterImage::InitDecoder(bool)':
../../../image/src/RasterImage.cpp:2581:24: error: comparison between signed and unsigned integer expressions

le sigh.

Comment 11

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd2331e6798

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ecd2331e6798