Last Comment Bug 739488 - Fennec Native nightly crashes in mozilla::AndroidBridge::EnableLocationHighAccuracy when page requests GPS location
: Fennec Native nightly crashes in mozilla::AndroidBridge::EnableLocationHighAc...
Status: VERIFIED FIXED
[MTD][native-crash]
: crash, regression, testcase, topcrash
Product: Core
Classification: Components
Component: Widget: Android (show other bugs)
: 14 Branch
: ARM Android
: -- critical with 1 vote (vote)
: mozilla14
Assigned To: Josh Dhaliwal (:Arreth)
:
Mentors:
http://envirochem.us/gpstest.htm
Depends on:
Blocks: 735011
  Show dependency treegraph
 
Reported: 2012-03-26 18:02 PDT by KLB
Modified: 2012-05-23 07:27 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
+


Attachments
testcase (492 bytes, text/html)
2012-03-27 15:29 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch v1 (2.20 KB, patch)
2012-03-28 22:38 PDT, Josh Dhaliwal (:Arreth)
dougt: review-
Details | Diff | Review
patch v2 (2.21 KB, patch)
2012-03-29 03:15 PDT, Josh Dhaliwal (:Arreth)
dougt: review+
Details | Diff | Review
patch v3 (3.14 KB, patch)
2012-03-29 15:04 PDT, Josh Dhaliwal (:Arreth)
dougt: review-
Details | Diff | Review

Description KLB 2012-03-26 18:02:05 PDT
Web page or screen you were on when you saw the issue: 

When page requests user's location Fennec Native Nightly (2012-03-26) crashes

Steps to reproduce:
1. Go to http://envirochem.us/gpstest.htm
2. Click on start button.
3. Fennec crashes.

Reproduced on: Samsung Galaxy SII and Samsung Galaxy Tab 10.1

This does not affect Fennec Native Aurora.

Crash report ID (if applicable): I don't know how to provide this information.
Comment 1 Scoobidiver (away) 2012-03-27 01:19:44 PDT
(In reply to KLB from comment #0)
> Crash report ID (if applicable): I don't know how to provide this
> information.
Go to about:crashes and provide the latest crash ID (bp-...).
Comment 2 Aaron Train [:aaronmt] 2012-03-27 06:42:11 PDT
Reproduced with a Galaxy Nexus (Android 4.0.2), Nightly (03/27)

bp-b1a81ca0-6521-48b9-b7cf-942c92120327

Signature: [@ dalvik-LinearAlloc (deleted)@0x290466 ] (is this valid?)
Comment 3 KLB 2012-03-27 06:55:38 PDT
My crash profiles for this issue:

Samsung Galaxy Tab 10.1
http://crash-stats.mozilla.com/report/index/bp-635e89a3-f575-415c-af4b-925b72120327
http://crash-stats.mozilla.com/report/index/bp-1bee072d-18b1-45e5-9b4f-c59b32120327
http://crash-stats.mozilla.com/report/index/bp-36dcb31c-7474-4c89-952f-dadad2120327
http://crash-stats.mozilla.com/report/index/bp-9d2c56fb-f181-449d-8f60-5d44a2120327

Samsung Galaxy SII
http://crash-stats.mozilla.com/report/index/bp-b0c572e8-2662-4641-a543-1e11a2120327

Note: I only tested on the SII once to see if it was specific to my tablet.  Also, I first discovered this on a private site not publicly accessible site that I'm developing so I isolated the offending code to the demo page I linked to above.
Comment 4 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-03-27 10:57:50 PDT
Interesting.  Maps.google.com shares the location just fine.  Doesn't crash on desktop.
Comment 5 KLB 2012-03-27 11:38:14 PDT
Naoki,

I wonder if this is because my implementation keeps asking for location updates until the user "clicks" on the stop and record button.  Alternatively, it could be because I'm trying to force the browser to expire out existing location data and grab an updated location. 

I don't think most implementations of the location request will care that much about getting the most current/accurate data possible. In our implementation, we need as accurate of data as possible so we are having users grab a series of readings until they get multiple readings at the most accurate level their device is capable of and then averaging those readings. Ideally we want 5+ readings at 5 meter accuracy, but this isn't always possible.

Regardless of why I'm implementing the location functionality the way I am, I think we'll all agree that being able to crash the browser from a webpage is a bad thing that we wouldn't want to get past the nightly phase. If this were happening in a release version of Firefox, it would be considered a security vulnerability.

Hopefully my providing a demo page that isolates out my geolocation JS  helps eliminate code noise and allows for easier diagnosis of the problem.
Comment 6 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-03-27 14:16:31 PDT
Yes, I agree that crashing is bad; my comment was more of intrigue that it didn't crash on the geolocation from maps.google.com

The test page does help; we may just need to experiment further with the two scenarios that you had mentioned to narrow down the issue further.
Comment 7 Aaron Train [:aaronmt] 2012-03-27 14:22:58 PDT
Possible regression from bug 735011? Does a build from the 21st not crash?
Comment 8 KLB 2012-03-27 15:02:07 PDT
I ran some tests on the code and I found what causes the crashes. At line 95 in my script I have the following instruction:

navigator.geolocation.getCurrentPosition(geo_success, geo_error, {enableHighAccuracy:true, maximumAge:1000, timeout:240000});

If I change"enableHighAccuracy" to false, nightly stops crashing, which probably explains why Google Maps doesn't cause Nightly to crash.
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-03-27 15:29:46 PDT
Created attachment 609918 [details]
testcase

Thanks for finding out what causes the crash.
Comment 10 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-03-27 15:40:26 PDT
Yes, excellent work, KLB!  Thank you for your report and analysis!
Comment 11 KLB 2012-03-27 15:45:54 PDT
My pleasure.  I can't fix the bugs, but I can certainly do my part to try and narrow down the the cause of bugs I report.
Comment 12 Scoobidiver (away) 2012-03-28 00:16:57 PDT
(In reply to Aaron Train [:aaronmt] from comment #7)
> Possible regression from bug 735011? Does a build from the 21st not crash?
Based on crash stats, it first appeared in 14.0a1/20120323, so it's indeed a regression from bug 735011.
Comment 13 chris hofmann 2012-03-28 12:05:40 PDT
are there twitter or other social apps that tap into high accuracy gps?  those would make good test cases to confirm the fix.   In a few searches I spotted several survey sites where this might turn up as a blocker, but no popular social apps yet.
Comment 14 Doug Turner (:dougt) 2012-03-28 12:29:39 PDT
we think we know what the problem is. we can build a test for it.  josh is going to be working on a fix shortly.
Comment 15 Josh Dhaliwal (:Arreth) 2012-03-28 22:38:30 PDT
Created attachment 610436 [details] [diff] [review]
patch v1

From the different crash signatures that have been cited in this bug, this patch seems to contain the proper solution. Unfortunately (due to some reason I can't seem to figure out), the builds that I've made on this and other machines have been crashing with a completely different crash signature that I can't find a solution for. I didn't want to have everyone wait for me to figure this out, so if somebody else has time, if they could try using this patch and testing it out. Thanks in advance, and apologies.
Comment 16 Doug Turner (:dougt) 2012-03-29 00:08:25 PDT
Comment on attachment 610436 [details] [diff] [review]
patch v1

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

::: widget/android/AndroidBridge.cpp
@@ +115,5 @@
>      jNotifyScreenShot = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "notifyScreenShot", "(Ljava/nio/ByteBuffer;III)V");
>      jAcknowledgeEventSync = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "acknowledgeEventSync", "()V");
>  
>      jEnableLocation = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "enableLocation", "(Z)V");
> +    jEnableLocation = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "enableLocationHighAccuracy", "(Z)V");

Shouldn't this be?

jEnableLocationHighAccuracy = (jmethodID) jEnv->GetStaticMethodID(jGeckoAppShellClass, "enableLocationHighAccuracy", "(Z)V");
Comment 17 Josh Dhaliwal (:Arreth) 2012-03-29 03:15:00 PDT
Created attachment 610501 [details] [diff] [review]
patch v2

:dougt - You are very right, my bad. Corrected this mistake.

Finally got my issue resolved. Tested, and crash no longer happens on my devices that I've tested on (Galaxy Nexus, and HTC Desire HD).
Comment 18 Cristian Nicolae (:xti) 2012-03-29 06:37:48 PDT
This crash has occurred on the latest Nightly build after I tapped on Share button on fandango.com:
https://crash-stats.mozilla.com/report/index/2b607e5a-2e98-4540-bf04-566bc2120329

--
Firefox 14.0a1 (2012-03-28)
Device: Samsung Galaxy S
OS: Android 2.2
Comment 19 Phil Ringnalda (:philor) 2012-03-29 08:31:01 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/bf171cf48355 for the XUL bustage in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3e6a7f9fb34e (the native talos bustage was probably inherited from below, but you won't know until you push the next patch to Try).
Comment 20 Josh Dhaliwal (:Arreth) 2012-03-29 15:04:12 PDT
Created attachment 610711 [details] [diff] [review]
patch v3

updated patch to define enableLocationHighAccuracy in embedding/android/GeckoAppShell.java, so that XUL is not affected.
Comment 21 Doug Turner (:dougt) 2012-04-02 09:13:05 PDT
Comment on attachment 610711 [details] [diff] [review]
patch v3

I removed: 
        mLocationHighAccuracy = enable;

Because, i don't think it will compile with this.
Comment 23 Marco Bonardo [::mak] 2012-04-03 02:01:20 PDT
https://hg.mozilla.org/mozilla-central/rev/33c419fc0fe4
Comment 24 Cristian Nicolae (:xti) 2012-05-23 07:27:04 PDT
No crash occurs if I perform the str from comment #0 on the latest Nightly. Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-23)
Device: Galaxy Nexus
OS: Android 4.0.2

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