965788 - Use shorter TCP connection timeout in AMO add-on compatibility searches

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[:Irving Reid (No longer working on Firefox)]

When we request add-on information from addons.mozilla.org, we don't specify a TCP connection timeout (see https://hg.mozilla.org/mozilla-central/annotate/25c361f6661f/toolkit/mozapps/extensions/AddonRepository.jsm#l1391). This leads to long delays in bug 772484 if we block browser startup to check add-on compatibility, and network connection attempts are going into a black hole.

The default TCP connection timeout is OS dependent and configurable per host, but typically 70-75 seconds. Should we use nsIXMLHttpRequest.timeout to exit sooner when we are not getting a response from AMO? If so, how short - 20 seconds? 30? Controlled by a preference?

Second question - should we apply the timeout to all AMO requests from AddonRepository.jsm, or only those triggered by the compatibility check (thus freezing the UI)?

Comment 1

[:Irving Reid (No longer working on Firefox)]

Attachment: Use XHR timeout for AMO ping and AddonUpdateChecker

Based on IRC and f2f conversations with Dave and Vladan:

1) Only applied the timeout for the blocking call from the add-on update UI
2) Converted AddonUpdateChecker to use XHR timeout instead of a separate timer

Comment 2

[Vladan Djeric (:vladan)]

30 seconds seems overly conservative, it's too long for the UI to be locked up

Comment 3

[Dave Townsend [:mossop]]

Comment on attachment 8373548 [details] [diff] [review]
Use XHR timeout for AMO ping and AddonUpdateChecker

Looks good, Blair should sign off on the timeout lengths though

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8373548 [details] [diff] [review]
Use XHR timeout for AMO ping and AddonUpdateChecker

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

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +98,5 @@
>  
> +// Wrap the XHR factory so that tests can override with a mock
> +let makeXHR = function() {
> +  return Cc["@mozilla.org/xmlextras/xmlhttprequest;1"].
> +           createInstance(Ci.nsIXMLHttpRequest);

Nit: Components.Constructor is the usual way to achieve this.

::: toolkit/mozapps/extensions/content/update.js
@@ +12,5 @@
>  const PREF_XPINSTALL_ENABLED                    = "xpinstall.enabled";
>  const PREF_EM_HOTFIX_ID                         = "extensions.hotfix.id";
>  
> +// timeout (in milliseconds) to wait for response to the AMO metadata ping
> +const AMO_TIMEOUT    = 30000;

Hm, this is tricky. 30sec is long for a UI... but at the same time, we're making it so that this UI should be a very rare occurrence (and when it does show, it'll have better behaviour). In the cases it does need to show, we really do want to give it a really good chance of being able to fetch the data it needs (as the alternative is a worse experience). So lets stick with 30sec for now.

Nit: Avoid explicitly referencing AMO, unless it really is specific only to AMO (almost nothing is). So this is refereed to as a "metadata ping" rather than a "AMO ping".

::: toolkit/mozapps/extensions/test/browser/Makefile.in
@@ +7,5 @@
>  
>  MOCHITEST_BROWSER_MAIN_FILES = \
>    head.js \
>    browser_about.js \
> +  browser_amoTimeout.js \

Ditto.

Comment 5

[:Irving Reid (No longer working on Firefox)]

Attachment: Use XHR timeout for metadata ping and AddonUpdateChecker - v2

Addressed comments...

Do we want telemetry on any of this (either a histogram of metadata ping durations, or a count of the number of ping successes/errors/timeouts)? At this point I can't think of a specific thing we could do with this information, aside from perhaps tuning the timeout, so I'm inclined to not instrument.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8374310 [details] [diff] [review]
Use XHR timeout for metadata ping and AddonUpdateChecker - v2

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

Yea, think the options for telemetry here are a bit too limited to warrant it.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/63621c041e89

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/63621c041e89