1177871 - Specify timeout for outbound network geolocation provider XHR

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Hanno Schlichting]

The network location provider currently does an outbound XHR call without specifying a timeout or ontimeout handler. In weird network situations this can lead to the request stalling pretty much forever without returning anything back to the caller.

We recently added a similar XHR request to determine the users country. There we added a simple timeout and telemetry to observe how long it takes our users to call out to the external web service. The 95th percentile here for desktop release users is 14 seconds.

To keep it simple, I'd add a xhr.timeout = 60 and specify an ontimeout handler for the xhr. The ontimeout can be the same as the onerror, simply returning back a position unavailable.

Comment 1

[Josh Matthews [:jdm]]

Relevant code is in http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js , in the sendLocationRequest method.

Comment 2

[Aidin Gharibnavaz]

Can I pick this bug? If it's OK, please assign it to me.

I need a little help about how to test the behaviour.
Thanks.

Comment 3

[:garvan]

Hi assigned it to you.
As for getting an xhr request to stall for testing, I am not sure about.

Comment 4

[Josh Matthews [:jdm]]

If we make the timeout configurable (using a preference), we can write a test that calls stop_geolocationProvider (http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/geolocation_common.js#33) and performs a geolocation operation (using an XHR timeout of 3s or something), then watch for an error to be reported.

Comment 5

[:garvan]

In case comment 4 doesn't trigger a hang (and errors out immediately),
perhaps having the handleRequest() function sit idle for a time longer than the timeout is an acceptable simulation of the error:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/network_geolocation.sjs#35

Comment 6

[Josh Matthews [:jdm]]

That's what stop_geolocationProvider does - it causes us to pass the stop-responding argument when requesting the geolocation information :)

Comment 7

[Aidin Gharibnavaz]

Attachment: 1177871-1.patch

I test it manually by setting time out to a very low value. Works.

Please review this change. If it was OK, I will try to write a test for it.

Thanks.

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 8638527 [details] [diff] [review]
1177871-1.patch

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

This looks great!

Comment 9

[Aidin Gharibnavaz]

Thanks (:
I will try to write a test for it, too. In next few days.

Comment 10

[Aidin Gharibnavaz]

Attachment: 1177871-1.patch

I add the unit test to the patch. Please check it again.

Comment 11

[Aidin Gharibnavaz]

Seems that there's a memory leak in geolocation provider. Seems that it have nothing to do with my changes, but I need approve. So I report it here:

When I run my unit test, I see this line in the output:

0:21.30 PROCESS_OUTPUT: Thread-1 (pid:3613) " => mFreeCount:            4589  --  LEAKED 1 !!!"

When I comment out my changes in "dom/system/NetworkGeolocationProvider.js", leaks become two!

0:43.35 PROCESS_OUTPUT: Thread-1 (pid:2913) " => mFreeCount:            4735  --  LEAKED 2 !!!"

Only if I change the code in a way that XHR fails (for example, set a wrong url), there will be no leak.

Here's the full output of the test:
https://pastebin.mozilla.org/8840691
And full output without my changes applied:
https://pastebin.mozilla.org/8840690

I couldn't figure out what's the problem. Can you check it please?

Comment 12

[:garvan]

Nice work!

Instead of creating an HttpServer manually, as comment 4 suggested, you can use the the *_geolocationProvider() utility functions:

resume_geolocationProvider(() => {force_prompt(true, your_test_func)});
// the above starts the http server for geo testing

function your_test_func() {
   // setup short timeout in pref as you are doing
   // now, stop http sever responding, and make sure error is called, not success 
   stop_geolocationProvider(() => {navigator.geolocation.getCurrentPosition(successIsBad, errorIsGood)});
}

To explain: mochitest has a built-in HttpServer mechanism, whereby one creates an .sjs file with a response handler. This particular test directory for geo has an .sjs file, and it has a handler that will simulate a timeout, by not responding (triggered with stop_geolocationProvider()).

Comment 13

[:garvan]

> I couldn't figure out what's the problem. Can you check it please?

Do you get leaks reported if you run mochitest on dom/tests/mochitest/geolocation ?
I don't get any leaks reported when I just checked.

Comment 14

[Josh Matthews [:jdm]]

Comment on attachment 8638978 [details] [diff] [review]
1177871-1.patch

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

This is great work Aidin! It's not really clear to me whether it's better to write a mochitest instead of an xpcshell test here, so I'm fine with this test if we make the following small changes to it.

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

success and watchId are unused, so they can be removed.

@@ +27,5 @@
> +  response.setHeader("Content-Type", "aplication/x-javascript", false);
> +  do_timeout(5000, function() {
> +    response.write(position);
> +    response.finish();
> +  });

Instead of having an actual response, let's just call response.processAsync() and return immediately.

Comment 15

[Aidin Gharibnavaz]

Attachment: 1177871-1.patch

Done (:

Comment 16

[Aidin Gharibnavaz]

garvank: I ran those mochitests, but didn't saw any memory leaks. To ensure, I also rewrite the XHR Timeout test as a Mochitest, but no leaks either.

Seems there's a problem with the test itself.

Comment 17

[Aidin Gharibnavaz]

Attachment: 1177871-1.patch

Really sorry, I was removed other files in the previous patch. Fixed.

Comment 18

[Josh Matthews [:jdm]]

Comment on attachment 8641066 [details] [diff] [review]
1177871-1.patch

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

Looks great!

Comment 19

[Aidin Gharibnavaz]

I test the patch on Try Server too, with all tests I though maybe related:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=25052baa7a6f

(Though it stuck at "gecko-decision opt"!)

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8010e0cbd228

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows debug test_geolocation_provider_timeout.js crashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=12365381&repo=mozilla-inbound

Comment 22

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefa8e4699d2

Comment 23

[:garvan]

I think the test just needs a:
 prefs.setBoolPref("geo.wifi.scan", false);

Comment 24

[Aidin Gharibnavaz]

Attachment: 1177871-1.patch

Really sorry. I should include Debug in my Try Server request. :\ Lesson learned.

Thanks Garvan for the tip. It fixed. Here's the Treeherder task result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5027071dacd2

The failed test is something else. I checked the output log, and my test was sucessfull.

Comment 25

[Josh Matthews [:jdm]]

Comment on attachment 8642177 [details] [diff] [review]
1177871-1.patch

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

Thanks!

Comment 26

[Aidin Gharibnavaz]

Thank you (:

Comment 27

[Pulsebot]

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

Comment 28

[Carsten Book [:Tomcat]]

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