739488 - Fennec Native nightly crashes in mozilla::AndroidBridge::EnableLocationHighAccuracy when page requests GPS location

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[KLB]

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)]

(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]]

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]

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)]

Interesting.  Maps.google.com shares the location just fine.  Doesn't crash on desktop.

Comment 5

[KLB]

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)]

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]]

Possible regression from bug 735011? Does a build from the 21st not crash?

Comment 8

[KLB]

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 (dead)]

Attachment: testcase

Thanks for finding out what causes the crash.

Comment 10

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

Yes, excellent work, KLB!  Thank you for your report and analysis!

Comment 11

[KLB]

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)]

(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]

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)]

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)]

Attachment: 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)]

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)]

Attachment: 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)]

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)]

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)]

Attachment: patch v3

updated patch to define enableLocationHighAccuracy in embedding/android/GeckoAppShell.java, so that XUL is not affected.

Comment 21

[Doug Turner (:dougt)]

Comment on attachment 610711 [details] [diff] [review]
patch v3

I removed: 
        mLocationHighAccuracy = enable;

Because, i don't think it will compile with this.

Comment 22

[Doug Turner (:dougt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33c419fc0fe4

Comment 23

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/33c419fc0fe4

Comment 24

[Cristian Nicolae (:xti)]

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