1128971 - crash in mozilla::image::ProgressTracker::SyncNotify(imgRequestProxy*)

Status: RESOLVED WORKSFORME

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

Description

[Petruta Horea [:phorea], Desktop QA]

This bug was filed from the Socorro interface and is 
report bp-648e5ee7-6922-457e-8a52-a19da2150203.
=============================================================

Reproduced on loading http://download.wavetlan.com/SVV/Media/HTTP/MP4/ConvertedFiles/QuickTime/QuickTime_test10_3m2s_AVC_VBR_762kbps_176x144_25fps_AAC-LCv4_CBR_320kbps_Stereo_48000Hz.mp4 - slow buffering on this site, so it could also be a networking issue. (choosing Video/Audio component for now)
I had other tabs opened containing mp4 files, several of them where loading slowly. 

Unfortunately I couldn't reproduce the crash again, so I have no reliable steps.

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

There are only 2 crash reports in the last 28 days both of which have been Firefox 36. I'm going to close this bug. Re-open it if it is still an issue.

Comment 2

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

This appears to have resurfaced on more recent builds:

https://crash-stats.mozilla.com/report/index/4ec21904-fe46-44a4-b1a5-b12c22160928
https://crash-stats.mozilla.com/report/index/4246964c-8248-440b-a59a-4bc3c2161002

It seems as if the underlying image got freed as it sometimes fails the VMT lookup.

Comment 3

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

Attachment: Remove mozilla::image::ProgressTracker's stored raw Image pointer, v1

The conflict this patch tries to avoid is where:
1) RasterImage is getting freed on one thread, its destructor is called and blocks on the mutex lock in ProgressTracker::ResetImage.
2) Another thread holds said mutex and acquires a strong reference to RasterImage, but it is going to be destroyed.
3) Later on it tries to use that reference, but the memory it was stored in either got cleared (hence the 0xe5e5e5e5 pointers observed) and/or reused (hence why some stack traces wander before crashing or continue slightly further on).

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

Ideally we could reduce eliminate the need for Image in ProgressTracker completely because it only uses it for 1) getting the URI, 2) getting the image dimensions in ProgressTracker::SyncNotify and 3) knowing that we have an image at all.

For 1), a strong reference to that object can easily be stored at ProgressTracker construction time.

For 2), the dimensions are only used for an empty/non-empty check. If Image::mError is true, GetWidth/GetHeight will fail, and it will also set the dimensions to non-empty. Thus the only time it is empty is before we get the size dimension.

For 3), I wonder if it is possible that not holding onto the image itself (in one of the async runnables) could potentially change the state machine in a minor/subtle way.

Comment 4

[Daniel Bodea [:danibodea]]

The test page in comment zero is no longer existent and, anyway, the crash appears to be a one-time event (probably intermittent).
@Andrew: do you have any ideas on how this crash could be reproduced? Please NI me if reproduction ideas are provided.

Comment 5

[Timothy Nikkel (:tnikkel)]

Low volume crash -> S3.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.