671960 - Crash [@ _moz_cairo_surface_get_device_offset ]

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Frame 	Module 	Signature [Expand] 	Source
0 	libxul.so 	_moz_cairo_surface_get_device_offset 	gfx/cairo/cairo/src/cairo-surface.c:1168
1 	libxul.so 	gfxASurface::GetDeviceOffset 	gfx/thebes/gfxASurface.cpp:278
2 	libxul.so 	nsWindow::DrawTo 	widget/src/android/nsWindow.cpp:941
3 	libxul.so 	nsWindow::OnDraw 	widget/src/android/nsWindow.cpp:1013
4 	libxul.so 	nsWindow::OnGlobalAndroidEvent 	widget/src/android/nsWindow.cpp:830
5 	libxul.so 	nsAppShell::ProcessNextNativeEvent 	widget/src/android/nsAppShell.cpp:410
6 	libxul.so 	nsBaseAppShell::DoProcessNextNativeEvent 	widget/src/xpwidgets/nsBaseAppShell.cpp:172
7 	libxul.so 	nsBaseAppShell::OnProcessNextEvent 	widget/src/xpwidgets/nsBaseAppShell.cpp:312
8 	libxul.so 	mozilla::dom::ContentParent::OnProcessNextEvent 	dom/ipc/ContentParent.cpp:976
9 	libxul.so 	nsThread::ProcessNextEvent 	nsTArray.h:139
10 	libxul.so 	NS_ProcessNextEvent_P 	objdir/xpcom/build/nsThreadUtils.cpp:245
11 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:111
12 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
13 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:511
14 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:191
15 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:223
16 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3688
17 	libxul.so 	Java_org_mozilla_gecko_GeckoAppShell_nativeRun 	toolkit/xre/nsAndroidStartup.cpp:132
18 	libmozutils.so 	libmozutils.so@0x49de 	
19 	libdvm.so 	libdvm.so@0x11c77 	
20 	dalvik-LinearAlloc (deleted) 	dalvik-LinearAlloc @0x20a02b 	
21 	dalvik-heap (deleted) 	dalvik-heap @0x73e527 	
22 	libdvm.so 	libdvm.so@0x41186 	
23 	data@[e-mail][e-mail] 	data@[e-mail][e-mail]@0xfd3d 	
24 	libmozutils.so 	libmozutils.so@0x49c8 	
25 	dalvik-LinearAlloc (deleted) 	dalvik-LinearAlloc @0x20a02b 	
26 	libdvm.so 	libdvm.so@0x4113c 	
27 	dalvik-heap (deleted) 	dalvik-heap @0x73e527 	
28 	libdvm.so 	libdvm.so@0x46788 	
29 	dalvik-LinearAlloc (deleted) 	dalvik-LinearAlloc @0x20a02b 	
30 	data@[e-mail][e-mail] 	data@[e-mail][e-mail]@0x9af9 	
31 	dalvik-heap (deleted) 	dalvik-heap @0x73e527 	
32 	libdvm.so 	libdvm.so@0x11e3f 	
33 	libdvm.so 	libdvm.so@0x16e9f 	
34 	libdvm.so 	libdvm.so@0x1bd5f 	
35 	libdvm.so 	libdvm.so@0x1bccf 	
36 	dalvik-LinearAlloc (deleted) 	dalvik-LinearAlloc @0x20b4d7 	
37 	libdvm.so 	libdvm.so@0x1ae13 	
38 	libdvm.so 	libdvm.so@0x16b1b 	
39 	core.odex 	core.odex@0x10097f 	
40 	dalvik-heap (deleted) 	dalvik-heap @0x82b717 	
41 	dalvik-LinearAlloc (deleted) 	dalvik-LinearAlloc @0x20a39f 	
42 	dalvik-mark-stack (deleted) 	dalvik-mark-stack @0x4dbc86f 	
43 	libdvm.so 	libdvm.so@0x9ef77 	
44 	libdvm.so 	libdvm.so@0x16b7f 	
45 	libdvm.so 	libdvm.so@0x16bf7 	
46 	libdvm.so 	libdvm.so@0x16a9f 	
47 	libdvm.so 	libdvm.so@0x16ac7 	
48 	libdvm.so 	libdvm.so@0x16b1b 	
49 	data@[e-mail][e-mail] 	data@[e-mail][e-mail]@0x6253 	
50 	data@[e-mail][e-mail] 	data@[e-mail][e-mail]@0x6253 	
51 	core.odex 	core.odex@0x155e1b 	
52 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x4f940 	
53 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x541d0 	
54 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x264623 	
55 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x25f80f 	
56 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x25f0c6 	
57 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6deaad 	
58 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x25f81d 	
59 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x266753 	
60 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6df6f3 	
61 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6dec4b 	
62 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6d77ea 	
63 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6dec46 	
64 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6d77e4 	
65 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x25f0c1 	
66 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6d77e8 	
67 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x6dec47 	
68 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x7870d1 	
69 	org.mozilla.firefox_beta-1.apk 	org.mozilla.firefox_beta-1.apk@0x541d2 	
70 	dalvik-heap (deleted) 	dalvik-heap @0x552f 	
71 	dalvik-LinearAlloc (deleted) 	dalvik-LinearAlloc @0x18433 	

Show/hide other threads

More Reports: https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2011-07-15%2012%3A00%3A00&signature=_moz_cairo_surface_get_device_offset&version=Fennec%3A6.0


On : Motorola Xoom
OS :  verizon/trygon/stingray:3.2/HLJ86/137724:user/release-keys
Uptime : < 352

Comment 1

[Scoobidiver (away)]

It is #1 top browser crasher in Fennec 6.0.

Comment 2

[Josh Matthews [:jdm]]

This looks like we have a null mSurface in gfxASurface.

Comment 3

[Josh Matthews [:jdm]]

>72 void
>73 gfxImageSurface::InitWithData(unsigned char *aData, const gfxIntSize& aSize,
>74                               long aStride, gfxImageFormat aFormat)
>75 {
>76     mSize = aSize;
>77     mOwnsData = PR_FALSE;
>78     mData = aData;
>79     mFormat = aFormat;
>80     mStride = aStride;
>81 
>82     if (!CheckSurfaceSize(aSize))
>83         return;

If CheckSurfaceSize fails, we are stuck with a NULL mSurface pointer (and mSurfaceValid is false, but we don't appear to check that religiously).

Comment 4

[Josh Matthews [:jdm]]

Attachment: Ensure that gfx surfaces contain a valid mSurface pointer even if the requested dimensions are disallowed.

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 547199 [details] [diff] [review]
Ensure that gfx surfaces contain a valid mSurface pointer even if the requested dimensions are disallowed.

Nevermind, I noticed that this check is repeated in a bunch of places. I'll address all of those as well.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Joe, we're going to want this in Beta(6) and Aurora(7). I'm assuming we're not going to want the refactor patch for the release branches. If my assumption is correct, can we get the jdm's smaller fix reviewed and ready for release branch landing.

Comment 7

[Joe Drew (not getting mail)]

Is it the case that attachment 547199 [details] [diff] [review] will fix this bug? It's not clear from reading comment 5.

Comment 8

[Josh Matthews [:jdm]]

A variation of the existing obsolete patch should solve this bug, since it will ensure that all gfxImageSurfaces will contain valid cairo surfaces. I can do a belt-and-suspenders fix as well by adding a check for CairoStatus in the android nsWindow code.

Comment 9

[Josh Matthews [:jdm]]

A regression window here would be interesting, at the very least, to figure out why this topcrash started occurring.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #9)
> A regression window here would be interesting, at the very least, to figure
> out why this topcrash started occurring.

First appeared on crast-stats on July 14th:
https://crash-stats.mozilla.com/report/index/aa3d7691-757e-44ab-aa49-f036f2110714

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #10)
> (In reply to comment #9)
> > A regression window here would be interesting, at the very least, to figure
> > out why this topcrash started occurring.
> 
> First appeared on crast-stats on July 14th:
> https://crash-stats.mozilla.com/report/index/aa3d7691-757e-44ab-aa49-
> f036f2110714

That is a lie

Comment 12

[Josh Matthews [:jdm]]

Attachment: Prevent calling cairo functions on invalid surfaces through gfxASurface.

Comment 13

[Josh Matthews [:jdm]]

My idea of creating a null cairo surface is too invasive and potentially-behaviour-changing for me to feel comfortable about it. Here's a much more reliable belt-and-suspenders solution that prevents calling cairo functions on null surfaces (some functions had the check, others didn't, so I figured me might as well go all out), as well as checking the CairoStatus of the new surfaces in the Android window impl and avoiding using them if they're not valid.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Prevent calling cairo functions on invalid surfaces through gfxASurface.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I accidentally perma-reproduced this by putting Nightly into "zoom mode", where it's zoomed to fill the screen on a Xoom.  FWIW.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #14)
> Created attachment 547600 [details] [diff] [review] [review]
> Prevent calling cairo functions on invalid surfaces through gfxASurface.

This seems like the wrong approach. If we want to be able to call functions on invalid surfaces we should just initialize the surface and let cairo do the checking. cairo already has the concept of a null surface.

Comment 17

[Josh Matthews [:jdm]]

I was seeing test failures when I tried that. For the patch to address the topcrash on Beta, I really want to go with the simples and most reliable option here.

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 547600 [details] [diff] [review]
Prevent calling cairo functions on invalid surfaces through gfxASurface.

Joe, you're on the hook for gfx/ changes, Doug's in charge of the Android window ones. Try was all green.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 547600 [details] [diff] [review]
Prevent calling cairo functions on invalid surfaces through gfxASurface.

On the android bits:
  |if (targetSurface && !targetSurface->CairoStatus())| is what we do on windows.

No idea if the gfxASurface changes are good.  I think you can have a null surface in cairo so, those cairo functions should do the right thing.  Maybe joe or jeff know the details.

Comment 20

[Joe Drew (not getting mail)]

Comment on attachment 547600 [details] [diff] [review]
Prevent calling cairo functions on invalid surfaces through gfxASurface.

Review of attachment 547600 [details] [diff] [review]:
-----------------------------------------------------------------

nsWindow parts are obviously good and correct. gfxASurface I want Jeff's input.

Comment 21

[Joe Drew (not getting mail)]

Comment on attachment 547600 [details] [diff] [review]
Prevent calling cairo functions on invalid surfaces through gfxASurface.

Review of attachment 547600 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 22

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cd6bf87809e9

Comment 23

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/cd6bf87809e9

Comment 24

[LegNeato]

Comment on attachment 547600 [details] [diff] [review]
Prevent calling cairo functions on invalid surfaces through gfxASurface.

Approved for mozilla-aurora and mozilla-beta. Please land asap

Comment 25

[Josh Matthews [:jdm]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/6dae6e92d99e
http://hg.mozilla.org/releases/mozilla-beta/rev/049e86d2547d

Comment 26

[Tony Chung [:tchung]]

if anyone can provide reproducible steps, please comment.  QA would like to verify this.

Comment 27

[Tony Chung [:tchung]]

(In reply to comment #26)
> if anyone can provide reproducible steps, please comment.  QA would like to
> verify this.

oh, and we do have a ASUS TF101 EEpad Transformer (with keyboard) to hammer on.  Since most of the crashes are being reported on that platform

Comment 28

[Scoobidiver (away)]

According to crash stats, there have been no crashes in Fennec 6.0b4 while there were about 15 crashes per day in previous Beta builds.
See https://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=_moz_cairo_surface_get_device_offset&version=Fennec%3A6.0 and click the Graph tab.

Comment 29

[Marcia Knous [:marcia]]

I was asked to get some URLs for verification purposes, so here they are:

      1 http://www.megatv.com/article.asp?catid=14693#toppage
      1 http://www.games.co.id/permainan/berpakaian/berpakaian.html
      1 http://www.cesgranrio.org.br/pdf/petrobras0111/petrobras0111_edital2.pdf
      1 http://mail.yahoo.com/
      1 http://clipat.maktoob.com/video.php?video_id=294884