859718 - crash in mozilla::image::RasterImage::DecodePool::DecodeJob::Run @ mozilla::image::RasterImage::IsDecodeFinished

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Scoobidiver (away)]

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

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't assume we have mImageRequest in RasterImage::IsDecodeFinished.

Proposed patch.

Comment 2

[Alex Keybl [:akeybl]]

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

Comment 3

[Seth Fowler [:seth] [:s2h]]

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

Comment 4

[Seth Fowler [:seth] [:s2h]]

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

Comment 5

[(not currently active) Ted Mielczarek]

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 6

[Joe Drew (not getting mail)]

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

Comment 7

[Seth Fowler [:seth] [:s2h]]

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

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: Don't assume we have mImageRequest in RasterImage::IsDecodeFinished.

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

Comment 9

[Seth Fowler [:seth] [:s2h]]

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

Comment 10

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/eee1831a1094

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eee1831a1094

Comment 12

[Boris Zbarsky [:bzbarsky]]

This seems to have led to some Ts Paint regressions?

Comment 13

[Seth Fowler [:seth] [:s2h]]

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

Comment 14

[Seth Fowler [:seth] [:s2h]]

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.

Comment 15

[Robert Lickenbrock [:rclick]]

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.

Comment 16

[Seth Fowler [:seth] [:s2h]]

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 17

[Seth Fowler [:seth] [:s2h]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9cf96539fe26

Comment 19

[Mihai Morar, (:MihaiMorar)]

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.

Comment 20

[u279076]

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

Comment 21

[Scoobidiver (away)]

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