Closed Bug 752426 Opened 10 years ago Closed 10 years ago

crash in mozilla::AndroidBridge::TakeScreenshot


(Core Graveyard :: Widget: Android, defect)

15 Branch
Not set


(firefox14 verified, firefox15 verified, blocking-fennec1.0 +, fennec14+)

Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +
fennec 14+ ---


(Reporter: scoobidiver, Assigned: bnicholson)



(4 keywords, Whiteboard: [native-crash])

Crash Data


(1 file, 1 obsolete file)

It's #1 top crasher in the trunk and first appeared in 15.0a1/20120506. The regression range is:
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
EMCheckCompatibility	True

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

More reports at:
Version: Trunk → 15 Branch
docElement is probably null.
Blocks: 750846
No longer blocks: 748531
The crash has a high reproducibility rate following the steps:
1. Go to
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
Keywords: reproducible
Assignee: nobody → bnicholson
blocking-fennec1.0: --- → ?
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.
Attachment #621671 - Flags: review?(blassey.bugs)
tracking-fennec: ? → 14+
blocking-fennec1.0: ? → +
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.
Attachment #621671 - Flags: review?(blassey.bugs) → review-
I guess this would also fix bug 749915, right?
Attached patch patch v2Splinter Review
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.
Attachment #621671 - Attachment is obsolete: true
Attachment #621752 - Flags: review?(blassey.bugs)
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
Attachment #621752 - Flags: review?(blassey.bugs) → review+
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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
Attachment #621752 - Flags: approval-mozilla-aurora?
Attachment #621752 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 749915
Last crash for this is on 20120508030517 build so far.
Verified by crash stats inspection.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.