1324642 - move release assert from bug 1323207 to where we actually call into JS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Timothy Nikkel (:tnikkel)]

Bug 1323207 added a release assert in nsImageLoadingContent::Notify so we can tell if we call JS during painting. But that's far too general, we know that all image notifications that end up in JS have to go through ScriptedNotificationObserver. So assert there instead.

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 8820144 [details] [diff] [review]
patch

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

Sounds reasonable. Thanks.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0a59cbeb48
Move assert from bug 1323207 from nsImageLoadingContent::Notify to ScriptedNotificationObserver. r=continuation

Comment 4

[Nicholas Nethercote [inactive]]

You know this code far better than I do so I trust this is sensible :)

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/0f0a59cbeb48

Comment 6

[Simon Lindholm]

As the author of one of the addons that use ScriptedNotificationObserver*, I take it I need to move away from it to avoid crashes?

The usage of it is here: https://github.com/simonlindholm/toggle-gifs/blob/f4248b0bbe134e6ad0c869aa7419c2cd530551eb/content/content.js#L477-L501
A notification observer is added for every image on receiving a "load" event, to wait for decode, upon which imgIContainer.animated is available. (One of the opt-in settings of the addon is to add a small indicator overlay to all animated images on a page.)

It's not trivial to move away from notification observers here - repeated polling and forced decodes are presumably both too slow, and probably intersection observers too. A plain async notification would be ideal. (I hope to carry the logic over to the implementation of a WebExtension API for image animation at some point, but haven't had the time yet. Hopefully it's not rejected due to obscurity and using old and crufty internal APIs.)

* https://addons.mozilla.org/en-US/firefox/addon/toggle-animated-gifs/

Comment 7

[Andrew McCreight [:mccr8]]

I think we're backing out the change that required this change, so we should probably get rid of this, too.