Closed Bug 1311841 Opened 4 years ago Closed 4 years ago

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


(Core :: ImageLib, defect, P3)




Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox53 --- fixed


(Reporter: mccr8, Assigned: tnikkel)



(Keywords: crash, Whiteboard: [gfx-noted])

Crash Data

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.

Hmm, imgINotificationObserver is marked as builtinclass...  Does that not mean that it can't be implemented in JS?
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
Whiteboard: [gfx-noted]
Priority: -- → P3
Jeff, any reason you can think of why we would need sync notifications during painting?
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(jmuizelaar)
Depends on: 1317551
Depends on: 1317552
Depends on: 1317554
Depends on: 1317556
Depends on: 1317558
Depends on: 1317559
Depends on: 1317562
Duplicate of this bug: 1324308
Crash Signature: [@ JS::Heap<T>::operator JSObject* const& ] [@ nsXPCWrappedJS::GetJSObject ] → [@ JS::Heap<T>::operator JSObject* const& ] [@ nsXPCWrappedJS::GetJSObject ] [@ nsImageLoadingContent::Notify]
This is the #11 topcrash in Aurora 20161216101750.
This is the #2 topcrash in Nightly 20161219030207.

tnikkel, any ideas on who should investigate/fix this?
Flags: needinfo?(tnikkel)
I wrote the necessary patches as described in bug 1317562 today, just waiting on try before requesting review.
Flags: needinfo?(tnikkel)
If I understand correctly, bug 1324642 should also change the signature (again) to ScriptedNotificationObserver::Notify, and hopefully be less frequent.
Crash Signature: [@ JS::Heap<T>::operator JSObject* const& ] [@ nsXPCWrappedJS::GetJSObject ] [@ nsImageLoadingContent::Notify] → [@ JS::Heap<T>::operator JSObject* const& ] [@ nsXPCWrappedJS::GetJSObject ] [@ nsImageLoadingContent::Notify][@ ScriptedNotificationObserver::Notify]
All blocking bugs resolved, this should be fixed now.
Closed: 4 years ago
Resolution: --- → FIXED
Are we going to be able to backport said fixes to 52 as well?
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Keywords: crash
Target Milestone: --- → mozilla53
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.
Resolution: FIXED → ---
Depends on: 1331574
I don't think we need to uplift these fixes.
Flags: needinfo?(tnikkel)
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.