1084136 - Make imgStatusTracker state as simple and monotonic as possible

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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.

Comment 1

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

Attachment: (Part 1) - Clean up image state flags

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.

Comment 2

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

Attachment: (Part 2) - Add FLAG_UNBLOCK_ONLOAD and never unset FLAG_BLOCK_ONLOAD

Comment 3

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

Attachment: (Part 3) - Don't reset image status flags on error

Comment 4

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

Attachment: (Part 4) - Record STATUS_DECODE_COMPLETE even if there's an error

Comment 5

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

Attachment: (Part 5) - Don't reset STATUS_DECODE_STARTED when decoding finishes

Comment 6

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

Attachment: (Part 5) - Don't reset any imgStatusTracker state when discarding

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.

Comment 7

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

Attachment: (Part 6) - Don't reset any imgStatusTracker state when discarding

Whoops, this one also got misnumbered.

Comment 8

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

Attachment: (Part 7) - Treat being multipart as an image state

Comment 9

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

Attachment: (Part 8) - Treat receiving the last part of a multipart image as an image state

Comment 10

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

Attachment: (Part 9) - Don't track whether we've ever been decoded separately

Comment 11

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

Attachment: (Part 10) - Materialize image status from image state when needed

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.

Comment 12

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

Attachment: (Part 11) - Don't treat FLAG_REQUEST_STARTED specially

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.

Comment 13

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

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.

Comment 14

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

Here's a try job for the entire patch series:

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

Comment 15

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

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

Comment 16

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

Here's through part 4:

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

Comment 17

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

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

Comment 18

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

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.

Comment 19

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

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.

Comment 20

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

Attachment: (Part 2) - Add FLAG_UNBLOCK_ONLOAD and never unset FLAG_BLOCK_ONLOAD

Here's the new version of the patch with the removed assert.

Comment 21

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

Attachment: (Part 9) - Don't track whether we've ever been decoded separately

The original version of this patch had a mistake in the code that determined whether we should send invalidations. I fixed that.

Comment 22

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

Attachment: (Part 10) - Materialize image status from image state when needed

Rebased on top of part 9.

Comment 23

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

Attachment: (Part 11) - Don't treat FLAG_REQUEST_STARTED specially

Rebased on top of part 9.

Comment 24

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

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

Comment 25

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

(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.

Comment 26

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

Attachment: (Part 2) - Add FLAG_UNBLOCK_ONLOAD and never unset FLAG_BLOCK_ONLOAD

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.

Comment 27

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

Here's a new try job for part 2:

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

Comment 28

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

Here's a try job through part 4, since hopefully part 2 is now green:

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

Comment 29

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

Attachment: (Part 2) - Add FLAG_UNBLOCK_ONLOAD and never unset FLAG_BLOCK_ONLOAD

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.)

Comment 30

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

Attachment: (Part 6) - Don't reset any imgStatusTracker state when discarding

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.

Comment 31

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

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 32

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 33

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

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=f245578c4fa4

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a28e572e9919
https://hg.mozilla.org/mozilla-central/rev/187394a214ea
https://hg.mozilla.org/mozilla-central/rev/e7ecd25d7e7c
https://hg.mozilla.org/mozilla-central/rev/7816c3056b65
https://hg.mozilla.org/mozilla-central/rev/00faf185fa5b
https://hg.mozilla.org/mozilla-central/rev/443d2ef1ba03
https://hg.mozilla.org/mozilla-central/rev/70b9dd23c3f9
https://hg.mozilla.org/mozilla-central/rev/e6a962ab3605
https://hg.mozilla.org/mozilla-central/rev/e0b56aa7e3b0
https://hg.mozilla.org/mozilla-central/rev/179a94abc0b3
https://hg.mozilla.org/mozilla-central/rev/f245578c4fa4