Closed
Bug 1216148
Opened 9 years ago
Closed 8 years ago
Geolocation does not turn off with Firefox OS browser backgrounded
Categories
(Core :: DOM: Geolocation, defect)
Core
DOM: Geolocation
Tracking
()
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: jhylands, Assigned: ywu)
References
Details
(Whiteboard: [Power:P1])
Attachments
(2 files, 5 obsolete files)
1.57 MB,
image/jpeg
|
Details | |
9.42 KB,
patch
|
Details | Diff | Splinter Review |
If the browser is started, and you go to "maps.google.com", and turn on the geolocation permission, wait for the map to load, and then press the homescreen button, geolocation should shut itself down after a certain (short) length of time. Currently, it does not shut down, and stays running, which adversely affects power consumption until the browser app is terminated.
Reporter | ||
Comment 1•9 years ago
|
||
This snapshot shows the homescreen with the browser backgrounded, after ten minutes, and the geolocation icon is still there.
Updated•9 years ago
|
Component: Gaia::Browser → Geolocation
Product: Firefox OS → Core
Reporter | ||
Comment 3•9 years ago
|
||
Power Usage: Idle Homescreen, no geolocation - 1.2 mA Idle Homescreen, geolocation on - 61.5 mA
Reporter | ||
Comment 4•9 years ago
|
||
The above power usage numbers are both with the display turned off...
Reporter | ||
Updated•9 years ago
|
Whiteboard: [Power]
Until there is a proper API for this, we should require apps be installed to run background watchPosition (currently webpages and installed apps are treated identically by the geo engine). This is partially a WebAPI bug IMHO, there was recent discussion about this problem on the mozilla dev-geolocation mailing list. There is no way to specify that geolocation should run in the background. A developer-centric solution (again IMHO) might look like: options = { runInBackground = true; // my imaginary extra option, currently, PositionOptions object does not have this }; id = navigator.geolocation.watchPosition(success, error, options)
The suggestion from Jonas on this thread was to look at using the wake lock API for this: https://groups.google.com/forum/#!topic/mozilla.dev.geolocation/UXkd3Tz1GPc
Comment 9•9 years ago
|
||
From previous discussion, we should make the background page stop receiving GPS as the default behavior. I need some time to investigate this topic before doing next step.
Flags: needinfo?(alchen)
Assignee | ||
Comment 10•9 years ago
|
||
It seems that we use a smart pointer to control how many applications are using GPS service now. And we close the service whenever the pointer counts to 0. But we only decrease this counter when (1) application stops using the service and disconnects by itself (2) application gets killed in the background. If I am right, maybe the solution is like Garvan said. We can have an optional variable to specify if this application wants GPS service running in background and decreases the counter when application gets switched to background.
Comment 11•9 years ago
|
||
[Tracking Requested - why for this release]: Removing nomination for 2.5 as its Not a regression. Tracking as part of backlog. Thanks
Updated•9 years ago
|
Keywords: regressionwindow-wanted
Reporter | ||
Comment 12•9 years ago
|
||
I've just tested this with the camera app, and it does the right thing - with geoloc turned on, I take a picture, then switch to the home screen, and within a few seconds the geoloc icon grays out, and then goes away. I'm doing this in my basement, so there's no chance to get a real location, and I've validated that the picture I took does not in fact contain a location tag.
Comment 13•9 years ago
|
||
The camera is using getCurrentPosition() (https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/getCurrentPosition), which is single shot. The geolocation service will stay alive for a short period after last use (~20s IIRC). The original report was google maps, which is using watchPosition() (https://developer.mozilla.org/en-US/docs/Web/API/Geolocation/watchPosition). The geolocation service (and the GPS itself) will not shut down if there is a process using watchPosition(). The geo service logic (and the UI for the user to choose if they want a web app/page to use background geo) is too primitive ATM: web devs using watchPosition() may not be intending to use background geolocation. Some do intend this, or in some cases, it depends on the use case of the app. To use google maps for navigation during travel, the GPS can't be allowed to go cold (the time-to-first-fix is too long); GPS updates need to be immediate.
Comment 14•9 years ago
|
||
(In reply to Garvan from comment #6) > Until there is a proper API for this, we should require apps be installed to > run background watchPosition (currently webpages and installed apps are > treated identically by the geo engine). Currently I have several packaged apps on my phone that erroneously hold geo open in the background, so I don't think limiting it to packaged apps fixes the bug. Also, there are valid reasons for web pages to want to hold geo open in the background. The biggest problem for me is that it's incredibly unclear from the UI that lots of power is being used by a webpage - only a small thumbtack icon, and there is no indication of which tab is using it - you basically have to close all your tabs or reboot your phone. There's a much better UI for background sound playback - if that could be reused for this, it would solve the immediate problem (allow jumping to / closing the background tab)
Updated•9 years ago
|
Whiteboard: [Power] → [Power:P1]
Comment 16•8 years ago
|
||
Ya-Chieh is working on this bug. She will provide the patch based on wakelock(Comment 7) next week. We can have a discussion about it once the patch is ready. Thanks.
Assignee | ||
Comment 17•8 years ago
|
||
We tried to do two things in this patch. (1) Make the default behavior that background pages automatically stop receiving GPS and orientation data. (2) Using WakeLock to keep GPS on if applications want to keep receiving the GPS data in the background. Add the below wakelock to keep GPS on. window.navigator.requestWakeLock('Gps');
Attachment #8690670 -
Flags: feedback?(kchen)
Attachment #8690670 -
Flags: feedback?(garvankeeley+bmo)
Comment 18•8 years ago
|
||
Comment on attachment 8690670 [details] [diff] [review] [WIP Nov. 23] proposal of how geolocation service acts in terms of application visibility Review of attachment 8690670 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Ya-Chieh. This is a very interesting feature! btw, Garvan no longer works at Mozilla, so he may be slow to respond. ::: dom/geolocation/nsGeolocation.cpp @@ +37,5 @@ > +#include "nsIDOMEvent.h" > +#include "mozilla/dom/Event.h" > +#include "mozilla/HalWakeLock.h" > +#include "mozilla/dom/WakeLock.h" > +#include "mozilla/Hal.h" These includes should be alphabetized and combined with the #include "mozilla/..." section above. @@ +38,5 @@ > +#include "mozilla/dom/Event.h" > +#include "mozilla/HalWakeLock.h" > +#include "mozilla/dom/WakeLock.h" > +#include "mozilla/Hal.h" > +using namespace mozilla::hal; You should add an empty line between the #includes and the `using namespace`. @@ +1221,5 @@ > } > > mPrincipal = doc->NodePrincipal(); > + > + doc->AddSystemEventListener(NS_LITERAL_STRING("visibilitychange"), Does RemoveSystemEventListener() need to be called elsewhere? If not, please add a short comment explaining the lifetime of this system event listener. @@ +1240,5 @@ > > +NS_IMETHODIMP > +Geolocation::HandleEvent(nsIDOMEvent* aEvent) > +{ > + Please remove this extra empty line. @@ +1251,5 @@ > + nsCOMPtr<nsIDocument> doc = do_QueryInterface(aEvent->InternalDOMEvent()->GetTarget()); > + MOZ_ASSERT(doc); > + > + if (doc->Hidden()) { > + Please remove this extra empty line. @@ +1252,5 @@ > + MOZ_ASSERT(doc); > + > + if (doc->Hidden()) { > + > + nsString GpsLockState; This GpsLockState variable is not used. @@ +1254,5 @@ > + if (doc->Hidden()) { > + > + nsString GpsLockState; > + nsString GpsLockName; > + GpsLockName = NS_LITERAL_STRING("Gps") ; I think the wakelock string should be lowercase "gps" instead of "Gps". The other wakelock strings in Gaia code are all lowercase, even for terms that are typically capitalized like "cpu" and "wifi". https://github.com/mozilla-b2g/gaia/search?utf8=%E2%9C%93&q=navigator.requestWakeLock&type=Code Also, please remove the extra space between the ) and ; @@ +1259,5 @@ > + WakeLockInformation info; > + GetWakeLockInfo(GpsLockName,&info); > + uint64_t myID = 0; > + ContentChild* cpc = ContentChild::GetSingleton(); > + myID = cpc->GetID(); The name "myID" is a little ambiguous. Who is the "my" in "myID"? You could merge these three lines of code into just one: uint64_t childID = ContentChild::GetSingleton()->GetID(); @@ +1261,5 @@ > + uint64_t myID = 0; > + ContentChild* cpc = ContentChild::GetSingleton(); > + myID = cpc->GetID(); > + bool thisProcessLocks = info.lockingProcesses().Contains(myID); > + if(!thisProcessLocks){ Please add a space between if and ( and ) and { @@ +1262,5 @@ > + ContentChild* cpc = ContentChild::GetSingleton(); > + myID = cpc->GetID(); > + bool thisProcessLocks = info.lockingProcesses().Contains(myID); > + if(!thisProcessLocks){ > + for (uint32_t i = 0 ; i < mLastWatchId; ++i) { Gecko's coding style uses 2 spaces per indent level. This for loop should be unindented 2 spaces to the left. @@ +1263,5 @@ > + myID = cpc->GetID(); > + bool thisProcessLocks = info.lockingProcesses().Contains(myID); > + if(!thisProcessLocks){ > + for (uint32_t i = 0 ; i < mLastWatchId; ++i) { > + ClearWatch(i); ClearWatch() should be unindented, too. ::: dom/geolocation/nsGeolocation.h @@ +36,5 @@ > #include "nsIDOMWindow.h" > #include "mozilla/Attributes.h" > > + > +#include "nsIDOMEventListener.h" Please move this new #include into the #include "nsI*.h" headers above. @@ +141,5 @@ > NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_AMBIGUOUS(Geolocation, nsIDOMGeoGeolocation) > > NS_DECL_NSIGEOLOCATIONUPDATE > NS_DECL_NSIDOMGEOGEOLOCATION > Please remove this empty line so NS_DECL_NSIDOMEVENTLISTENER below is grouped with this NS_DECL_NSI* block. @@ +147,1 @@ > Geolocation(); Please add an empty line between NS_DECL_NSIDOMEVENTLISTENER and the Geolocation() constructor.
Comment 19•8 years ago
|
||
Comment on attachment 8690670 [details] [diff] [review] [WIP Nov. 23] proposal of how geolocation service acts in terms of application visibility Review of attachment 8690670 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I agree with what cpeterson said and add a few more comments. ::: dom/geolocation/nsGeolocation.cpp @@ +1256,5 @@ > + nsString GpsLockState; > + nsString GpsLockName; > + GpsLockName = NS_LITERAL_STRING("Gps") ; > + WakeLockInformation info; > + GetWakeLockInfo(GpsLockName,&info); Space after comma @@ +1259,5 @@ > + WakeLockInformation info; > + GetWakeLockInfo(GpsLockName,&info); > + uint64_t myID = 0; > + ContentChild* cpc = ContentChild::GetSingleton(); > + myID = cpc->GetID(); Here we should not assume ContentChild. When XRE_IsParentProcess() is true we should use 0 as process ID. @@ +1263,5 @@ > + myID = cpc->GetID(); > + bool thisProcessLocks = info.lockingProcesses().Contains(myID); > + if(!thisProcessLocks){ > + for (uint32_t i = 0 ; i < mLastWatchId; ++i) { > + ClearWatch(i); I still think we should notify the content via error callback instead of drop the watch silently. We may also want to adjust the way we collect mozilla::Telemetry::GEOLOCATION_ERROR.
Attachment #8690670 -
Flags: feedback?(kchen) → feedback+
Assignee | ||
Comment 20•8 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #19) > Comment on attachment 8690670 [details] [diff] [review] > [WIP Nov. 23] proposal of how geolocation service acts in terms of > application visibility > > Review of attachment 8690670 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good to me. > I agree with what cpeterson said and add a few more comments. > > ::: dom/geolocation/nsGeolocation.cpp > @@ +1256,5 @@ > > + nsString GpsLockState; > > + nsString GpsLockName; > > + GpsLockName = NS_LITERAL_STRING("Gps") ; > > + WakeLockInformation info; > > + GetWakeLockInfo(GpsLockName,&info); > > Space after comma > > @@ +1259,5 @@ > > + WakeLockInformation info; > > + GetWakeLockInfo(GpsLockName,&info); > > + uint64_t myID = 0; > > + ContentChild* cpc = ContentChild::GetSingleton(); > > + myID = cpc->GetID(); > > Here we should not assume ContentChild. When XRE_IsParentProcess() is true > we should use 0 as process ID. > @@ +1263,5 @@ > > + myID = cpc->GetID(); > > + bool thisProcessLocks = info.lockingProcesses().Contains(myID); > > + if(!thisProcessLocks){ > > + for (uint32_t i = 0 ; i < mLastWatchId; ++i) { > > + ClearWatch(i); > > I still think we should notify the content via error callback instead of > drop the watch silently. We may also want to adjust the way we collect > mozilla::Telemetry::GEOLOCATION_ERROR. Thanks Peterson and Kan-Ru. And do you guys have any suggestion for a new PositionError.code? It seems that the three original PositionError.code don't fit into this situation. Any thoughts?
Comment 21•8 years ago
|
||
(In reply to Ya-Chieh Wu from comment #20) > (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #19) > > I still think we should notify the content via error callback instead of > > drop the watch silently. We may also want to adjust the way we collect > > mozilla::Telemetry::GEOLOCATION_ERROR. > > Thanks Peterson and Kan-Ru. > And do you guys have any suggestion for a new PositionError.code? > It seems that the three original PositionError.code don't fit into this > situation. > Any thoughts? That's a good question. I think POSITION_UNAVAILABLE would be appropriate because the geolocation position will no longer be available to the backgrounded app. We can't define new PositionError codes and the API request was not denied by the user (PERMISSION_DENIED) and did not timeout (TIMEOUT). Quoting from the API documentation: POSITION_UNAVAILABLE: The acquisition of the geolocation failed because one or several internal source of position returned an internal error. https://developer.mozilla.org/en-US/docs/Web/API/PositionError/code
Assignee | ||
Comment 22•8 years ago
|
||
In this patch, we tried to do two things. (1) By default, we will remove the gps-using requests if app is invisible and send back a "POSITION_UNAVAILABLE"[1] error to the app. (2) Using Navigator.requestWakeLock() method of the Wake Lock API to keep gps-using requests on in the background. [1]https://developer.mozilla.org/en-US/docs/Web/API/PositionError/code [2]https://developer.mozilla.org/en-US/docs/Web/API/Navigator/requestWakeLock
Attachment #8695144 -
Flags: review?(kchen)
Comment 23•8 years ago
|
||
Comment on attachment 8695144 [details] [diff] [review] clean up the background GPS usage if app doesn't ask to run in the background Review of attachment 8695144 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/geolocation/nsGeolocation.cpp @@ +1234,5 @@ > > +NS_IMETHODIMP > +Geolocation::HandleEvent(nsIDOMEvent* aEvent) > +{ > + if (!XRE_IsContentProcess()) { When does this happen? If the geolocation service is not supposed to be receiving events in a given process, should this XRE_IsContentProcess() check before moved before calling AddSystemEventListener() above? @@ +1253,5 @@ > + WakeLockInformation info; > + GetWakeLockInfo(GpsLockName,&info); > + uint64_t ChildID = ContentChild::GetSingleton()->GetID(); > + bool thisProcessLocks = info.lockingProcesses().Contains(ChildID); > + if( !thisProcessLocks ){ nit: Mozilla Codying Sytle says spaces should be outside the parens, not inside. Like "if (!thisProcessLocks) {".
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → ywu
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #23) > Comment on attachment 8695144 [details] [diff] [review] > clean up the background GPS usage if app doesn't ask to run in the > background > > Review of attachment 8695144 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/geolocation/nsGeolocation.cpp > @@ +1234,5 @@ > > > > +NS_IMETHODIMP > > +Geolocation::HandleEvent(nsIDOMEvent* aEvent) > > +{ > > + if (!XRE_IsContentProcess()) { > > When does this happen? If the geolocation service is not supposed to be > receiving events in a given process, should this XRE_IsContentProcess() > check before moved before calling AddSystemEventListener() above? > Thanks Peterson. That is a good point. It's true that the service shouldn't register this event. thanks
Assignee | ||
Comment 25•8 years ago
|
||
take Peterson's comments to fix some problems in the last patch
Attachment #8695201 -
Flags: review?(kchen)
Attachment #8695201 -
Flags: feedback?(cpeterson)
Comment 26•8 years ago
|
||
Comment on attachment 8695201 [details] [diff] [review] clean up the background GPS usage if app doesn't ask to run in the background Review of attachment 8695201 [details] [diff] [review]: ----------------------------------------------------------------- LGTM :)
Attachment #8695201 -
Flags: feedback?(cpeterson) → feedback+
Assignee | ||
Updated•8 years ago
|
Attachment #8695201 -
Flags: review?(dougt)
Comment 27•8 years ago
|
||
Comment on attachment 8695201 [details] [diff] [review] clean up the background GPS usage if app doesn't ask to run in the background Review of attachment 8695201 [details] [diff] [review]: ----------------------------------------------------------------- This introduces a odd side effect behavior. upon putting an application in the background, the geolocation listener will get an error. then when the application is brought into the foreground, the application will no longer receive geolocation events. so, now all applications that use watchPosition(), will not have to re-register upon app visible change I think it would be better if you just prevented the geolocation change events from the provide to be posted to the child geolocation process. In this way, the applications do not have to do anything special. If an application in the background, times out, the current code will send an Timeout error message anyhow. please think about it, and let me know
Attachment #8695201 -
Flags: review?(dougt) → review-
Assignee | ||
Comment 28•8 years ago
|
||
I have some different thoughts. First, if we just not send the position change events back to the child geolocation process, the GPS is still eating the battery. And we still not fix this battery drain problem. On Android, it will stop the service as soon as the application get switched to the background. Second, I think if it will be a bit late to wait until an timeout to notify the application. Would it be long for some cases? I think how about we keep the designs like the patch and try to resume or send an error message to the application when we switch the application back to the foreground?
Comment 29•8 years ago
|
||
Doug makes a very good point. If the design requirements are: 1. Allow the GPS to be turned off when a GPS-using app is backgrounded. 2. Do not require the app developer to write any special code to handle foreground/background transitions. It sounds like when an app is backgrounded, we want to turn off the GPS (assuming the new foreground does not use the GPS) but remember the registered watchPosition callbacks and parameters. When the app is foregrounded, we turn on the GPS and *Gecko*, not the app, automatically "reconnect" the app's watchPosition callbacks to GPS. The app would not need to be notified that anything special happened. Calculating watchPosition timeouts for a backgrounded app might be a little tricky.
Comment 30•8 years ago
|
||
Comment on attachment 8690670 [details] [diff] [review] [WIP Nov. 23] proposal of how geolocation service acts in terms of application visibility Review of attachment 8690670 [details] [diff] [review]: ----------------------------------------------------------------- Comment 27 covers the issues, I'll be interested in seeing how those can be addressed.
Attachment #8690670 -
Flags: feedback?(garvankeeley+bmo)
Comment 31•8 years ago
|
||
I agree that it's better to not require the app developer to handle foreground/background transition. Othe the other hand, I prefer explicit over implicit. The proposal by Doug just delay the error to a later timeout error, the app still needs to re-register. Automatically "reconnect" or resume the watchPositions sounds nice, but what should the app callback receive when it goes to background? Because we do not pause the app when it goes to background, it could notice the callback does not fire; it could still register more watch requests. I think most web pages will handle the error callback the same no matter what the error message is so I prefer a new error code. If Doug thinks that a new error code is not desirable, I could live with using timeout. If we accept the watchPosition could enter a stalled state that does not fire either the callback or error callback when page is hidden then automatically pause/resume is better then error. Whatever method we choose, we should try to update the spec to reference this behavior with the visibility spec.
Updated•8 years ago
|
Attachment #8695144 -
Flags: review?(kchen)
Updated•8 years ago
|
Attachment #8695201 -
Flags: review?(kchen)
Comment 32•8 years ago
|
||
TLDR see my last line (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #31) > I agree that it's better to not require the app developer to handle > foreground/background transition. Othe the other hand, I prefer explicit > over implicit. The proposal by Doug just delay the error to a later timeout > error, the app still needs to re-register. I misunderstood Doug's proposal that I agreed with, and because I am too lazy to re-read :), I'll explain my understanding instead. Basically, watchPosition needs a "paused" state, which it enters when the requesting process is backgrounded. This is implicit and not managed by the developer. When process is foregrounded, watchPositions are returned to their active state. > Automatically "reconnect" or resume the watchPositions sounds nice, but what > should the app callback receive when it goes to background? I was thinking because of the implicit pausing, they would receive nothing. > not pause the app when it goes to background, it could notice the callback > does not fire; it could still register more watch requests. This is true. This means the app developer has an additional check for location timeout instead of relying on the watchPosition callback –but it would be bad design IMHO (and hopefully very rare). If the app chose to register additional watchPosition requests, I think this would be harmless when the app got foregrounded again. > If we accept the watchPosition could enter a stalled state that does not > fire either the callback or error callback when page is hidden then > automatically pause/resume is better then error. Aha. Agreed. I didn't need to write all the above, and just agree with this paragraph :).
Assignee | ||
Comment 33•8 years ago
|
||
In this patch, I tried to stop backgrounded app to receive the geolocation change events by removing the corresponding request in the parent side. And then, when this app is foregrounded, I will go through all the requests to recreate the one corresponding request in the parent side. The new request in the parent side will be almost the same as the original one except the watchId. Finally, if backgrounded app wants to keep receiving events, it needs to request lock = window.navigator.requestWakeLock('gps') in the app.
Attachment #8690670 -
Attachment is obsolete: true
Attachment #8695144 -
Attachment is obsolete: true
Attachment #8695201 -
Attachment is obsolete: true
Attachment #8703478 -
Flags: review?(dougt)
Assignee | ||
Updated•8 years ago
|
Attachment #8703478 -
Flags: review?(kchen)
Comment 34•8 years ago
|
||
Comment on attachment 8703478 [details] [diff] [review] handle backgrounded app's gps usage Review of attachment 8703478 [details] [diff] [review]: ----------------------------------------------------------------- The approach described in comment 33 looks good to me but this patch doesn't match the description. ::: dom/geolocation/nsGeolocation.cpp @@ +1247,5 @@ > +{ > + nsAutoString type; > + aEvent->GetType(type); > + if (!type.EqualsLiteral("visibilitychange")) { > + return NS_ERROR_FAILURE; return NS_OK here. @@ +1257,5 @@ > + if (doc->Hidden()) { > + nsString GpsLockName; > + GpsLockName = NS_LITERAL_STRING("gps"); > + WakeLockInformation info; > + GetWakeLockInfo(GpsLockName,&info); Use NS_LITERAL_STRING("gps") here directly. nit: one space after comma @@ +1258,5 @@ > + nsString GpsLockName; > + GpsLockName = NS_LITERAL_STRING("gps"); > + WakeLockInformation info; > + GetWakeLockInfo(GpsLockName,&info); > + ContentChild* cpc = ContentChild::GetSingleton(); You may want to MOZ_ASSERT(XRE_IsContentProcess()) if you don't want to null check cpc. @@ +1270,5 @@ > + mWatchingCallbacks[i]->Allow(JS::UndefinedHandleValue); > + } > + for (uint32_t i = 0, length = mPendingCallbacks.Length(); i < length; ++i) { > + mPendingCallbacks[i]->Allow(JS::UndefinedHandleValue); > + } Why requests are unconditionally Allowed? I think what you want is nsGeolocationService::StartDevice or a subset of it. @@ +1686,5 @@ > { > if (aRequest->IsWatch()) { > + if (!mWatchingCallbacks.Contains(aRequest)) { > + mWatchingCallbacks.AppendElement(aRequest); > + } Fix HandleEvent then you don't need this change.
Attachment #8703478 -
Flags: review?(kchen) → review-
Assignee | ||
Comment 35•8 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #34) Kan-Ru thanks a lot for reviewing. But I have some questions that I want to discuss. > @@ +1258,5 @@ > > + nsString GpsLockName; > > + GpsLockName = NS_LITERAL_STRING("gps"); > > + WakeLockInformation info; > > + GetWakeLockInfo(GpsLockName,&info); > > + ContentChild* cpc = ContentChild::GetSingleton(); > > You may want to MOZ_ASSERT(XRE_IsContentProcess()) if you don't want to null > check cpc. Do we need to check this here? Only content process will register this event @ +1287. > > @@ +1270,5 @@ > > + mWatchingCallbacks[i]->Allow(JS::UndefinedHandleValue); > > + } > > + for (uint32_t i = 0, length = mPendingCallbacks.Length(); i < length; ++i) { > > + mPendingCallbacks[i]->Allow(JS::UndefinedHandleValue); > > + } > > Why requests are unconditionally Allowed? > > I think what you want is nsGeolocationService::StartDevice or a subset of it. The reason that those requests are unconditionally allowed is because if the requests are in mWatchingCallbacks or mPendingCallbacks, it means that they've been allowed before. That's the reason that we unconditionally allow it again. And the reason why I resume the requests from Allow is because I want them to check if there is any cached data to use before they setup the requests in parent side. Because even this process is suspended, it doesn't mean other processes can't update the cache. That's why I resume them from nsGeolocationRequest::Allow. Does this make sense to you? Thank you again kan-ru.
Comment 36•8 years ago
|
||
(In reply to Ya-Chieh Wu from comment #35) > (In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #34) > > Kan-Ru thanks a lot for reviewing. > But I have some questions that I want to discuss. > > > @@ +1258,5 @@ > > > + nsString GpsLockName; > > > + GpsLockName = NS_LITERAL_STRING("gps"); > > > + WakeLockInformation info; > > > + GetWakeLockInfo(GpsLockName,&info); > > > + ContentChild* cpc = ContentChild::GetSingleton(); > > > > You may want to MOZ_ASSERT(XRE_IsContentProcess()) if you don't want to null > > check cpc. > > Do we need to check this here? Only content process will register this event > @ +1287. Yes, the assert will ensure that we only register this event in content process. It documents the assumption we made and also notify us when the assumption no longer hold. > > @@ +1270,5 @@ > > > + mWatchingCallbacks[i]->Allow(JS::UndefinedHandleValue); > > > + } > > > + for (uint32_t i = 0, length = mPendingCallbacks.Length(); i < length; ++i) { > > > + mPendingCallbacks[i]->Allow(JS::UndefinedHandleValue); > > > + } > > > > Why requests are unconditionally Allowed? > > > > I think what you want is nsGeolocationService::StartDevice or a subset of it. > > The reason that those requests are unconditionally allowed is because if the > requests are in mWatchingCallbacks or mPendingCallbacks, it means that > they've been allowed before. > That's the reason that we unconditionally allow it again. > > And the reason why I resume the requests from Allow is because I want them > to check if there is any cached data to use before they setup the requests > in parent side. Because even this process is suspended, it doesn't mean > other processes can't update the cache. That's why I resume them from > nsGeolocationRequest::Allow. It means as soon as a page becomes visible, it receives an update if cache is allowed; that's probably fine. Do we really want to reset the timer? It also means if we only use the cache and never call StartDevice the watch callback will never receive more updated position. It seems a bug to me. Could you file a separate bug if it's true? > Does this make sense to you? > Thank you again kan-ru. yes, please r? again.
Assignee | ||
Comment 37•8 years ago
|
||
Hey Kan-Ru, as the comment #36, we make a early return in nsGeolocationRequest::Allow if the request is already in either the callback arrays so we wouldn't reset the timmer later. And yes, the problem that if we take the cached data and without other requests which setup the connection in the parent side, then we can't get any later update is true. I will handle this after I put more insepction. Thank you
Attachment #8703478 -
Attachment is obsolete: true
Attachment #8703478 -
Flags: review?(dougt)
Attachment #8706202 -
Flags: review?(kchen)
Comment 38•8 years ago
|
||
Comment on attachment 8706202 [details] [diff] [review] How geolocation service acts when the visibility changes Review of attachment 8706202 [details] [diff] [review]: ----------------------------------------------------------------- r=me with nits fixed ::: dom/geolocation/nsGeolocation.cpp @@ +1254,5 @@ > +Geolocation::requestContains(nsGeolocationRequest* aRequest) > +{ > + if (aRequest->IsWatch()) { > + if (mWatchingCallbacks.Contains(aRequest)) > + return true; Gecko style requires every if clause be followed by braces, ie. if (...) { ... } @@ +1266,5 @@ > + > +NS_IMETHODIMP > +Geolocation::HandleEvent(nsIDOMEvent* aEvent) > +{ > + MOZ_ASSERT(XRE_IsContentProcess()); Move this line just before using ContentChild::GetSingleton() ::: dom/geolocation/nsGeolocation.h @@ +159,5 @@ > // Register an allowed request > void NotifyAllowedRequest(nsGeolocationRequest* aRequest); > > + // Check if callbacks arrays already contain this request > + bool requestContains(nsGeolocationRequest* aRequest); Function name should use CamelCase
Attachment #8706202 -
Flags: review?(kchen) → review+
Assignee | ||
Comment 40•8 years ago
|
||
Below is the result of try server. It looks fine. https://treeherder.mozilla.org/#/jobs?repo=try&revision=033f94656d35
Attachment #8706202 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
OS: Gonk (Firefox OS) → All
Hardware: ARM → All
Comment 41•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffeff522119
Keywords: checkin-needed
Comment 42•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0ffeff522119
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment 43•8 years ago
|
||
Kanru: Will there be a problem for some documents asking for geolocation in Firefox where: 1) document requests geolocation 2) user clicks accepts 3) user *quickly* switches away from the document I am not sure Then thing about calling Allow() unconditionally also tripped me up. This should have had a comment. Please add it in a follow. Also, the comment #36 presents a real problem. If you always return a cached location, if the device (or laptop) moves, you will not kick off another Geo. Is that right?
Flags: needinfo?(kchen)
Comment 44•8 years ago
|
||
(In reply to Doug Turner (:dougt) from comment #43) > Kanru: > > Will there be a problem for some documents asking for geolocation in Firefox > where: > > 1) document requests geolocation > 2) user clicks accepts > 3) user *quickly* switches away from the document > > I am not sure In current design if it's a watch request without timeout, it will resume when user switches back to the document. In other cases they will just timeout. Do you think we should add some grace period before suspend the request? And for Desktop and Fennec that don't have wake lock exposed, maybe we should not ship this for them yet. > Then thing about calling Allow() unconditionally also tripped me up. This > should have had a comment. Please add it in a follow. Ya-Chieh, could you please add a comment? > Also, the comment #36 presents a real problem. If you always return a > cached location, if the device (or laptop) moves, you will not kick off > another Geo. Is that right? I think so. That should be addressed by bug 1238873.
Flags: needinfo?(ywu)
Flags: needinfo?(kchen)
Flags: needinfo?(dougt)
Assignee | ||
Comment 45•8 years ago
|
||
(In reply to Kan-Ru Chen [:kanru] (UTC+8) from comment #44) > (In reply to Doug Turner (:dougt) from comment #43) > > Kanru: > > > > Will there be a problem for some documents asking for geolocation in Firefox > > where: > > > > 1) document requests geolocation > > 2) user clicks accepts > > 3) user *quickly* switches away from the document > > > > I am not sure > Does dougt means if it could happened that we handle the visibility change event before the requests finally are put into the callbacks array? Like very *quickly* switches away from the document. If so, we can put an additional check in Allow(). > > > Then thing about calling Allow() unconditionally also tripped me up. This > > should have had a comment. Please add it in a follow. > > Ya-Chieh, could you please add a comment? Yes, I will do it.
Flags: needinfo?(ywu)
Reporter | ||
Comment 46•8 years ago
|
||
I can validate that the power issue with respect to this bug has been fixed: https://raptor.mozilla.org/dashboard/script/power?var-device=flame-kk&var-memory=1024&var-branch=master&from=1452067128535&to=1453366733542 You can clearly see the transition on Jan 13/14.
Comment 47•8 years ago
|
||
Is there some way for a user to turn off this "feature"? I can't upgrade my phone to a build that turns off GPS as I need to continue to run FxStumbler and my GPS tracking app. Also, is there a way for a simple hosted app (like lantea.kairo.at that I use for GPS tracking) to keep the GPS from being turned off by this "feature"? To me as a user of GPS tracking and GPS-aware apps like FxStumbler, this very much sounds like an anti-feature by all means.
Assignee | ||
Comment 48•8 years ago
|
||
Hey Robert, I think what users gain from this feather is more than what they lose. Gps is known for the speed to eat battery. For the Fennec/FFOS users, they properly don't want to find out that they forget to close the google map in non-focused tabs/apps when the battery is almost dead. On FFOS, the developers can use wake lock[1] to keep gps awake when app is invisible. On Fennec, in my opinion, most of the users won't use a browser as a gps tracking app. At this point, I think this would be a better default behavior for users. But I agree that it is a good idea to maybe leave users a bit more control on this. [1]https://developer.mozilla.org/en-US/docs/Web/API/Wake_Lock_API/Keeping_the_geolocation_on_when_the_application_is_invisible thanks!
Comment 49•8 years ago
|
||
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #47) > Is there some way for a user to turn off this "feature"? I can't upgrade my > phone to a build that turns off GPS as I need to continue to run FxStumbler > and my GPS tracking app. > > Also, is there a way for a simple hosted app (like lantea.kairo.at that I > use for GPS tracking) to keep the GPS from being turned off by this > "feature"? > > To me as a user of GPS tracking and GPS-aware apps like FxStumbler, this > very much sounds like an anti-feature by all means. By the way, we have enabled stumbler on FxOS in Bug 1154435.
Comment 50•8 years ago
|
||
(In reply to Alphan Chen [:alchen] from comment #49) > By the way, we have enabled stumbler on FxOS in Bug 1154435. If it's something that I can watch, turn on and off, have on all the time when traveling and have only send data when I want, then it can replace FxStumbler. Otherwise, if it e.g. is only like the FxAndroid stumbling stuff, then we still need a separate stumbler app. That said, this stumbling would also use battery, is it turned off randomly by this "feature" as well and made pretty useless therefore?
Comment 51•8 years ago
|
||
> If it's something that I can watch, turn on and off, have on all the time > when traveling and have only send data when I want, then it can replace > FxStumbler. Otherwise, if it e.g. is only like the FxAndroid stumbling > stuff, then we still need a separate stumbler app. Some items on why FxStumbler is not an app that guides the direction of b2g geo work: • FxStumbler -while it is a very cool app- is not recommended my MLS staff, see here: https://github.com/mozilla/MozStumbler/issues/780#issuecomment-163381812 • While I worked on MLS, the data contribution level from FxStumbler was inconsequential. > That said, this stumbling would also use battery, is it turned off randomly > by this "feature" as well and made pretty useless therefore? It is passive collection, another app must be using the GPS already. The effect of performing a periodic wi-fi and cell scan are inconsequential compared to the effect of the GPS being on.
Comment 52•8 years ago
|
||
Kanru -- back out this asap. I believe it caused Bug 784505. (if not, let me know). Also, I want to review the next time you land. We can not cause these sorts of failures in Firefox
Status: RESOLVED → REOPENED
Flags: needinfo?(dougt)
Resolution: FIXED → ---
Assignee | ||
Comment 53•8 years ago
|
||
This is the problem that b2g and browser are different structures but we used the same gecko. On b2g, an app will have its own nsGeolocationSerive but on browser, it uses a singleton nsGeolocationSerive. This sometimes introduce different behaviors. Another example is that we save the cache in nsGeolocationSerive. On browser, it's good that tabs can share the same cache but it's weird for b2g. On b2g, every app will have its own cache. There is no such thing as a singleton nsGeolocationSerive on b2g. So a new launched app could not get the cached position even if there are other gps apps which already get updated position. Back to this bug, I think this feature was creating to solve the power issue for devices at first so it's good to me to back out this patch first. I still think this is critical on devices but maybe this kind of "the behavior that turn off the invisible document's gps" doesn't really fit on browser? many thanks!
Comment 54•8 years ago
|
||
We will fix this in bug 1240664. If it's too time consuming we can back this out first.
Depends on: 1240664
Comment 55•8 years ago
|
||
*Please* see: - https://github.com/slightlyoff/ServiceWorker/issues/745 IMHO the following is also well worth a look : - https://github.com/w3c/geofencing-api/issues/25 Also, Chromes bugfix for the same problem can be found here: - https://code.google.com/p/chromium/issues/detail?id=112938 WAKE_LOCKs or CPU_LOCKs or GPS_LOCKs, are a retrograde step with battery consumption and, curiously, ignore the best background processing since slice-bread "Service Workers"!
Comment 56•8 years ago
|
||
I am curious about the lack of importance flagged against this bug. FireFox is facilitating the unauthorized tracking of users by unscrupulous sites, and is not in a hurry to do anything about it :-( The user may well be happy to consent but the fact that they are oblivious to what's going on cannot be good surely? This doesn't sound like the mozilla way or, for that matter, the way of anyone who doesn't want to open themselves up to litigation.
Comment hidden (abuse-reviewed) |
Assignee | ||
Comment 59•8 years ago
|
||
Hey Ryan, I think Jon Hylands reported this for b2g and as Comment 46, he also validated that the power issue is solved. So maybe Chris could help to answer if we should close this bug report. Thanks!
Flags: needinfo?(ywu) → needinfo?(cpeterson)
Comment 60•8 years ago
|
||
(In reply to Richard Maher from comment #58) Per comment 54, this should have been fixed for Firefox 46+ via bug 1240664. Can you reproduce on a current DevEdition or Nightly build?
Comment 61•8 years ago
|
||
I don't know the current state of this bug. IIUC, bug 1240664 made the "turn off geolocation when backgrounded" fix only work on Firefox OS, not desktop or Android. I think physical tracking of user movement is less of a concern for desktop or laptop computers than for mobile devices. Also, the user had to explicitly grant the web page permission to access geolocation.
Flags: needinfo?(cpeterson)
Comment 62•8 years ago
|
||
@59 I wouldn't be so quick to paraphrase Jon Hylands. #46 was a side issue about power. #1 was about a vulnerability in Firefox that lets unscrupulous sites track users and devices. @60 The "fix" only related to FxOS as wakeLock is currently unsupported on FF Android. See #9 on the bug you quoted. But yes, I have tried Nightly (47.0a1) and it still let's stalkers and estranged spouses follow their vitims around. You all must be ecstatic! @61 So Chris does not think Android is a "mobile device"? So the user grants location access and then thinks they have close the app but you keep your dossier running? Are there no adults around here? Can we get this out into the Computing press so that someone may shame these identity theieves into doing something? onVisibilityChange {go to sleep} How hard can it be?
Reporter | ||
Comment 63•8 years ago
|
||
I created this bug as a power issue affecting Firefox OS phones. It has since morphed into much more, but I haven't really kept up with it. The power issue was fixed at one point in time (per comment 46), but my understanding is the fix had other side effects that were also undesirable.
Comment hidden (abuse-reviewed) |
Comment 65•8 years ago
|
||
Richard: insults are not persuasive. Please keep your criticisms constructive.
Comment hidden (abuse-reviewed) |
(In reply to Richard Maher from comment #64) > The bottom line is that if someone closes this bug I'll immediately open > another one for Firefox browser. It's up to you. Would you mind doing that anyway, even if this one isn't closed? This bug is about Firefox OS. A big part of the reason people are pushing back so much is that we really try to keep one bug report per problem, which allows us to track the progress of the fix. That's the point of a bug tracking system. (If we didn't do that we could just have one bug called "Firefox has bugs" and put all the problems there.) This bug report is about Firefox OS. A separate bug report about Firefox would be much clearer than discussing Firefox issues in this one.
Comment hidden (offtopic) |
Comment hidden (offtopic) |
Summary: Geolocation does not turn off with browser backgrounded → Geolocation does not turn off with Firefox OS browser backgrounded
Comment hidden (abuse-reviewed) |
Comment 71•8 years ago
|
||
The Firefox OS bug has been fixed. For Firefox Desktop or mobile, please move to bug 1254911.
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment hidden (offtopic) |
See Also: → 1254911
See Also: → 784505
You need to log in
before you can comment on or make changes to this bug.
Description
•