It first showed up in 23.0a1/20130405 then in 23.0a1/20130408. The regression range is: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c232bec6974d&tochange=55f9e3e3dae7 It's likely a regression from bug 854803. Bug 856602 may have partially fixed it. Signature mozilla::image::RasterImage::IsDecodeFinished() More Reports Search UUID c768c4e6-55ea-4340-a0b9-21b5d2130409 Date Processed 2013-04-09 06:12:40 Uptime 220 Last Crash 26.1 minutes before submission Install Age 25.8 minutes since version was first installed. Install Time 2013-04-09 05:46:28 Product Firefox Version 23.0a1 Build ID 20130408030928 Release Channel nightly OS Windows NT OS Version 6.1.7601 Service Pack 1 Build Architecture x86 Build Architecture Info GenuineIntel family 6 model 58 stepping 9 Crash Reason EXCEPTION_ACCESS_VIOLATION_READ Crash Address 0x24 App Notes AdapterVendorID: 0x8086, AdapterDeviceID: 0x0166, AdapterSubsysID: 056d1028, AdapterDriverVersion: 8.15.10.2696 D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ Processor Notes sp-processor06.phx1.mozilla.com_9594:2008 EMCheckCompatibility True Adapter Vendor ID 0x8086 Adapter Device ID 0x0166 Total Virtual Memory 2147352576 Available Virtual Memory 1643831296 System Memory Use Percentage 45 Available Page File 4168970240 Available Physical Memory 1541476352 Frame Module Signature Source 0 xul.dll mozilla::image::RasterImage::IsDecodeFinished image/src/RasterImage.cpp:3295 1 xul.dll mozilla::image::RasterImage::DecodePool::DecodeJob::Run image/src/RasterImage.cpp:3691 2 xul.dll nsThreadPool::Run xpcom/threads/nsThreadPool.cpp:194 3 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:627 4 xul.dll nsThread::ThreadFunc xpcom/threads/nsThread.cpp:265 5 nss3.dll _PR_NativeRunThread nsprpub/pr/src/threads/combined/pruthr.c:395 6 nss3.dll pr_root nsprpub/pr/src/md/windows/w95thred.c:90 7 msvcr100.dll _callthreadstartex f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314 8 msvcr100.dll _threadstartex f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292 9 kernel32.dll BaseThreadInitThunk 10 ntdll.dll __RtlUserThreadStart 11 ntdll.dll _RtlUserThreadStart More reports at: https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Aimage%3A%3ARasterImage%3A%3AIsDecodeFinished%28%29

Proposed patch.

22 may be affected soon, since we're tracking bug 854803

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=8f32e8690d20

(In reply to Alex Keybl [:akeybl] from comment #2) > 22 may be affected soon, since we're tracking bug 854803 Just don't uplift it without this patch. It's already ready to go. If you need me to arrange for the review to be fast-tracked just let me know.

I crashed at this webcam URL: http://cryptogasm.com/webcams/webcam.php?id=20507 https://crash-stats.mozilla.com/report/index/bp-4bb73f6a-4782-46fb-a165-02a232130413 Looks like we can also crash [@ mozilla::image::RasterImage::DecodePool::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::RasterImage::DecodePool::DecodeType, unsigned int) ] : https://crash-stats.mozilla.com/report/index/bp-8418ba45-7413-4b7f-850f-a20312130413 (I hit these two crashes on the same page.)

Comment on attachment 737031 [details] [diff] [review] Don't assume we have mImageRequest in RasterImage::IsDecodeFinished. Review of attachment 737031 [details] [diff] [review]: ----------------------------------------------------------------- I believe this is correct after having reviewed bug 854803 too. ::: image/src/RasterImage.cpp @@ +3286,5 @@ > } > } else if (mDecoder->GetDecodeDone()) { > return true; > } > can you remove this erroneous whitespace too please

Thanks for the review, Joe. Will post an updated patch shortly.

Removed the erroneous whitespace, making the world a tiny bit better. =)

Try looks OK so we're just waiting on inbound to open.

This seems to have led to some Ts Paint regressions?

(In reply to Boris Zbarsky (:bz) from comment #12) > This seems to have led to some Ts Paint regressions? Pretty difficult for me to understand how since this patch just adds a null check. I notice that bug 861731 is also in the regression range. It seems like a more likely candidate to me.

Ack, hmm, I see there's another email where the regression range is narrowed down to this commit. I am puzzled as to how this could have any significant effect, since whenever behavior before and after this patch would differ, we should crash or assert.

I suspect that Bug 857876's patch makes the null check here unnecessary, although I haven't investigated deeply enough to be certain yet. My reasoning for asserting mDecodeRequest instead of explicitly null checking it is that decode requests should outlive their decoder. InitDecoder() creates a DecodeRequest if there isn't one, and the only places that mDecodeRequest is set to null (Discard() and OnNewSourceData()) assert !mDecoder. Assuming these invariants hold, it should be impossible to have a decoder but no decode request. My understanding is that this invariant was broken in the multipart stream case due to Bug 857876. I need to do some debugging to make sure my assumptions reflect reality; however, I won't have cycles to do that until this weekend.

That sounds reasonable. I'd prefer caution w.r.t. Aurora, so what I'm going to do is nominate this for uplift, and then we can remove the null check again on m-c once we confirm it's safe. (Note that it was confirmed elsewhere that the regression range mentioned above is wrong; this patch doesn't cause any performance regressions.)

Comment on attachment 737674 [details] [diff] [review] Don't assume we have mImageRequest in RasterImage::IsDecodeFinished. [Approval Request Comment] Bug caused by (feature/regressing bug #): 854803 User impact if declined: Potential crashes. Testing completed (on m-c, etc.): On m-c since 2013-04-16 with no issues. Risk to taking this patch (and alternatives if risky): Zero risk. Just adds a null check. String or IDL/UUID changes made by this patch: None.

Not able to reproduce this issue on FF 23b1 on: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 Mozilla/5.0 (X11; Linux i686; rv:23.0) Gecko/20100101 Firefox/23.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0 Build ID: 20130625125232 Here are the current stats for these signatures from Soccoro for last 2 weeks: https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=mozilla%3A%3Aimage%3A%3ARasterImage%3A%3AIsDecodeFinished%28%29&reason_type=contains&date=06%2F26%2F2013%2009%3A17%3A48&range_value=2&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Aimage%3A%3ARasterImage%3A%3AIsDecodeFinished%28%29 https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=mozilla%3A%3Aimage%3A%3ARasterImage%3A%3ADecodePool%3A%3ADecodeSomeOfImage%28mozilla%3A%3Aimage%3A%3ARasterImage%2A%2C%20mozilla%3A%3Aimage%3A%3ARasterImage%3A%3ADecodePool%3A%3ADecodeType%2C%20unsigned%20int%29&reason_type=contains&date=06%2F26%2F2013%2009%3A18%3A32&range_value=2&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Aimage%3A%3ARasterImage%3A%3ADecodePool%3A%3ADecodeSomeOfImage%28mozilla%3A%3Aimage%3A%3ARasterImage%2A%2C%20mozilla%3A%3Aimage%3A%3ARasterImage%3A%3ADecodePool%3A%3ADecodeType%2C%20unsigned%20int%29 Not sure this is fix even if I was not able to reproduce this crash, as long there are still crashes on FF 22b5 and 22b6 on Soccoro.

Scoobidiver, can you please check to see if this is still a concern for Firefox 22 and 23?

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #20) > Scoobidiver, can you please check to see if this is still a concern for > Firefox 22 and 23? There have been 21 crashes for the last four weeks in fixed versions. It's likely some edge cases not handled by the patch but let's consider it as fixed according to the pre-patch crash volume.