684722 - NetworkGeolocationProvider should cause error handlers to fire if the xhr fails

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Josh Matthews [:jdm]]

Right now if an error occurs, we might log it, but nothing else happens. Does it make sense to report this as an error to consumers? If they're not using a timeout, they'll just hang around indefinitely.

Comment 1

[Doug Turner (:dougt)]

yes, this is a lot easier now since, the general case, we only have one geolocation provider (the network one or whatever the OS provides). As of a few versions ago, we may have had multiple providers (the network one AND the OS one) and supporting error handling was more trouble than worth.

Comment 2

[Josh Matthews [:jdm]]

This is even easier now that we've eliminated multiple geolocation providers.

Comment 3

[(not currently active) Ted Mielczarek]

Probably related: Google doesn't like me, so the geolocation API returns:
{"status" : "ZERO_RESULTS"}, but we never call the error callback in that case either.

Comment 4

[Josh Matthews [:jdm]]

Now that bug 874996, this is totally possible.

Comment 5

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 6

[mjh563]

Another thing that should be reported as an error is the XHR open command failing. That can happen if the geo.wifi.uri pref is set to a string that's not a valid URL.

This line currently throws an uncaught NS_ERROR_MALFORMED_URI exception if the URL is invalid:

xhr.open("POST", url, true);

Comment 7

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 8

[Bharath Thiruveedula[:tbharath]]

According to the #comment6, I have added support for handling NS_ERROR_MALFORMED_URI in the latest patch.

Comment 9

[Josh Matthews [:jdm]]

Comment on attachment 813104 [details] [diff] [review]
Bug684722.patch

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

This looks great! I have a couple requests now:
1) please make the commit message more specific (Invoke geolocation error handlers when network request fails.), and add r=jdm to the commit message (which means I am the reviewer).
2) let's add an automated test for this. We can build on the tests in http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/ and make a fake network handler that returns a non-200 HTTP status (http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/network_geolocation.sjs?force=1 is the code that runs in our http server). See https://developer.mozilla.org/en-US/docs/Mochitest for more information about these tests.

::: dom/system/NetworkGeolocationProvider.js
@@ +224,5 @@
>      }
>    },
>  };
>  
> +function displayError() {

Call this triggerError instead.

@@ +225,5 @@
>    },
>  };
>  
> +function displayError() {
> +    Cc["@mozilla.org/geolocation/service;1"].getService(Ci.nsIGeolocationUpdate)	

Remove the extra whitespace on this line.

Comment 10

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Added test to the bug and also modified the patch based on comment9

Comment 11

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 12

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Solved Bug 924097, based on the jdm comment

Comment 13

[Josh Matthews [:jdm]]

Comment on attachment 814072 [details] [diff] [review]
Bug684722.patch

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

This is looking good; just a few more changes required :)

::: dom/src/geolocation/nsGeolocation.cpp
@@ +371,5 @@
> +  NotifyErrorAndShutdown(nsIDOMGeoPositionError::TIMEOUT);
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP

Since this method only returns NS_OK, we can just make it return void instead.

@@ +1168,5 @@
>    }
>  
>    for (uint32_t i = mPendingCallbacks.Length(); i > 0; i--) {
> +    mPendingCallbacks[i-1]->NotifyErrorAndShutdown(aErrorCode);
> +    //RemoveRequest(mPendingCallbacks[i-1]);

Why is this commented out?

::: dom/tests/mochitest/geolocation/geolocation_common.js
@@ +52,5 @@
>  }
>  
> +function send404_geolocationProvider(callback)
> +{
> +  SpecialPowers.pushPrefEnv({"set": [["geo.wifi.uri", "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs?action=send404"]]}, function() {

I think this can just pass callback instead of using this anonymous function here.

::: dom/tests/mochitest/geolocation/network_geolocation.sjs
@@ +63,5 @@
>    var delay = 0;
>    if ('delay' in params) {
>      delay = params.delay;
>    }
> +  if(params.action === "send404") {

Space before (.

@@ +64,5 @@
>    if ('delay' in params) {
>      delay = params.delay;
>    }
> +  if(params.action === "send404") {
> +    response.setStatusLine("1.0", 404, "Not Found");

Add a response.finish() here, and then return.

::: dom/tests/mochitest/geolocation/test_errorcheck.html
@@ +33,5 @@
> +    if(error.code == 3) {
> +      ok(0, "Time out error occured");
> +    } else {
> +        is(error.code, 2, "Geolocation error handler fired");
> +    }

Just replace this if/else with `is(error.code, SpecialPowers.Ci.nsIDOMGeoPositionError.POSITION_UNAVAILABLE, "Geolocation error handler fired")`, since that is what we want to test.

@@ +43,5 @@
> +    SimpleTest.finish();
> +}
> +
> +function test2() {
> +  navigator.geolocation.getCurrentPosition(successCallback, errorCallback, {timeout: 1000});

Timeouts like this can cause intermittent failures if the network request doesn't complete soon enough (for example, if our test machines hang for a bit of time). Just remove the timeout, since the test harness will mark it as a failure if the test doesn't succeed within 60 seconds.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 814116 [details] [diff] [review]
Bug684722.patch

Clearing the review flag until my comments from the previous review are addressed.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 814072 [details] [diff] [review]
Bug684722.patch

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

::: dom/src/geolocation/nsGeolocation.cpp
@@ +1168,5 @@
>    }
>  
>    for (uint32_t i = mPendingCallbacks.Length(); i > 0; i--) {
> +    mPendingCallbacks[i-1]->NotifyErrorAndShutdown(aErrorCode);
> +    //RemoveRequest(mPendingCallbacks[i-1]);

I understand your explanation. Please remove this code, and add a comment like "NotifyErrorAndShutdown removes the request from the array."

Comment 16

[mjh563]

Comment on attachment 814116 [details] [diff] [review]
Bug684722.patch

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

::: dom/system/NetworkGeolocationProvider.js
@@ +178,1 @@
>      xhr.setRequestHeader("Content-Type", "application/json; charset=UTF-8");

This line will also throw an exception if the url is invalid, because the catch block falls through.

You should probably either return after calling triggerError in the catch, or put the entire XHR code block from xhr.open to xhr.send inside the try{}.

Comment 17

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Modified the code based on comment14, comment15 and comment16

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 814125 [details] [diff] [review]
Bug684722.patch

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

Great! Attach a version that builds and passes the test locally and it should be ready to go :)

Comment 19

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 814345 [details] [diff] [review]
Bug684722.patch

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

Thank you for doing this work!

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0263aa798812

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

Comment 23

[Josh Matthews [:jdm]]

Joel, do you know who understands the interaction on the Android 4.0 devices between httpd.js and tests that use it? The test added in this patch timed out for reasons I cannot explain. Maybe the very short 404 response got buffered and never sent?

Comment 24

[Joel Maher ( :jmaher ) (UTC -8)]

I have looked at adb logcat on my local panda board, did some initial debugging.  Even hacking up the test case failed for me.  I could spend more time on this.  Does this work just fine on desktop?  Android 2.2?

httpd.js is the same for android 2.2 and android 4.0- one difference is android 4.0 is run on mozharness while android 2.2 is not.

Comment 25

[mjh563]

Looking at how the 200 and 404 responses are handled in network_geolocation.sjs, I see these differences:

1. the 404 response doesn't write a response body, it only sets headers
2. when returning a 404, response.finish() is called immediately, but when returning a 200 it's called in a timer callback
3. when returning a 404, the response status is actually set twice, first to 200 "OK" and then to 404 "Not Found"

We know that the 200 response works, because it's used in the existing tests. So I wonder if one of those differences is causing the 404 response to not get sent? 

For reference, here's the relevant code:

  var position = ...
  
  var response;
  response.processAsync();
  response.setStatusLine("1.0", 200, "OK");
  response.setHeader("Cache-Control", "no-cache", false);
  response.setHeader("Content-Type", "aplication/x-javascript", false);

  var delay = 0;
  if ('delay' in params) {
    delay = params.delay;
  }
  if (params.action === "send404") {
    response.setStatusLine("1.0", 404, "Not Found");
    response.finish();
    return;
  }
  timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
  timer.initWithCallback(function() {
    response.write(position);
    response.finish();
  }, delay, timer.TYPE_ONE_SHOT);
}

Comment 26

[Josh Matthews [:jdm]]

That's a good point. Bharath, could you try removing the response.finish call and the early return, and just set position to ''? That will let us take the same code path as the 200 response, which might work better. If you attach a new patch, I'll send it to our tryserver to see if it works on Android devices.

Comment 27

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Uploaded new patch according to the comments of jdm.

Comment 28

[Joel Maher ( :jmaher ) (UTC -8)]

oh, we might be fighting an uphill battle here, right now the geolocation tests are skipped on android due to timeouts:
http://dxr.mozilla.org/mozilla-central/source/testing/mochitest/android.json#l182

Comment 29

[Josh Matthews [:jdm]]

Odd; the log in comment 22 shows the tests running. How are the contents of android.json actually used?

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

yes, this is confusing.  There are a couple cases which are not listed in the android.json:
* test_clearWatch_invalid
* test_cancelWatch
* test_cancelCurrent

These three test cases expect an error and no geolocation response.  

We use android.json on the commandline as a manifest: --run-only-tests=android.json.  If you look at android.json in more detail, you will see we have a list of tests that are in the 'excludetests' section.  It might be best to put this new test case in the excludetests.

Comment 31

[Josh Matthews [:jdm]]

Comment on attachment 818406 [details] [diff] [review]
Bug684722.patch

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

Bharath, could you fix these problems and add this test to android.json?

::: dom/tests/mochitest/geolocation/network_geolocation.sjs
@@ +41,4 @@
>        return;
>    }
>  
> +  var position = '';

This will break all existing geolocation tests.

@@ +67,5 @@
>    }
> +  if (params.action === "send404") {
> +    response.setStatusLine("1.0", 404, "Not Found");
> +	//response.finish();
> +    //return;

Please remove the commented out code and clear the position variable in this block.

Comment 32

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 33

[Josh Matthews [:jdm]]

Comment on attachment 818990 [details] [diff] [review]
Bug684722.patch

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

Looks good; thanks! I'll push this to the tryserver before trying to commit it to the tree.

Comment 34

[Josh Matthews [:jdm]]

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

Comment 35

[Josh Matthews [:jdm]]

I pushed the wrong version: https://tbpl.mozilla.org/?tree=Try&rev=7ae57098ddb4

Comment 36

[mjh563]

I can reproduce the test failure in my local build. I think you need to add a delay to send404_geolocationProvider, like is done in the start_sending_garbage and stop_sending_garbage functions.

If I've understood it correctly, the problem is that when the test starts, there may be existing XHR requests (sent during the previous test) that haven't yet returned a response. These will return a success response, because they're from before the geo.wifi.uri was changed to make the server return a 404.

So after the getCurrentPosition request is set up, the provider will receive one of these success responses before it receives the first 404 response.

Adding a delay will allow time for the existing responses to be cleared.

In my (very slow) debug build though, a 1000ms delay wasn't long enough. I had to increase it to 3000ms before the test would pass.

Comment 37

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 38

[Josh Matthews [:jdm]]

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

It's good!

Comment 39

[Josh Matthews [:jdm]]

Comment on attachment 824465 [details] [diff] [review]
Bug684722.patch

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

::: dom/tests/mochitest/geolocation/geolocation_common.js
@@ +53,5 @@
>  
> +function send404_geolocationProvider(callback)
> +{
> +  SpecialPowers.pushPrefEnv({"set": [["geo.wifi.uri", "http://mochi.test:8888/tests/dom/tests/mochitest/geolocation/network_geolocation.sjs?action=send404"]]}, function() {
> +    sleep(5000);

The more I look at this, the more I don't like this solution. I think the problem we're facing is the repeating timer at http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js#94 . Bharath, does this test pass for you if you add |SpecialPowers.Cc["@mozilla.org/geolocation/provider;1"].getService(SpecialPowers.Ci.nsIGeolocationProvider).shutdown();| before this line, then change this to only sleep for 1000?

Comment 40

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 41

[Josh Matthews [:jdm]]

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

Comment 42

[Josh Matthews [:jdm]]

Comment on attachment 825164 [details] [diff] [review]
Bug684722.patch

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

Excellent!

Comment 43

[Ryan VanderMeulen [:RyanVM]]

So this was backed out for Android failures and none of the Try runs ran on Android? Is there another link I should be looking at before attempting to push this again?

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Oh nevermind, you're skipping it on Android :)

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b54f084f8e39

Comment 46

[Ryan VanderMeulen [:RyanVM]]

However, you should have also disabled this on Android x86 and B2G.
https://hg.mozilla.org/integration/b2g-inbound/rev/d83f69660b5a

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b54f084f8e39
https://hg.mozilla.org/mozilla-central/rev/d83f69660b5a

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Sorry, the timeouts on OSX are happening too frequently for me to leave this in the tree. Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/753a91f79d6f

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Do make sure that your next patch has the changes from comment 45 as well.

Comment 50

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 51

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 52

[Josh Matthews [:jdm]]

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

Comment 53

[Bharath Thiruveedula[:tbharath]]

Attachment: Bug684722.patch

Comment 54

[Josh Matthews [:jdm]]

Seventeen OS X retriggers show green.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

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

Comment 56

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

https://hg.mozilla.org/mozilla-central/rev/a4b6ff90623b