Closed Bug 965788 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Add-ons Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: Irving, Assigned: Irving)

References

Details

Attachments

(1 file, 1 obsolete file)

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)?
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
Assignee: nobody → irving
Status: NEW → ASSIGNED
Attachment #8373548 - Flags: review?(dtownsend+bugmail)
30 seconds seems overly conservative, it's too long for the UI to be locked up
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
Attachment #8373548 - Flags: review?(dtownsend+bugmail)
Attachment #8373548 - Flags: review?(bmcbride)
Attachment #8373548 - Flags: review+
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.
Attachment #8373548 - Flags: review?(bmcbride) → review+
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.
Attachment #8373548 - Attachment is obsolete: true
Attachment #8374310 - Flags: review?(bmcbride)
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.
Attachment #8374310 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/integration/fx-team/rev/63621c041e89
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/63621c041e89
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.