Specify timeout for outbound network geolocation provider XHR

RESOLVED FIXED in Firefox 42

Status

()

Core
Geolocation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Hanno Schlichting, Assigned: Aidin Gharibnavaz, Mentored)

Tracking

unspecified
mozilla42
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

3 years ago
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.
Relevant code is in http://mxr.mozilla.org/mozilla-central/source/dom/system/NetworkGeolocationProvider.js , in the sendLocationRequest method.
Mentor: josh
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 2

3 years ago
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

3 years ago
Hi assigned it to you.
As for getting an xhr request to stall for testing, I am not sure about.
Assignee: nobody → aidin
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

3 years ago
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
That's what stop_geolocationProvider does - it causes us to pass the stop-responding argument when requesting the geolocation information :)
(Assignee)

Comment 7

3 years ago
Created attachment 8638527 [details] [diff] [review]
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.
Attachment #8638527 - Flags: review?(josh)
Comment on attachment 8638527 [details] [diff] [review]
1177871-1.patch

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

This looks great!
Attachment #8638527 - Flags: review?(josh) → review+
(Assignee)

Comment 9

3 years ago
Thanks (:
I will try to write a test for it, too. In next few days.
(Assignee)

Comment 10

3 years ago
Created attachment 8638978 [details] [diff] [review]
1177871-1.patch

I add the unit test to the patch. Please check it again.
Attachment #8638527 - Attachment is obsolete: true
Attachment #8638978 - Flags: review?(josh)
(Assignee)

Comment 11

3 years ago
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

3 years ago
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

3 years ago
> 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 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.
Attachment #8638978 - Flags: review?(josh) → feedback+
(Assignee)

Comment 15

3 years ago
Created attachment 8641056 [details] [diff] [review]
1177871-1.patch

Done (:
Attachment #8638978 - Attachment is obsolete: true
Attachment #8641056 - Flags: review?(josh)
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Comment 17

3 years ago
Created attachment 8641066 [details] [diff] [review]
1177871-1.patch

Really sorry, I was removed other files in the previous patch. Fixed.
Attachment #8641056 - Attachment is obsolete: true
Attachment #8641056 - Flags: review?(josh)
Attachment #8641066 - Flags: review?(josh)
Comment on attachment 8641066 [details] [diff] [review]
1177871-1.patch

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

Looks great!
Attachment #8641066 - Flags: review?(josh) → review+
(Assignee)

Comment 19

3 years ago
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"!)
Keywords: checkin-needed
Backed out for Windows debug test_geolocation_provider_timeout.js crashes.
https://treeherder.mozilla.org/logviewer.html#?job_id=12365381&repo=mozilla-inbound

Comment 23

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

Comment 24

3 years ago
Created attachment 8642177 [details] [diff] [review]
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.
Attachment #8641066 - Attachment is obsolete: true
Attachment #8642177 - Flags: review?(josh)
Comment on attachment 8642177 [details] [diff] [review]
1177871-1.patch

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

Thanks!
Attachment #8642177 - Flags: review?(josh) → review+
(Assignee)

Comment 26

3 years ago
Thank you (:
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b1e9aeeab2c9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.