1098108 - Use assertions to ensure that ProgressTracker reports progress in a consistent way

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Right now there's nothing stopping us from reporting progress to ProgressTracker (and hence sending out imgINotificationObserver notifications) in almost any order. However, there is a lot of code that relies on these notifications arriving in a specific order.

We should add code to verify that we report progress in the same way consistently. It turns out that doing that immediately revealed some bugs in ImageLib, though the issues are relatively minor, so this bug will also contain the patches that fix those bugs and make the initial set of assertions pass. Hopefully we can continue to make these assertions stronger over time.

Comment 1

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

Attachment: (Part 1) - Don't overwrite nsICODecoder's progress with the contained decoder's progress

This is a recent regression from the refactorings in bug 910441. It got detected immediately by these assertions.

Comment 2

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

Attachment: (Part 2) - Don't send DECODE_STOPPED or ONLOAD_UNBLOCKED for size decodes

We don't send DECODE_STARTED or ONLOAD_BLOCKED for size decodes, so we shouldn't send DECODE_STOPPED or ONLOAD_UNBLOCKED, either.

Comment 3

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

Attachment: (Part 3) - Record REQUEST* notifications for imgTools decodes

We weren't recording REQUEST_STARTED and REQUEST_STOPPED for imgTools decodes.

Comment 4

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

Attachment: (Part 4) - Check consistency whenever progress changes in ProgressTracker

This is the patch where we actually add the consistency checks. These are the strongest preconditions I could determine right now; we may be able to make these stronger over time.

I didn't |#ifdef DEBUG| CheckProgressConsistency because, since it contains only |if| blocks and |MOZ_ASSERT| calls, it should have exactly the behavior we want already: it'll be optimized away in builds with optimizations on and assertions disabled.

Comment 5

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

Here's a try job:

https://tbpl.mozilla.org/?tree=Try&rev=b93b7a047881

Comment 6

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

Attachment: (Part 4) - Make resniffed multipart requests progress like normal requests

Looks like we need an additional tweak here. Resniffed multipart requests weren't progressing the same as normal requests. I also strengthened the assertion in imgStatusTracker::SetIsMultipart.

Comment 7

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

Attachment: (Part 5) - Check consistency whenever progress changes in ProgressTracker

The old part 4 is now part 5. I've tweaked how the assertions behave w.r.t. multipart requests. Nothing else has changed.

Comment 8

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

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=a35286609a01

Comment 9

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

Thanks for the review! Tests look good, too.

Comment 10

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/2750a8bef37d
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a54f722b1cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e51b39c7d64b
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b3a6fee879c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c786bdac4406

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2750a8bef37d
https://hg.mozilla.org/mozilla-central/rev/4a54f722b1cd
https://hg.mozilla.org/mozilla-central/rev/e51b39c7d64b
https://hg.mozilla.org/mozilla-central/rev/0b3a6fee879c
https://hg.mozilla.org/mozilla-central/rev/c786bdac4406