Last Comment Bug 752426 - crash in mozilla::AndroidBridge::TakeScreenshot
: crash in mozilla::AndroidBridge::TakeScreenshot
: crash, regression, reproducible, topcrash
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: 15 Branch
: ARM Android
-- critical (vote)
: mozilla15
Assigned To: Brian Nicholson (:bnicholson)
: Jim Chen [:jchen] [:darchons]
: 749915 (view as bug list)
Depends on:
Blocks: 750846
  Show dependency treegraph
Reported: 2012-05-07 00:36 PDT by Scoobidiver (away)
Modified: 2012-05-17 19:52 PDT (History)
9 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Only update thumbnail if URL hasn't changed (4.28 KB, patch)
2012-05-07 11:06 PDT, Brian Nicholson (:bnicholson)
blassey.bugs: review-
Details | Diff | Splinter Review
patch v2 (1.45 KB, patch)
2012-05-07 15:10 PDT, Brian Nicholson (:bnicholson)
blassey.bugs: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Scoobidiver (away) 2012-05-07 00:36:43 PDT
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:
Comment 1 User image Kartikaya Gupta ( 2012-05-07 05:46:03 PDT
docElement is probably null.
Comment 2 User image Adrian Tamas (:AdrianT) 2012-05-07 07:06:22 PDT
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
Comment 3 User image Brian Nicholson (:bnicholson) 2012-05-07 11:06:25 PDT
Created attachment 621671 [details] [diff] [review]
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 User image Brad Lassey [:blassey] (use needinfo?) 2012-05-07 14:21:17 PDT
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 User image Martijn Wargers [:mwargers] 2012-05-07 14:33:53 PDT
I guess this would also fix bug 749915, right?
Comment 6 User image Brian Nicholson (:bnicholson) 2012-05-07 15:10:50 PDT
Created attachment 621752 [details] [diff] [review]
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 User image Brad Lassey [:blassey] (use needinfo?) 2012-05-07 16:27:00 PDT
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 User image Brian Nicholson (:bnicholson) 2012-05-07 16:53:45 PDT
Comment 9 User image Ed Morley [:emorley] 2012-05-08 03:22:39 PDT
Comment 10 User image Brian Nicholson (:bnicholson) 2012-05-08 12:28:28 PDT
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 11 User image Martijn Wargers [:mwargers] 2012-05-08 13:46:32 PDT
*** Bug 749915 has been marked as a duplicate of this bug. ***
Comment 12 User image Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-10 09:28:42 PDT
Last crash for this is on 20120508030517 build so far.
Comment 13 User image Brian Nicholson (:bnicholson) 2012-05-11 17:54:00 PDT
Comment 14 User image Kevin Brosnan [:kbrosnan] 2012-05-17 19:52:20 PDT
Verified by crash stats inspection.

Note You need to log in before you can comment on or make changes to this bug.