816850 - geolocation timeout after airplane mode toggled

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect, P1)

Description

[Michael Vines [:m1] [:evilmachines]]

Whenever we do Airplane mode ON-> OFF or GPS OFF-> ON, consecutive E2E GPS sessions are showing error Timeout, but fixes are getting triggered and final position is being received by HAL.

Steps:
1)	Make sure airplane mode is OFF or GPS is ON
2)	trigger E2E GPS session using test app, it will report the lat/long properly
3)	Now set airplane mode to ON or GPS to OFF, wait 30 sec-ish and undo this change
4)	trigger E2E GPS session using test app, it will report the lat/long properly --> Here, App unable to show lat/long, gives Timeout-error

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Some analysis:

When GPS is turned off, it triggers HandleMozsettingValue with value false, which results in calling mGeoLocators[i]->shutdown(). In nsGeolocation::Shutdown, we remove it from the nsGeolocationService.

But when we turn back GPS, there is no call to add the nsGeolocation instance back to the mGeoLocators list. Hence even though we get the position fix from the HAL in nsGeolocationService::Update, since the mGeoLocators list is empty, the call does not get processes further.

The issue is the only place where the nsGeolocation instance gets added to the service is in nsGeolocation::Init, which is getting called only once when I open the qcgpstest app for the first time and start the session. 

If i comment out the following code in nsGeolocation::Shutdown, i am able to solve the issue.

/*if (mService) { printf_stderr("nsGeolocation:: Remove self from service"); mService->RemoveLocator(this); }*/

Comment 2

[Michael Vines [:m1] [:evilmachines]]

Can't share a patch to nsGeolocation.cpp yet, but I think we may just need to move this [1] to here [2]

[1] http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#1142
[2] http://mxr.mozilla.org/mozilla-central/source/dom/src/geolocation/nsGeolocation.cpp#1091

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Broken feature.  Thanks for the feedback :).  Things are coming along ;).

Kan-Ru, can you take a look or reassign?  Thanks!

Comment 4

[Doug Turner (:dougt)]

i can look Kan-Ru if you are overloaded.

Comment 5

[Kan-Ru Chen [:kanru] (UTC+9)]

I have free cycles.

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Don't call nsGeolocation::Shutdown() when disabled via settings. v1

Don't call nsGeolocation::Shutdown(), this method is destructive. It does not only remove itself from mService but also remove all registered callbacks.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 687646 [details] [diff] [review]
Don't call nsGeolocation::Shutdown() when disabled via settings. v1

This makes sense.  Just change the comment before you check in:

Please change the comment to from:

       // turn things off

to:

       // Do not shutdown the mGeolocators, but instead just stop the device and do a final update of null.  See bug #816850.

Comment 8

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3b4311176fa

Comment 9

[Ed Morley [:emorley]]

Backed out for making test_mozsettingsWatch.html fail frequently on OS X 10.6:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17551432&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=17551711&tree=Mozilla-Inbound

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00be45c9abe2

Comment 10

[Kan-Ru Chen [:kanru] (UTC+9)]

ah, I forget the remove the first watcher

Comment 11

[Ed Morley [:emorley]]

Also on Windows:
https://tbpl.mozilla.org/php/getParsedLog.php?id=17551956&tree=Mozilla-Inbound

Comment 12

[Faramarz Rashed]

:kanru can you wrap this up quickly?

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Faramarz (:faramarz) from comment #12)
> :kanru can you wrap this up quickly?

Yeah, the try run looks good. I'll have a updated patch shortly.

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Don't call nsGeolocation::Shutdown() when disabled via settings. v1.1

Clear watch after test.

https://tbpl.mozilla.org/?tree=Try&rev=0415371db6a9

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 688120 [details] [diff] [review]
Don't call nsGeolocation::Shutdown() when disabled via settings. v1.1

Failed again. The log looks very strange, some tests are just skipped or not shown.

Comment 16

[Dietrich Ayala (:dietrich)]

Hi Kanru, what's the next step here?

Comment 17

[Kan-Ru Chen [:kanru] (UTC+9)]

Last try: https://tbpl.mozilla.org/?tree=Try&rev=e9277e141086

It still failed, that doesn't make sense :/

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Attachment: Don't call nsGeolocation::Shutdown() when disabled via settings. v1.2

https://tbpl.mozilla.org/?tree=Try&rev=cd83b544930f

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88986a16df5b

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88986a16df5b

Comment 21

[Kan-Ru Chen [:kanru] (UTC+9)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/aaa8c4b2af14
https://hg.mozilla.org/releases/mozilla-beta/rev/a29e2be8f636