1103157 - nsImageLoadingContent should replay notifications to new observers

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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.

Comment 1

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

Attachment: (Part 1) - Add HAS_TRANSPARENCY and IS_ANIMATED imgIRequest flags

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.

Comment 2

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

Attachment: (Part 2) - Replay notifications to new nsImageLoadingContent observers

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.

Comment 3

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

Here's a try job:

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

Comment 4

[Timothy Nikkel (:tnikkel)]

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.

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eba0d88ca0f5
https://hg.mozilla.org/mozilla-central/rev/5a5ae0ef5714