Last Comment Bug 752426 - crash in mozilla::AndroidBridge::TakeScreenshot
: crash in mozilla::AndroidBridge::TakeScreenshot
Status: VERIFIED FIXED
[native-crash]
: 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) (on PTO through June 3)
:
Mentors:
: 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: ---
verified
verified
+
14+


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

Description 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:
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 (Back on May31) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-07 05:46:03 PDT
docElement is probably null.
Comment 2 Adrian Tamas (:AdrianT) 2012-05-07 07:06:22 PDT
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) (on PTO through June 3) 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 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2012-05-07 14:33:53 PDT
I guess this would also fix bug 749915, right?
Comment 6 Brian Nicholson (:bnicholson) (on PTO through June 3) 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 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 Brian Nicholson (:bnicholson) (on PTO through June 3) 2012-05-07 16:53:45 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/8aadf11d119e
Comment 9 Ed Morley [:emorley] 2012-05-08 03:22:39 PDT
https://hg.mozilla.org/mozilla-central/rev/8aadf11d119e
Comment 10 Brian Nicholson (:bnicholson) (on PTO through June 3) 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 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2012-05-08 13:46:32 PDT
*** Bug 749915 has been marked as a duplicate of this bug. ***
Comment 12 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 Brian Nicholson (:bnicholson) (on PTO through June 3) 2012-05-11 17:54:00 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/7505c2f398d5
Comment 14 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.