Closed Bug 811470 Opened 7 years ago Closed 5 years ago

geolocation ipc does not return error codes to child processes

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
blocking-basecamp -
Tracking Status
b2g18 + ---

People

(Reporter: dougt, Assigned: shettyvinayg, Mentored)

Details

(Whiteboard: [lang=c++][Tako_Blocker])

Attachments

(1 file, 6 obsolete files)

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
blocking-basecamp: --- → ?
minusing.  There is *no* new information here that would suggest that we should block.
blocking-basecamp: ? → -
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.
blocking-basecamp: - → ?
blocking-basecamp: ? → -
tracking-b2g18: --- → +
Is this bug still valid? How can it be reproduced?
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.
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.
Whiteboard: [mentor=jdm][lang=c++]
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).
Mentor: josh
Whiteboard: [mentor=jdm][lang=c++] → [lang=c++]
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
(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.
Attached patch 811470.patch (obsolete) — Splinter Review
Please review the code changes consisting for implementation of nsIDOMGeoPositionErrorCallback interface in ContentParent.cpp.
Attachment #8503296 - Flags: review?(josh)
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?
Attachment #8503296 - Flags: review?(josh)
Attached patch 811470_2.patch (obsolete) — Splinter Review
Attachment #8505691 - Flags: review?(josh)
Comment on attachment 8505691 [details] [diff] [review]
811470_2.patch

Please review the second set of changes based on jdm review

Vinay
Attached patch 811470(latest).patch (obsolete) — Splinter Review
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
Attachment #8505691 - Attachment is obsolete: true
Attachment #8505691 - Flags: review?(josh)
Attachment #8507333 - Flags: review?(josh)
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.
Attachment #8507333 - Flags: review?(josh) → feedback+
Attached patch 811470(modified).patch (obsolete) — Splinter Review
Did all the cleanup based on previous comments
Attachment #8508904 - Flags: review?(josh)
Attached patch 811470(modified).patch (obsolete) — Splinter Review
Attachment #8508904 - Attachment is obsolete: true
Attachment #8508904 - Flags: review?(josh)
Attachment #8508917 - Flags: review?(josh)
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.
Attachment #8508917 - Flags: review?(josh) → feedback+
Oh, and please mark the previous patches obsolete when you attach new versions!
Attached patch 811470.patchSplinter Review
Attachment #8503296 - Attachment is obsolete: true
Attachment #8507333 - Attachment is obsolete: true
Attachment #8508917 - Attachment is obsolete: true
Attachment #8510493 - Flags: review?(josh)
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/
Attachment #8510493 - Flags: review?(josh) → review+
Thank You jdm. This feels really great!. Its my first bug. :)
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.
Assignee: nobody → shettyvinayg
Flags: needinfo?(shettyvinayg)
Attached patch 811470.patch (obsolete) — Splinter Review
I guess it was because of the unwanted if condition in the last line. I have removed that in the current patch.
Attachment #8510493 - Attachment is obsolete: true
Attachment #8511489 - Flags: review?(josh)
Flags: needinfo?(shettyvinayg)
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
Attachment #8511489 - Attachment is obsolete: true
Attachment #8511489 - Flags: review?(josh)
Attachment #8510493 - Attachment is obsolete: false
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
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
Flags: needinfo?(bbajaj)
I would rate the risk as exceedingly low. This is a very simple addition with no moving parts.
Whiteboard: [lang=c++] → [lang=c++][Tako_Blocker]
(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.
Flags: needinfo?(bbajaj)
You need to log in before you can comment on or make changes to this bug.