823789 - geolocation.watchposition position_options ignored

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

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

position_options are getting lost when the switch from content to b2g process occurs.

Net effect is that enableHighAccuracy,timeout,maximumAge does not work when running from a content process.   Works fine when OOP is disabled.


Quick Analysis:
1) Content process calls
http://mxr.mozilla.org/mozilla-beta/source/dom/src/geolocation/nsGeolocation.cpp#932
2) On the parent process side we then forge a dummy position_options of JSVAL_VOID (http://mxr.mozilla.org/mozilla-beta/source/dom/ipc/ContentParent.cpp#1903) and pass that void along to the nsIGeolocationProviders

Comment 1

[Doug Turner (:dougt)]

josh, you interested in an e10s bug?

Comment 2

[Josh Matthews [:jdm]]

Michael, are you positive that timeout and maximumAge don't work? There's no reason I can see that they should not, since all of the relevant bits of code are executed in the content process.

Comment 3

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

Yep, pretty sure.   nsIGeolocationProviders run in the system process, and the position_options are getting lost [1] as they exit the content process, so the geolocation providers never get them.

[1] http://mxr.mozilla.org/mozilla-beta/source/dom/ipc/ContentParent.cpp#1903

Comment 4

[Josh Matthews [:jdm]]

Attachment: Pass enableHighAccuracy geolocation changes to the chrome process.

Indeed, I have a test which demonstrates that timeouts certainly work in content processes in contrived conditions.

Comment 5

[Josh Matthews [:jdm]]

Michael, the providers don't need the timeout values. The timeouts are only needed in the content process where the timers run.

Comment 6

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

(In reply to Josh Matthews [:jdm] from comment #5)
> Michael, the providers don't need the timeout values. The timeouts are only
> needed in the content process where the timers run.

Ah, missed that.  Sorry.  I checked with the original reporter for this issue and looks like only real complaint was the lack of the enableHighAccuracy arriving at the provider.  The rest was creative extrapolation.  :)

So attachment 695517 [details] [diff] [review] lgtm!

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 695517 [details] [diff] [review]
Pass enableHighAccuracy geolocation changes to the chrome process.

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

::: dom/tests/unit/test_geolocation_timeout.js
@@ +51,5 @@
> +
> +  if (Cc["@mozilla.org/xre/app-info;1"].getService(Ci.nsIXULRuntime)
> +        .processType == Ci.nsIXULRuntime.PROCESS_TYPE_DEFAULT) {
> +    var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +    prefs.setCharPref("geo.wifi.uri", "http://localhost:4444/geo");  

trailing ws

::: dom/tests/unit/test_geolocation_timeout_wrap.js
@@ +2,5 @@
> +const Ci = Components.interfaces;
> +
> +function run_test() {
> +  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +  prefs.setCharPref("geo.wifi.uri", "http://localhost:4444/geo");  

trailing ws

Comment 8

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3114517c141a

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 695517 [details] [diff] [review]
Pass enableHighAccuracy geolocation changes to the chrome process.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Confusion and brokenness if we base b2g builds off of aurora.
Testing completed (on m-c, etc.): manual testing
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Broken high geolocation accuracy for all content.
Testing completed: manual testing
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 10

[Josh Matthews [:jdm]]

Backed out for suprising linux dbus crashes: https://hg.mozilla.org/integration/mozilla-inbound/rev/06c61e9f344a

https://tbpl.mozilla.org/php/getParsedLog.php?id=18311010&tree=Mozilla-Inbound

Comment 11

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2606506e12dc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2606506e12dc

Comment 13

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3a75155ac44d
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d1349e9be2af

Comment 14

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df31048b8060
https://hg.mozilla.org/releases/mozilla-b2g18/rev/23a6717c6d3f

I landed an old version on aurora and b2g18.

Comment 15

[Josh Matthews [:jdm]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5e7fae488345
https://hg.mozilla.org/releases/mozilla-aurora/rev/39a162ff4025

Comment 16

[:gerard-majax]

Does it means bug 806219 is fixed?