752426 - crash in mozilla::AndroidBridge::TakeScreenshot

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Scoobidiver (away)]

It's #1 top crasher in the trunk and first appeared in 15.0a1/20120506. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0a48e6561534&tochange=94ce5f33a9ea
It's likely a regression from bug 748531.

Signature 	mozilla::AndroidBridge::TakeScreenshot More Reports Search
UUID	30795d8f-2832-40a8-92a4-a2d632120507
Date Processed	2012-05-07 06:33:14
Uptime	99
Last Crash	13.4 minutes before submission
Install Age	6.0 hours since version was first installed.
Install Time	2012-05-07 00:34:11
Product	FennecAndroid
Version	15.0a1
Build ID	20120506030520
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 2.6.36-P6800XXLA4-CL616407 #3 SMP PREEMPT Thu Jan 5 21:02:55 KST 2012 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
AdapterVendorID: smdkc210, AdapterDeviceID: GT-P6800.
AdapterDescription: 'Model: 'GT-P6800', Product: 'GT-P6800', Manufacturer: 'samsung', Hardware: 'smdkc210''.
samsung GT-P6800
samsung/GT-P6800/GT-P6800:3.2/HTJ85B/XXLA4:user/release-keys
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libxul.so 	mozilla::AndroidBridge::TakeScreenshot 	widget/android/AndroidBridge.cpp:2414
1 	libxul.so 	nsAppShell::ProcessNextNativeEvent 	widget/android/nsAppShell.cpp:475
2 	libxul.so 	nsBaseAppShell::DoProcessNextNativeEvent 	widget/xpwidgets/nsBaseAppShell.cpp:171
3 	libxul.so 	nsBaseAppShell::OnProcessNextEvent 	widget/xpwidgets/nsBaseAppShell.cpp:306
4 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
5 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
6 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:114
7 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
8 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:201
9 	libxul.so 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:189
10 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:295
11 	libxul.so 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3780
12 	libxul.so 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3857
13 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3933
14 	libxul.so 	GeckoStart 	toolkit/xre/nsAndroidStartup.cpp:109

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3AAndroidBridge%3A%3ATakeScreenshot

Comment 1

[Kartikaya Gupta (email:[e-mail])]

docElement is probably null.

Comment 2

[Adrian Tamas (:AdrianT)]

The crash has a high reproducibility rate following the steps:
1. Go to http://imgur.com/
2. Choose "View the Imagur Gallery".
3. Use "Next" to go to the next image.

Usually Fennec crashes 50% of times "Next" is pressed and the next page loads.

Tested using:
Nightly 15.0a1 2012-04-07
Device: HTC Desire
OS: Android 2.2

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: Only update thumbnail if URL hasn't changed

When clicking Next in imgur, it looks like we load a page that quickly redirects to another page each time (you can also tell this happens as the throbber stops, then quickly starts again). So we're getting 2 document stop events: one before the redirection, then one after. When we get the first document stop, we start a Runnable that takes the screenshot - but by the time the Runnable runs, we've already started loading the next page. The documentElement won't necessarily exist at this point, resulting in the crash.

This can be fixed by ensuring the document stop URL is the URL we're expecting (the current tab's URL). I also added null checks in takeScreenshot() just to be safe.

Comment 4

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

Comment on attachment 621671 [details] [diff] [review]
Only update thumbnail if URL hasn't changed

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

::: widget/android/AndroidBridge.cpp
@@ +2398,5 @@
>      nsresult rv;
>  
>      // take a screenshot, as wide as possible, proportional to the destination size
>      if (!srcW && !srcH) {
> +        nsCOMPtr<nsIDOMDocument> doc = 0;

nsCOMPtr's don't need to be initialized to null.

@@ +2403,5 @@
> +        window->GetDocument(getter_AddRefs(doc));
> +        if (!doc)
> +            return NS_ERROR_FAILURE;
> +
> +        nsCOMPtr<nsIDOMElement> docElement = 0;

nsCOMPtr's don't need to be initialized to null.

@@ +2406,5 @@
> +
> +        nsCOMPtr<nsIDOMElement> docElement = 0;
> +        doc->GetDocumentElement(getter_AddRefs(docElement));
> +        if (!docElement)
> +            return NS_ERROR_FAILURE;

does null checking the docElement alone fix this crash? If so, let's land that as the fix by itself and take a look at the changes on the java side separately.

Comment 5

[Martijn Wargers (dead)]

I guess this would also fix bug 749915, right?

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: patch v2

Addresses above comments.

> does null checking the docElement alone fix this crash? If so, let's land
> that as the fix by itself and take a look at the changes on the java side
> separately.

Yes - filed bug 752713.

Comment 7

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

Comment on attachment 621752 [details] [diff] [review]
patch v2

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

::: widget/android/AndroidBridge.cpp
@@ -2405,4 @@
>  
>          nsCOMPtr<nsIDOMElement> docElement;
> -        rv = doc->GetDocumentElement(getter_AddRefs(docElement));
> -        NS_ENSURE_SUCCESS(rv, rv);

still check the return values

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/8aadf11d119e

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8aadf11d119e

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 621752 [details] [diff] [review]
patch v2

[Approval Request Comment]
Regression caused by (bug #): bug 750846
User impact if declined: redirecting pages may crash
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low risk (just adds null checks)
String changes made by this patch: none

Comment 12

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

Last crash for this is on 20120508030517 build so far.

Comment 13

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/7505c2f398d5

Comment 14

[Kevin Brosnan [Ex-Mozilla]]

Verified by crash stats inspection.