Closed Bug 1084136 Opened 5 years ago Closed 5 years ago

Make imgStatusTracker state as simple and monotonic as possible

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(11 files, 8 obsolete files)

14.69 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.65 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
1.35 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.78 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.40 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.70 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.08 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
15.83 KB, patch
tnikkel
: review+
jrmuizel
: superreview+
Details | Diff | Splinter Review
2.92 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
8.76 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.61 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
To make imgStatusTracker easier to reason about, it's important to minimize the amount of state an imgStatusTracker holds. It's also important that that state be as monotonic as possible - in other words, once we set a flag, we should avoid unsetting it, so that state transitions are one-way and irrevocable.

It turns out that these two goals are pretty closely related - there's a lot of state on imgStatusTracker that only exists because not all flags are monotonic, and if we make them monotonic, that state disappears.

There's one important exception to monotonicity that I'm explicitly going to allow: it's fine to reset state when we transition to the next part of a multipart/x-mixed-replace image. There's not a simple way around this with the current architecture, and this is something of an edge case on the modern web, so it's somewhat more acceptable for it to involve some complexity.
This is split into a ton of different parts to make it easier to localize the source of any bugs that creep in, so I won't provide a detailed description for every part. Hopefully these patches are fairly self-explanatory given the motivation in comment 0, though.
Attachment #8506512 - Flags: review?(tnikkel)
In this patch imgStatusTracker::RecordDiscard() ended up not doing anything, so I removed it. That led me to remove imgStatusTrackerObserver::OnDiscard(), because its implementation just called imgStatusTracker::RecordDiscard(). It wasn't used anyway; decoders don't know about discarding at all.
Attachment #8506523 - Flags: review?(tnikkel)
Whoops, this one also got misnumbered.
Attachment #8506524 - Flags: review?(tnikkel)
Attachment #8506523 - Attachment is obsolete: true
Attachment #8506523 - Flags: review?(tnikkel)
At this point in the patch series it's redundant to track image "status" (imgIRequest::STATUS_* flags) separately from image "state", so we shift to only tracking state and compute the status when we need it.

It became clear in the process of writing this patch that imgIRequest::STATUS_LOAD_PARTIAL is totally unused, since it's never referred to outside of imgStatusTracker and can't become set within imgStatusTracker. I therefore went ahead and removed it from the code base.
Attachment #8506532 - Flags: review?(tnikkel)
Attachment #8506532 - Flags: superreview?(jmuizelaar)
The old code treated FLAG_REQUEST_STARTED specially in various ways; we went out of our way to avoid unsetting it. Since the flags are all monotonic now, outside of the multipart/x-mixed-replace case (in which FLAG_REQUEST_STARTED is handled explicitly in imgStatusTracker::OnStartRequest), there's no reason to treat FLAG_REQUEST_STARTED specially any more.
Attachment #8506533 - Flags: review?(tnikkel)
There's still some unfinished work to be done in followups. In part 11, I added two "XXX(seth)" comments about situations where we chose a subset of the flags to reset or filter. I want to make sure those subsets actually make sense, but I'd prefer to handle that in a different bug.

Also, I'd like us to remove imgStatusTracker::mInvalidRect as well. That's complex enough that it too should be handled in a different bug.

The followups will be additional blockers of bug 910441.
Here's a try job for the entire patch series:

https://tbpl.mozilla.org/?tree=Try&rev=8da3d001eae8
I'm skeptical that changes of this complexity are correct the first time, so here are some preemptive additional try jobs for only part of the series. Here's through part 2:

https://tbpl.mozilla.org/?tree=Try&rev=4984fa5fe114
And here's through part 6. My best guess is that the first 6 parts are most likely to introduce bugs:

https://tbpl.mozilla.org/?tree=Try&rev=4086cf2d5f43
So those try jobs are *very* orange, but the situation actually doesn't look too bad. Looks like the vast majority (all?) of the orangeness comes from a bug in part 2.
Attachment #8506512 - Flags: review?(tnikkel) → review+
So I investigated those oranges with a try job with some additional logging, and it turns out that they're triggered by the fact that we unconditionally block onload in OnStartDecode(), even if we're just doing a redecode.

That's not actually a good idea. It obviously potentially slows page load, and it doesn't add any value, because blocking onload is about ensuring that we have loaded the data from the network (and can determine the size of the image, but in practice that's subsumed by the other requirement); it's *not* about whether or not the image is decoded. If we have the image in cache, but the decoded data has been discarded, we shouldn't block onload. In general, the modern imagelib attitude is that discarding and redecoding is normal and is to some extent an implementation detail of imagelib.

Based on these results, I'm going to remove that assert.
Here's the new version of the patch with the removed assert.
Attachment #8507299 - Flags: review?(tnikkel)
Attachment #8506514 - Attachment is obsolete: true
Attachment #8506514 - Flags: review?(tnikkel)
The original version of this patch had a mistake in the code that determined whether we should send invalidations. I fixed that.
Attachment #8507301 - Flags: review?(tnikkel)
Attachment #8506528 - Attachment is obsolete: true
Attachment #8506528 - Flags: review?(tnikkel)
Rebased on top of part 9.
Attachment #8507302 - Flags: review?(tnikkel)
Attachment #8506532 - Attachment is obsolete: true
Attachment #8506532 - Flags: superreview?(jmuizelaar)
Attachment #8506532 - Flags: review?(tnikkel)
Rebased on top of part 9.
Attachment #8507303 - Flags: review?(tnikkel)
Attachment #8506533 - Attachment is obsolete: true
Attachment #8506533 - Flags: review?(tnikkel)
Attachment #8507302 - Flags: superreview?(jmuizelaar)
Here's a try job for the new version of part 2. Still one or two orange tests, but *much* better.

https://tbpl.mozilla.org/?tree=Try&rev=63be725a133d
(In reply to Seth Fowler [:seth] from comment #13)
> Also, I'd like us to remove imgStatusTracker::mInvalidRect as well. That's
> complex enough that it too should be handled in a different bug.

I just filed this bug and uploaded the patch; it's bug 1084679.
OK, based on local testing I think I've resolved the remaining oranges. The problem is that sometimes we speculatively unblock onload (i.e., without being sure that we've blocked it first). That was previously OK since we were just clearing the FLAG_BLOCK_ONLOAD bit, so if it wasn't already set there was no effect. However, now that FLAG_UNBLOCK_ONLOAD is a separate bit, we need to explicitly check that we've already set FLAG_BLOCK_ONLOAD before setting FLAG_UNBLOCK_ONLOAD.
Attachment #8508911 - Flags: review?(tnikkel)
Attachment #8507299 - Attachment is obsolete: true
Attachment #8507299 - Flags: review?(tnikkel)
Here's a new try job for part 2:

https://tbpl.mozilla.org/?tree=Try&rev=72d54a28ef72
Here's a try job through part 4, since hopefully part 2 is now green:

https://tbpl.mozilla.org/?tree=Try&rev=540642ef3626
Attachment #8506516 - Flags: review?(tnikkel) → review+
Attachment #8506515 - Flags: review?(tnikkel) → review+
Attachment #8506518 - Flags: review?(tnikkel) → review+
Attachment #8506524 - Flags: review?(tnikkel) → review+
Attachment #8506525 - Flags: review?(tnikkel) → review+
Attachment #8506527 - Flags: review?(tnikkel) → review+
Attachment #8507301 - Flags: review?(tnikkel) → review+
Attachment #8507303 - Flags: review?(tnikkel) → review+
So it turns out that there was one remaining orange with part 2 - the 'test_bug347174_write.html' failure visible in the previous try job. That turned out to mostly be an issue with the test rather than the code in part 2; it looks like that test got busted years ago and just happened to continue to work because of the specific order in which ImageLib sent its notifications.

I uploaded a fix for the test in bug 1087615.

However, debugging that issue did make me change one thing about part 2: we should send UnblockOnload before OnStopDecode and OnStopRequest, so that observers that want to fire events for those notifications can do so right away instead of waiting for UnblockOnload. I've made that change in this version of the patch.

This version of the patch, based on top of the patch in bug 1087615, looks to be green on try, so I don't think I'll need to revisit this patch again. (Excepting review comments, of course.)
Attachment #8509875 - Flags: review?(tnikkel)
Attachment #8508911 - Attachment is obsolete: true
Attachment #8508911 - Flags: review?(tnikkel)
I didn't change any C++ code in this version of part 6, but I changed 'browser_bug666317.js', which is a test that we discard images on memory pressure events. The test previously checked whether the imgIRequest had STATUS_FRAME_COMPLETE to determine whether it was decoded; if STATUS_FRAME_COMPLETE was not set, then the image had been discarded.

Since in this part we don't reset STATUS_FRAME_COMPLETE (or any other flags) when discarding, 'browser_bug666317.js' had to be written different. I changed it to explicitly listen for an OnDiscard event instead, and it now passes.
Attachment #8509913 - Flags: review?(tnikkel)
Attachment #8506524 - Attachment is obsolete: true
This new 'all' try job contains all 11 parts in this bug:

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

Based upon the results, it looks like these patches are in good shape.
Comment on attachment 8507302 [details] [diff] [review]
(Part 10) - Materialize image status from image state when needed

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

I have no objection to this kind of thing.
Attachment #8507302 - Flags: superreview?(jmuizelaar) → superreview+
Blocks: 1089880
No longer blocks: 1089880
Attachment #8509913 - Flags: review?(tnikkel) → review+
Attachment #8507302 - Flags: review?(tnikkel) → review+
Attachment #8509875 - Flags: review?(tnikkel) → review+
Blocks: 1100253
No longer blocks: 1100253
Depends on: 1100253
Blocks: 1042926
Depends on: 1174510
You need to log in before you can comment on or make changes to this bug.