1311841 - mozilla::image::ProgressTracker::SyncNotifyProgress called during InterruptCallback

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Andrew McCreight [:mccr8]]

It looks like painting can decoding to finish, which ends up calling RasterImage::NotifyProgress(), which calls ProgressTracker::SyncNotifyProgress(), and the imgINotificationObserver can be implemented in JS. I'm not sure why the notification here is sync instead of async.

bp-daeb3af0-dbfe-44b8-9d64-b2ce72161018
bp-69b0b959-89eb-43dc-ba2e-c38462161020
bp-aa3f3c70-9324-409b-84ab-dce692161015

Comment 1

[(no longer active)]

Hmm, imgINotificationObserver is marked as builtinclass...  Does that not mean that it can't be implemented in JS?

Comment 2

[Timothy Nikkel (:tnikkel)]

But there is imgITools::createScriptedObserver and ScriptedNotificationObserver which are used to wrap imgINotificationObserver and let js observe these notifications. Looks like in tree we only use that in tests, but there are a few addons that seems to use it. If we could make this a testing builds only feature that would be good, but I don't know if we can because of addons.

Unless I'm missing something obvious, it was be a good idea to make these notifications during painting async even before this bug came about. I wrote patches for that and they seem to be green on try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0dd792d809d43a6df6f825451ba21a7f2089ff

Comment 3

[Timothy Nikkel (:tnikkel)]

Jeff, any reason you can think of why we would need sync notifications during painting?

Comment 4

[Jeff Muizelaar [:jrmuizel]]

No

Comment 6

[Nicholas Nethercote [inactive]]

This is the #11 topcrash in Aurora 20161216101750.

Comment 7

[Nicholas Nethercote [inactive]]

This is the #2 topcrash in Nightly 20161219030207.

tnikkel, any ideas on who should investigate/fix this?

Comment 8

[Timothy Nikkel (:tnikkel)]

I wrote the necessary patches as described in bug 1317562 today, just waiting on try before requesting review.

Comment 9

[Nicholas Nethercote [inactive]]

Excellent! Thank you.

Comment 10

[Andrew McCreight [:mccr8]]

If I understand correctly, bug 1324642 should also change the signature (again) to ScriptedNotificationObserver::Notify, and hopefully be less frequent.

Comment 11

[Timothy Nikkel (:tnikkel)]

All blocking bugs resolved, this should be fixed now.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Are we going to be able to backport said fixes to 52 as well?

Comment 13

[Timothy Nikkel (:tnikkel)]

Had to backout bug 1317562 for a talos regression. The fix is not looking quick and easy. Might be best to come up with a different plan than uplift for 52.

Comment 14

[Timothy Nikkel (:tnikkel)]

I don't think we need to uplift these fixes.