Closed Bug 1103157 Opened 5 years ago Closed 5 years ago

nsImageLoadingContent should replay notifications to new observers

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files)

When debugging another problem, I noticed that nsImageLoadingContent does not replay notifications to new observers. What this means in practice is that nsImageFrame often never gets notifications that a naive reading of the code would lead you to expect it should get - for example, it often never gets FRAME_COMPLETE, so when it gets FRAME_UPDATE it always uses the code path that is intended for the case where we're displaying a partial invalidation of an incomplete frame.

I am frankly amazed that we don't have more problems because of this.

The fix I'm going to use in this bug is to have nsImageLoadingContent replay the notifications. It would be better to lean on ImageLib itself to do this, but I've already explored this approach and it requires bigger changes that I don't want to wait for to get this fixed.
Here's the setup, so we can replay as many notifications as possible - we add HAS_TRANSPARENCY and IS_ANIMATED equivalents to the imgIRequest::STATUS_* flags.
Attachment #8527060 - Flags: review?(tnikkel)
Here's the patch which makes us actually replay the notifications. I've verified that this fixes the "nsImageFrame often never receives FRAME_COMPLETE" issue that I mentioned above.
Attachment #8527064 - Flags: review?(tnikkel)
Attachment #8527060 - Flags: review?(tnikkel) → review+
Comment on attachment 8527064 [details] [diff] [review]
(Part 2) - Replay notifications to new nsImageLoadingContent observers

The only concern I have with this is that AddObserver is called from the Init of nsImageFrame and for svg image frames. The frame might not be fully set up at that point, I think it's mParent pointer might not be set yet for example. But still I don't think that will be a problem as the side effects of most of the Notify handles are invalidations, a new frame will invalidate anyways. The other cases don't look problematic.
Attachment #8527064 - Flags: review?(tnikkel) → review+
Thanks for the review!

(In reply to Timothy Nikkel (:tn) from comment #4)
> The only concern I have with this is that AddObserver is called from the
> Init of nsImageFrame and for svg image frames. The frame might not be fully
> set up at that point, I think it's mParent pointer might not be set yet for
> example.

That's a good point; it might be worth thinking about replaying these notifications asynchronously. Avoiding duplicate notifications gets tricky in that case, though. (We handle it for real imgRequestProxy observers by blocking *all* notifications until the async replay happens.)

As we discussed on IRC, long term it's quite possible that it'd be better for nsImageFrame et al. not to be imgINotificationObservers at all.

Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eba0d88ca0f5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5a5ae0ef5714
https://hg.mozilla.org/mozilla-central/rev/eba0d88ca0f5
https://hg.mozilla.org/mozilla-central/rev/5a5ae0ef5714
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1110085
You need to log in before you can comment on or make changes to this bug.