1214054 - Don't fire DECODE_COMPLETE in VectorImage::OnSVGDocumentError since it implies we have a size available when we don't

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Firing DECODE_COMPLETE in VectorImage::OnSVGDocumentError() implies that we have a size available, but because parsing the SVG document failed, we don't. This is causing the new assertions I'm adding in bug 1210745 to fire. The fix is easy: just don't fire DECODE_COMPLETE here, since there's no reason to do it.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't fire DECODE_COMPLETE in VectorImage::OnSVGDocumentError().

Here's the patch.

Comment 2

[Daniel Holbert [:dholbert]]

From digging through hg blame a bit, I think this flag is here simply because we used to call imgDecoderObserver::OnStopDecode (which was "Called when all decoding has terminated"), with an error nsresult in this function:
 http://hg.mozilla.org/mozilla-central/rev/b892bd18e0d9#l6.68

And that function internally set this flag (via RecordStopDecode), which at that point was called FLAG_DECODE_STOPPED.  And in the refactoring that's removed these layers of abstraction, we've preserved that flag here.

(In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> Firing DECODE_COMPLETE in VectorImage::OnSVGDocumentError() implies that we
> have a size available

But we do also have FLAG_SIZE_AVAILABLE (and STATUS_SIZE_AVAILABLE), which we're explicitly not setting here.  Are you that setting DECODE_COMPLETE must now imply that we have also set SIZE_AVAILABLE?

(Do we enforce this in raster images as well? Maybe you're making us enforce this over in bug 1210745?)

Comment 3

[Daniel Holbert [:dholbert]]

Put another way: do we *really* want DECODE_COMPLETE to necessarily imply SIZE_AVAILABLE?

From my skimming of the code, it seems like we interpret DECODE_COMPLETE as letting us know whether we're still decoding & potentially might be missing information about the image which will be filled in later.  So if we switch this code to *no longer* set it, it seems like we're giving the implication that we're still decoding & more information is still coming (incorrectly).  Maybe that doesn't actually matter for any of the callers though, not sure.

In any case:
 (1) If you're sure you want there to be a strict dependency/implication between DECODE_COMPLETE and SIZE_AVAILABLE, please make sure that's documented somewhere
 (2) Please make sure (e.g. via code inspection) that image status clients who check DECODE_COMPLETE (via FLAG_ / STATUS_ / whatever) will be OK with the flag being unset for images that have errored out.  (either because their assumptions are fine, or because they check for FLAG_ERROR/STATUS_ERROR/whatever before checking this flag)

I'm happy to r+ with (1) and (2). (maybe you've already taken care of one or both of those in another bug)

Comment 4

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] (less responsive Oct 9-12 & 15-18) from comment #2)
> But we do also have FLAG_SIZE_AVAILABLE (and STATUS_SIZE_AVAILABLE), which
> we're explicitly not setting here.  Are you that setting DECODE_COMPLETE
> must now imply that we have also set SIZE_AVAILABLE?

Yeah, and really it always did, we just have been sloppy in the past and are getting less sloppy.

The reason is that loading an image can essentially result in two outcomes:

- The load event firing, which occurs when we get all the data from the network and can decode enough of the image to determine the intrinsic size. This is indicated by the combination of SIZE_AVAILABLE and LOAD_COMPLETE, and it's a requirement that we fire SIZE_AVAILABLE before or at the same time as LOAD_COMPLETE.

- The error event firing, which occurs when, for whatever reason, we cannot decode the image enough to determine an intrinsic size. This is indicated by firing ERROR.

We never start a full decode until we've entered one of those two states, and we'd never trigger a full decode if we were in the ERROR state, so we cannot ever reach DECODE_COMPLETE without firing SIZE_AVAILABLE. (Well, at least in RasterImage, which is unfortunately tested much more thoroughly than VectorImage. But it should be like that in VectorImage too.)


> (Do we enforce this in raster images as well? Maybe you're making us enforce
> this over in bug 1210745?)

We do enforce this in RasterImage, yeah. We previously didn't have any test that tested that VectorImage also enforced this. In bug 1210745 I added an assert that fires in some cases because, it turns out, VectorImage doesn't enforce this after all.

(In reply to Daniel Holbert [:dholbert] (less responsive Oct 9-12 & 15-18) from comment #3)
> Maybe that doesn't actually matter for any of the callers though, not sure.

It doesn't, because a caller that saw that ERROR had fired should not expect DECODE_COMPLETE to fire. The existing code expects this, since RasterImage works this way.

> In any case:
>  (1) If you're sure you want there to be a strict dependency/implication
> between DECODE_COMPLETE and SIZE_AVAILABLE, please make sure that's
> documented somewhere

The asserts in CheckProgressConsistency in ProgressTracker.cpp are considered (by me at least! =) the canonical documentation for this in some sense. They could probably use more comments. We could potentially add some documentation in imgIRequest.idl as well, though I worry it could get out of sync with the code.

>  (2) Please make sure (e.g. via code inspection) that image status clients
> who check DECODE_COMPLETE (via FLAG_ / STATUS_ / whatever) will be OK with
> the flag being unset for images that have errored out.  (either because
> their assumptions are fine, or because they check for
> FLAG_ERROR/STATUS_ERROR/whatever before checking this flag)

As mentioned above, any client that couldn't handle this would already fail, since this is the same behavior as RasterImage.

Sold? =)

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #4)
> We never start a full decode until we've entered one of those two states,
> and we'd never trigger a full decode if we were in the ERROR state, so we
> cannot ever reach DECODE_COMPLETE without firing SIZE_AVAILABLE. (Well, at
> least in RasterImage, which is unfortunately tested much more thoroughly
> than VectorImage. But it should be like that in VectorImage too.)

Obviously we are sorta faking this in the VectorImage case since, unlike RasterImage, there's no separate metadata decode that pulls out the intrinsic size. In other words, the distinction about when we'd trigger a full decode doesn't *actually* apply to VectorImage. But I think it's important that, to the best of our ability, callers don't perceive any difference between VectorImage and RasterImage. And in theory we could add a metadata decode phase to VectorImage in the future, but it's not much of a win if we have to decode on the main thread.

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8672891 [details] [diff] [review]
Don't fire DECODE_COMPLETE in VectorImage::OnSVGDocumentError().

(In reply to Seth Fowler [:seth] [:s2h] from comment #4)
> >  (1) If you're sure you want there to be a strict dependency/implication
> > between DECODE_COMPLETE and SIZE_AVAILABLE, please make sure that's
> > documented somewhere
> 
> The asserts in CheckProgressConsistency in ProgressTracker.cpp are
> considered (by me at least! =) the canonical documentation for this in some
> sense. They could probably use more comments. We could potentially add some
> documentation in imgIRequest.idl as well, though I worry it could get out of
> sync with the code.

Gotcha.  I was looking for this to be documented (or at least hinted at) where the actual flags are defined/briefly-documented in ProgressTracker.h:
 30 // Image progress bitflags.
 31 enum {
 32   FLAG_SIZE_AVAILABLE     = 1u << 0,  // STATUS_SIZE_AVAILABLE
 33   FLAG_DECODE_COMPLETE    = 1u << 1,  // STATUS_DECODE_COMPLETE
 [...]
http://mxr.mozilla.org/mozilla-central/source/image/ProgressTracker.h#30

Perhaps it'd be worth adding a brief comment there (after "Image progress bitflags"), saying e.g.
  // See CheckProgressConsistency in ProgressTracker.cpp for
  // invariants about these flags' interdependencies.

(In reply to Seth Fowler [:seth] [:s2h] from comment #5)
> But I think it's
> important that, to the best of our ability, callers don't perceive any
> difference between VectorImage and RasterImage.

Agreed.

This all sounds good; thanks for explaining! r=me

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Gotcha.  I was looking for this to be documented (or at least hinted at)
> where the actual flags are defined/briefly-documented in ProgressTracker.h:

Done.

Thanks for the review!

Comment 8

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c37c41d2efe

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1c37c41d2efe