Closed Bug 1098108 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

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.
This is a recent regression from the refactorings in bug 910441. It got detected immediately by these assertions.
Attachment #8521921 - Flags: review?(tnikkel)
We don't send DECODE_STARTED or ONLOAD_BLOCKED for size decodes, so we shouldn't send DECODE_STOPPED or ONLOAD_UNBLOCKED, either.
Attachment #8521926 - Flags: review?(tnikkel)
We weren't recording REQUEST_STARTED and REQUEST_STOPPED for imgTools decodes.
Attachment #8521927 - Flags: review?(tnikkel)
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.
Attachment #8521928 - Flags: review?(tnikkel)
Attachment #8521921 - Flags: review?(tnikkel) → review+
Attachment #8521926 - Flags: review?(tnikkel) → review+
Attachment #8521927 - Flags: review?(tnikkel) → review+
Attachment #8521928 - Flags: review?(tnikkel) → review+
Blocks: 1098490
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.
Attachment #8522543 - Flags: review?(tnikkel)
The old part 4 is now part 5. I've tweaked how the assertions behave w.r.t. multipart requests. Nothing else has changed.
Attachment #8521928 - Attachment is obsolete: true
Attachment #8522543 - Flags: review?(tnikkel) → review+
Thanks for the review! Tests look good, too.
Depends on: 1133356
No longer depends on: 1133356
You need to log in before you can comment on or make changes to this bug.