Closed Bug 1324642 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file)

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.
Attached patch patchSplinter Review
Attachment #8820144 - Flags: review?(n.nethercote)
Attachment #8820144 - Flags: review?(continuation)
Comment on attachment 8820144 [details] [diff] [review]
patch

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

Sounds reasonable. Thanks.
Attachment #8820144 - Flags: review?(continuation) → review+
Attachment #8820144 - Flags: review?(n.nethercote)
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0a59cbeb48
Move assert from bug 1323207 from nsImageLoadingContent::Notify to ScriptedNotificationObserver. r=continuation
You know this code far better than I do so I trust this is sensible :)
https://hg.mozilla.org/mozilla-central/rev/0f0a59cbeb48
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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/
I think we're backing out the change that required this change, so we should probably get rid of this, too.
You need to log in before you can comment on or make changes to this bug.