Closed Bug 1311841 Opened 3 years ago Closed 3 years ago
Tracker::Sync Notify Progress called during Interrupt Callback
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
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 https://treeherder.mozilla.org/#/jobs?repo=try&revision=dc0dd792d809d43a6df6f825451ba21a7f2089ff
Jeff, any reason you can think of why we would need sync notifications during painting?
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?
I wrote the necessary patches as described in bug 1317562 today, just waiting on try before requesting review.
Excellent! Thank you.
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Are we going to be able to backport said fixes to 52 as well?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't think we need to uplift these fixes.
Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.