Closed Bug 1311841 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: ImageLib, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- unaffected
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: mccr8, Assigned: tnikkel)

References

Details

(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.

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
Whiteboard: [gfx-noted]
Priority: -- → P3
Jeff, any reason you can think of why we would need sync notifications during painting?
Flags: needinfo?(jmuizelaar)
No
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)
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?
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1331574
I don't think we need to uplift these fixes.
Flags: needinfo?(tnikkel)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.