811470 - geolocation ipc does not return error codes to child processes

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Doug Turner (:dougt)]

If you return an error, from say, StartDevice().  That error does not ever end up in the child process.  The result is that the child, if having set an error callback, will never notice/see that error.

I/Gecko   ( 6172): nsGeolocationRequest::Allow
I/Gecko   ( 6172): StartDevice called 0 1 1
I/Gecko   ( 6172): starting devices
I/Gecko   ( 6096): attempting to get setttings;
I/Gecko   ( 6096): got settings;
I/Gecko   ( 6096): created lock;
I/Gecko   ( 6096): getting geo setting;
I/Gecko   ( 6096): ok;
I/Gecko   ( 6096): nsGeolocationService::HandleMozsettingValue 1
I/Gecko   ( 6096): nsGeolocation ServiceReady
I/Gecko   ( 6096): nsGeolocationRequest::Allow
I/Gecko   ( 6096): StartDevice called 0 1 0
I/Gecko   ( 6096): starting devices
I/Gecko   ( 6096): erroring out 1 
I/Gecko   ( 6096): NotifyError 0x0
I/Gecko   ( 6096): NotifyCallback
I/Gecko   ( 6096): HandleMozsettingValue setting sGeoInitPending to 0

Comment 1

[Doug Turner (:dougt)]

minusing.  There is *no* new information here that would suggest that we should block.

Comment 2

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

Dougt, I think this should still go through triage for tracking purposes.  

I would like to have this tracked; not necessarily blocking.

I think it's important to have this for user feedback when we do fail due to some error, so that the user understands why the geolocation failed.

Comment 3

[mjh563]

Is this bug still valid? How can it be reproduced?

Comment 4

[Josh Matthews [:jdm]]

It could be reproduced via an IPC xpcshell test that replaces the geolocation provider service with one that throws in StartDevice. The patch at https://gist.github.com/jdm/6862469 has an example of how to do these things.

Comment 5

[Josh Matthews [:jdm]]

We'll need a test as described in comment 4, and ContentParent (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.h) will need to implement the nsIDOMGeoPositionErrorCallback interface (http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/geolocation/nsIDOMGeoPositionErrorCallback.idl), passing itself as the error listener to the WatchPosition call.

Comment 6

[Josh Matthews [:jdm]]

Furthermore, another IPC message for passing errors cross-process will be required in PContent.ipdl (http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#274).

Comment 7

[Vinay G Shetty]

hey jdm,

Can you please explain a bit about the sentence "passing itself as the error listener to the WatchPosition call" in comment 5. 

i have started looking into this bug. I am new this. I want your help

Comment 8

[Josh Matthews [:jdm]]

(In reply to Vinay G Shetty from comment #7)
> hey jdm,
> 
> Can you please explain a bit about the sentence "passing itself as the error
> listener to the WatchPosition call" in comment 5. 
> 
> i have started looking into this bug. I am new this. I want your help

https://developer.mozilla.org/en-US/docs/Web/API/Geolocation.watchPosition explains about the API that we're using (well, http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/geolocation/nsIDOMGeoGeolocation.idl#23 is the actual API we are specifically calling). You'll note that in AddGeolocationListener in ContentParent.cpp, we currently take an nsIDOMGeoPositionCallback parameter (which is just |this| in the caller). We also want to pass an error callback object, which should also be implemented by ContentParent.

Comment 9

[Vinay G Shetty]

Attachment: 811470.patch

Please review the code changes consisting for implementation of nsIDOMGeoPositionErrorCallback interface in ContentParent.cpp.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 8503296 [details] [diff] [review]
811470.patch

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

This is a good start! However, we should write a test that shows the changes are correct, since that would catch the fact that the new error callback isn't actually used in the current patch. We should be able to modify /dom/tests/unit/test_geolocation_timeout.js to check that the code property of the error object received in the callback is the timeout code; you can run it with ./mach xpcshell-test dom/tests/unit/test_geolocation_timeout_wrap.js (the _wrap makes sure that it is run in a child process, so we execute the code you're changing).

::: dom/ipc/ContentParent.cpp
@@ +3556,5 @@
>      return true;
>  }
>  
>  static int32_t
> +AddGeolocationListener(nsIDOMGeoPositionCallback* watcher, nsIDOMGeoPositionErrorCallback* errorCallBack, bool highAccuracy)

Note that we're not using the new argument.

@@ +3629,5 @@
>  
> +NS_IMETHODIMP
> +ContentParent::HandleEvent(nsIDOMGeoPositionError* postionError)
> +{
> +    unused << SendGeolocationUpdate(GeoPosition(postionError));

I don't see how this compiles. You're going to need to send a different message than GeolocationUpdate that takes an error code instead. See the messages defined at http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#435.

::: dom/ipc/ContentParent.h
@@ +68,5 @@
>  class ContentParent MOZ_FINAL : public PContentParent
>                                , public nsIContentParent
>                                , public nsIObserver
>                                , public nsIDOMGeoPositionCallback
> +							  , public nsIDOMGeoPositionErrorCallback	

nit: please match the existing indentation (using spaces, not tabs) and remove the trailing whitespace

@@ +154,5 @@
>  
>      NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>      NS_DECL_NSIOBSERVER
>      NS_DECL_NSIDOMGEOPOSITIONCALLBACK
> +	NS_DECL_NSIDOMGEOPOSITIONERRORCALLBACK

nit: spaces not tabs

::: dom/ipc/PContent.ipdl
@@ +602,5 @@
>                         int64_t aContentLength,
>                         OptionalURIParams aReferrer,
>                         nullable PBrowser aBrowser);
>  
> +    AddGeolocationListener(Principal principal, Principal aPrincipal, bool highAccuracy);

What happened here?

Comment 11

[Vinay G Shetty]

Attachment: 811470_2.patch

Comment 12

[Vinay G Shetty]

Comment on attachment 8505691 [details] [diff] [review]
811470_2.patch

Please review the second set of changes based on jdm review

Vinay

Comment 13

[Vinay G Shetty]

Attachment: 811470(latest).patch

Latest patch with the test js for the errorCallBack implementation. The child process is able to receive the POSITION_UNAVAILABLE error code now apart from the timeout.
Please review the patch

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 8507333 [details] [diff] [review]
811470(latest).patch

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

Good progress! It's mostly just cleanup left at this point, although I'm confused why the test is passing right now.

::: dom/ipc/ContentChild.cpp
@@ +1753,5 @@
>      gs->Update(position);
>      return true;
>  }
>  
> +

nit: unnecessary newline

@@ +1761,5 @@
> +    nsCOMPtr<nsIGeolocationUpdate> gs = do_GetService("@mozilla.org/geolocation/service;1");
> +    if (!gs) {
> +        return true;
> +    }
> +    gs->NotifyError((unsigned short)errorCode);

Is this cast necessary?

::: dom/ipc/ContentChild.h
@@ +24,5 @@
>  struct ResourceMapping;
>  struct OverrideMapping;
>  
> +
> +

nit: unnecessary newlines

@@ +293,5 @@
>                                    const InfallibleTArray<CpowEntry>& aCpows,
>                                    const IPC::Principal& aPrincipal) MOZ_OVERRIDE;
>  
>      virtual bool RecvGeolocationUpdate(const GeoPosition& somewhere) MOZ_OVERRIDE;
> +	

nit: trailing whitespace.

@@ +294,5 @@
>                                    const IPC::Principal& aPrincipal) MOZ_OVERRIDE;
>  
>      virtual bool RecvGeolocationUpdate(const GeoPosition& somewhere) MOZ_OVERRIDE;
> +	
> +	virtual bool RecvGeoPositionErrorMsg(const uint16_t& errorCode) MOZ_OVERRIDE;

nit: please indent using spaces, not tabs.

::: dom/ipc/ContentParent.cpp
@@ +3631,5 @@
> +NS_IMETHODIMP
> +ContentParent::HandleEvent(nsIDOMGeoPositionError* postionError)
> +{
> +    int16_t errorCode;
> +    postionError->GetCode(&errorCode);

This should check the return value and return early if it fails (nsresult rv = ...; NS_ENSURE_SUCCESS(rv, rv)).

@@ +3636,5 @@
> +    unused << SendGeoPositionErrorMsg(errorCode);
> +    return NS_OK;
> +}
> +
> +

nit: unnecessary newline.

::: dom/ipc/ContentParent.h
@@ +68,5 @@
>  class ContentParent MOZ_FINAL : public PContentParent
>                                , public nsIContentParent
>                                , public nsIObserver
>                                , public nsIDOMGeoPositionCallback
> +                              , public nsIDOMGeoPositionErrorCallback	

nit: trailing whitespace

::: dom/ipc/PContent.ipdl
@@ +437,5 @@
>      UpdateDictionaryList(nsString[] dictionaries);
>  
> +	//Send error msg from parent
> +	GeoPositionErrorMsg(uint16_t errorCode);
> +	

nit: please indent using spaces, not tabs. Also, just call this GeolocationError and move it next to the GeolocationUpdate message. No need for the comment.

@@ -606,5 @@
>  
>      AddGeolocationListener(Principal principal, bool highAccuracy);
>      RemoveGeolocationListener();
>      SetGeolocationHigherAccuracy(bool enable);
> -

nit: please re-add this newline.

::: dom/tests/unit/test_geolocation_position_unavailable.js
@@ +7,5 @@
> +
> +var httpserver = null;
> +var geolocation = null;
> +var success = false;
> +var watchId = -1;

This is unused.

@@ +9,5 @@
> +var geolocation = null;
> +var success = false;
> +var watchId = -1;
> +
> +function geoHandler(metadata, response)

No need for the unused geolocation handler.

@@ +36,5 @@
> +  do_test_finished();
> +}
> +
> +function errorCallback(err) {
> +  do_check_eq(2,err.code);

do_check_eq(Ci.nsIDOMGeoPositionError.POSITION_UNAVAILABLE, err.code);

@@ +37,5 @@
> +}
> +
> +function errorCallback(err) {
> +  do_check_eq(2,err.code);
> +  do_print(err.message);  

Remove this line.

@@ +58,5 @@
> +    httpserver.start(-1);
> +    var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +    prefs.setBoolPref("geo.wifi.scan", false);
> +    prefs.setCharPref("geo.wifi.uri", "http://localhost:" +
> +                      httpserver.identity.primaryPort + "/geo");

We shouldn't be starting an http server or setting a real geolocation provider here.

@@ +63,5 @@
> +    prefs.setBoolPref("dom.testing.ignore_ipc_principal", true);
> +  }
> +
> +  geolocation = Cc["@mozilla.org/geolocation;1"].getService(Ci.nsISupports);
> +  geolocation.getCurrentPosition(successCallback, errorCallback, {enableHighAccuracy: true});

No need for enableHighAccuracy.

::: dom/tests/unit/test_geolocation_position_unavailable_wrap.js
@@ +10,5 @@
> +  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +  prefs.setBoolPref("geo.wifi.scan", false);
> +
> +  httpserver = new HttpServer();
> +  httpserver.start(-1);

No need for the httpserver.

@@ +12,5 @@
> +
> +  httpserver = new HttpServer();
> +  httpserver.start(-1);
> +  prefs.setCharPref("geo.wifi.uri", "http://localhost:" +
> +                    httpserver.identity.primaryPort + "/geo");

I'm confused why this test passes when we seem to be providing a real geolocation provider.

::: dom/tests/unit/xpcshell.ini
@@ +18,5 @@
>  skip-if = os == "android"
>  [test_geolocation_reset_accuracy_wrap.js]
>  skip-if = os == "mac" || os == "android"
> +[test_geolocation_position_unavailable.js]
> +#Bug 811470:  geolocation ipc does not return error codes to child processes

No need for this comment.

Comment 15

[Vinay G Shetty]

Attachment: 811470(modified).patch

Did all the cleanup based on previous comments

Comment 16

[Vinay G Shetty]

Attachment: 811470(modified).patch

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 8508917 [details] [diff] [review]
811470(modified).patch

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

Great work! There are just a couple small issues left to fix before this can be committed. Additionally, if you could make the commit message look like "Bug 811470 - Send geolocation error codes to the content process. r=jdm", that would be great.

::: dom/ipc/PContent.ipdl
@@ +434,5 @@
>  
>      GeolocationUpdate(GeoPosition somewhere);
>  
> +    GeolocationError(uint16_t errorCode);
> +	

nit: trailing whitespace.

@@ +439,2 @@
>      UpdateDictionaryList(nsString[] dictionaries);
> +	

nit: trailing whitespace.

@@ +608,5 @@
>  
>      AddGeolocationListener(Principal principal, bool highAccuracy);
>      RemoveGeolocationListener();
>      SetGeolocationHigherAccuracy(bool enable);
> +	

nit: trailing whitespace.

::: dom/tests/unit/test_geolocation_position_unavailable.js
@@ +2,5 @@
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;
> +
> +Cu.import("resource://testing-common/httpd.js");

No need for this import.

@@ +10,5 @@
> +  do_test_finished();
> +}
> +
> +function errorCallback(err) {
> +  do_check_eq(Ci.nsIDOMGeoPositionError.POSITION_UNAVAILABLE,err.code);

nit: space after ,

@@ +27,5 @@
> +    do_get_profile();
> +
> +    var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +    prefs.setBoolPref("geo.wifi.scan", false);
> +    prefs.setCharPref("geo.wifi.uri", "UrlNotUsedHere:"+"/geo");

The extra "geo" bit here isn't necessary.

@@ +32,5 @@
> +    prefs.setBoolPref("dom.testing.ignore_ipc_principal", true);
> +  }
> +
> +  geolocation = Cc["@mozilla.org/geolocation;1"].getService(Ci.nsISupports);
> +  geolocation.getCurrentPosition(successCallback, errorCallback, {});

No need for {}.

::: dom/tests/unit/test_geolocation_position_unavailable_wrap.js
@@ +1,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://testing-common/httpd.js");

No need for this.

@@ +9,5 @@
> +  var prefs = Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch);
> +  prefs.setBoolPref("geo.wifi.scan", false);
> +
> +  //httpserver = new HttpServer();
> +  //httpserver.start(-1);

Remove these.

@@ +10,5 @@
> +  prefs.setBoolPref("geo.wifi.scan", false);
> +
> +  //httpserver = new HttpServer();
> +  //httpserver.start(-1);
> +  prefs.setCharPref("geo.wifi.uri", "UrlNotUsedHere"+"/geo");

No need for the extra bit.

Comment 18

[Josh Matthews [:jdm]]

Oh, and please mark the previous patches obsolete when you attach new versions!

Comment 19

[Vinay G Shetty]

Attachment: 811470.patch

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 8510493 [details] [diff] [review]
811470.patch

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

Fantastic! I'll push this to our tryserver to make sure it passes tests; you should definitely apply for access if you don't have it yet: https://www.mozilla.org/hacking/committer/

Comment 21

[Josh Matthews [:jdm]]

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

Comment 22

[Vinay G Shetty]

Thank You jdm. This feels really great!. Its my first bug. :)

Comment 23

[Josh Matthews [:jdm]]

According to that link, the xpcshell testsuite failed because the manifest change wasn't correct (xpcshell.ini). You can see the specific error by clicking on any of the orange Xs.

Comment 24

[Vinay G Shetty]

Attachment: 811470.patch

I guess it was because of the unwanted if condition in the last line. I have removed that in the current patch.

Comment 25

[Josh Matthews [:jdm]]

I apologize; that appears to be my fault. The change I pushed to the tryserver had a weird bit at the very end: https://hg.mozilla.org/try/rev/94c627c2ffc5#l8.16

Comment 26

[Josh Matthews [:jdm]]

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

Comment 27

[Josh Matthews [:jdm]]

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

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Looks like the version with the broken manifest landed. I pushed an in-place fix. Please double-check in the future :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0a21777c55

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d0c7bece66e3
https://hg.mozilla.org/mozilla-central/rev/4e0a21777c55

Comment 30

[Francis Lee [:frlee]]

hi Bhavana,

due to 1092790 is partner's  blocker and 811470 patch has been verified in m-c, please evaluate the risk to uplift to 2.1

Comment 31

[Josh Matthews [:jdm]]

I would rate the risk as exceedingly low. This is a very simple addition with no moving parts.

Comment 32

[bhavana bajaj [:bajaj]]

(In reply to Francis Lee [:frlee] from comment #30)
> hi Bhavana,
> 
> due to 1092790 is partner's  blocker and 811470 patch has been verified in
> m-c, please evaluate the risk to uplift to 2.1

Given the extreme low risk here and partner request, we can uplift this to 2.1. Please seek approval-mozilla-b2g34? with approval comments for this to get uplifted, when ready.