1478707 - Image memory reporters may not be thread-safe, causing crashes on Android

Status: RESOLVED INVALID

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

Description

[Andrew Sutherland [:asuth] (he/him)]

In https://bugzilla.mozilla.org/show_bug.cgi?id=1468541#c38 I was looking into some corrupt stack crashes on Fennec and noticed that it appears that RasterImage::CollectSizeOfSurfaces may call into mFrameAnimator->CollectSizeOfCompositingSurfaces on the main thread, but mFrameAnimator might get cleared off the main thread, leaving the main thread in a precarious and potentially crash-prone state.

The particular excerpt from that comment is:
- RasterImage::CollectSizeOfSurfaces conditionally calls mFrameAnimator->CollectSizeOfCompositingSurfaces
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/image/RasterImage.cpp#705
- But mFrameAnimator is nullable in case of error (in DoError) which means that maybe mFrameAnimator was not null when the main thread started to do stuff, but it got freed out from under the reporter.
https://searchfox.org/mozilla-central/rev/a80651653faa78fa4dfbd238d099c2aad1cec304/image/RasterImage.cpp#1615

Is there someone with a better understanding of how graphics threads/objects work available to figure out if this is a reasonable suspicion and/or if there could be other image/surface reporters going rogue?

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Andrew will take this.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

mFrameAnimator should not be cleared off the main thread. RasterImage::DoError should post a runnable to run on the main thread if it is called off the main thread. I will investigate further.

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

After some offline discussions, and further investigations documented in the parent bug 1468541, I think we can safely rule this possibility out.