795942 - crash in skia::`anonymous namespace''::ConvolveHorizontally

Status: RESOLVED DUPLICATE of bug 795940

(Core :: Graphics: Canvas2D, defect)

Description

[Scoobidiver (away)]

It first appeared in 18.0a1/20120930183350. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a680fd777c3b&tochange=31ae286fff78
It's likely a regression from bug 486918.

Signature 	skia::`anonymous namespace''::ConvolveHorizontally<int>(unsigned char const*, skia::ConvolutionFilter1D const&, unsigned char*) More Reports Search
UUID	f21e93b3-8b46-4d5c-ac3e-e7faf2121001
Date Processed	2012-10-01 16:23:10
Uptime	1089
Install Age	23.3 minutes since version was first installed.
Install Time	2012-10-01 15:59:28
Product	Firefox
Version	18.0a1
Build ID	20121001030603
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	0x23069000
App Notes 	
AdapterVendorID: 0x1002, AdapterDeviceID: 0x6818, AdapterSubsysID: 32501682, AdapterDriverVersion: 8.982.0.0
Has dual GPUs. GPU #2: AdapterVendorID2: 0x8086, AdapterDeviceID2: 0x0152, AdapterSubsysID2: 0000000c, AdapterDriverVersion2: 8.15.10.2761D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True
Adapter Vendor ID	0x1002
Adapter Device ID	0x6818
Total Virtual Memory	4294836224
Available Virtual Memory	3660398592
System Memory Use Percentage	35
Available Page File	12851879936
Available Physical Memory	5437710336

Frame 	Module 	Signature 	Source
0 	gkmedias.dll 	skia::`anonymous namespace'::ConvolveHorizontally<1> 	gfx/2d/convolver.cpp:141
1 	gkmedias.dll 	skia::BGRAConvolve2D 	gfx/2d/convolver.cpp:817
2 	gkmedias.dll 	skia::ImageOperations::ResizeBasic 	gfx/2d/image_operations.cpp:518
3 	gkmedias.dll 	skia::ImageOperations::Resize 	gfx/2d/image_operations.cpp:354
4 	gkmedias.dll 	skia::ImageOperations::Resize 	gfx/2d/image_operations.cpp:533
5 	gkmedias.dll 	mozilla::gfx::Scale 	gfx/2d/Scale.cpp:45
6 	xul.dll 	ScaleFrameImage 	image/src/RasterImage.cpp:75
7 	xul.dll 	mozilla::image::RasterImage::ScaleWorker::Run 	image/src/RasterImage.cpp:2689
8 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:612
9 	xul.dll 	nsThread::ThreadFunc 	xpcom/threads/nsThread.cpp:256
10 	nspr4.dll 	_PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:395
11 	nspr4.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:90
12 	msvcr100.dll 	_callthreadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:314
13 	msvcr100.dll 	_threadstartex 	f:\dd\vctools\crt_bld\self_x86\crt\src\threadex.c:292
14 	kernel32.dll 	BaseThreadInitThunk 	
15 	ntdll.dll 	__RtlUserThreadStart 	
16 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=skia%3A%3A%60anonymous+namespace%27%27%3A%3AConvolveHorizontally%3Cint%3E%28unsigned+char+const*%2C+skia%3A%3AConvolutionFilter1D+const%26%2C+unsigned+char*%29

Comment 1

[Joe Drew (not getting mail)]

Attachment: Don't use imgFrames off main thread, and don't rely on them sticking around

I am fairly sure this is because the RasterImage was deleted while the scale request was running. Since we simply hold on to a raw pointer to imgFrames, which are not refcounted, this blew up in our face.

Instead, let's hold on to gfxImageSurfaces, which *are* refcounted, and not rely on imgFrame working cross-thread. Now we get surfaces explicitly when adding a scale request, and only use pointers and sizes on the scaler thread.

Flow of data:

Scale request comes in. Source imgFrame is locked on the main thread, and we explicitly grab a reference to the source imgFrame's gfxImageSurface. That imgFrame is then unlocked, and no further reference is kept to it.

We then create another imgFrame on the main thread, lock it, and grab a reference to that destination gfxImageSurface.

We then pass the pointers for those gfxImageSurface, which have explicit references and therefore are alive for as long as we are, scale, then pass them back to the main thread, which releases the explicit reference to those surfaces.

Comment 2

[Joe Drew (not getting mail)]

Comment on attachment 666740 [details] [diff] [review]
Don't use imgFrames off main thread, and don't rely on them sticking around

For now I'm cancelling this patch, because the patch on bug 759740 should fix this bug too, and it's a lot simpler.

We might still want to do this work, but perhaps not right away.

Comment 3

[Scoobidiver (away)]

(In reply to Joe Drew (:JOEDREW! \o/) from comment #2)
> We might still want to do this work, but perhaps not right away.
Do you want to do it in this bug or a new one?

Comment 4

[Joe Drew (not getting mail)]

Yeah, no need to keep this bug around. The patch in this bug has been incorporated into the one in bug 795940.