1127627 - ReaderMode.jsm should have an xhr timeout

Status: RESOLVED INVALID

(Toolkit :: Reader Mode, defect, P5)

Description

[Mark Hammond [:markh] [:mhammond]]

I was just scanning this to get up to speed on all things RL, and noticed the XHR in ReaderMode.jsm doesn't have a timeout.

The reason I think this could be a problem is that nsSearchService.jsm also uses an XHR for geoip, and as a fallback implements a 100 second timeout on the XHR - and it records telemetry.  The telemetry dashboard shows that for Aurora 37, 2% of users hit this timeout - and I suspect for those users the request would never actually complete - it typically takes 2-3 seconds.

Maybe that's not a problem in this context (in which case just close WONTFIX) but thought it worth bringing this up.

Comment 1

[:Margaret Leibovic]

I think this issue is less important in the reader mode code, because this XHR isn't blocking anything important (e.g. starting up the search service). Additionally, in some cases, if we just bail on this XHR, the user will have no article to read, so the UI would just fail.

However, it would be good to take a closer look at this and see if in some cases it does make sense to have a timeout, such as when we're trying to download multiple articles to cache in the background (bug 1113454).

Adding telemetry sounds like a really good idea.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

(In reply to :Margaret Leibovic from comment #1)
> I think this issue is less important in the reader mode code, because this
> XHR isn't blocking anything important (e.g. starting up the search service).

FWIW, the XHR timeout in the search service is *not* to avoid blocking startup (there's a timer for that) - it exists only to avoid resource leakage in the off-chance it *never* completes/errors.  The telemetry probes seem to indicate it is doing just that.

> Additionally, in some cases, if we just bail on this XHR, the user will have
> no article to read, so the UI would just fail.

The XHR never completing is just as poor from a UX POV but also might have memory/resource usage implications.  I imagine that after (say) 10 (100?) minutes we could assume the user has already given up on the UI responding.

Comment 3

[:Gijs (he/him)]

(In reply to Mark Hammond [:markh] from comment #2)
> (In reply to :Margaret Leibovic from comment #1)
> > I think this issue is less important in the reader mode code, because this
> > XHR isn't blocking anything important (e.g. starting up the search service).
> 
> FWIW, the XHR timeout in the search service is *not* to avoid blocking
> startup (there's a timer for that) - it exists only to avoid resource
> leakage in the off-chance it *never* completes/errors.  The telemetry probes
> seem to indicate it is doing just that.
> 
> > Additionally, in some cases, if we just bail on this XHR, the user will have
> > no article to read, so the UI would just fail.
> 
> The XHR never completing is just as poor from a UX POV but also might have
> memory/resource usage implications.  I imagine that after (say) 10 (100?)
> minutes we could assume the user has already given up on the UI responding.

I'm fairly sure there are network code limits on how long we'll keep the XHR alive, and they're in the region of 5 minutes.

We had (gone in current nightly, v50) telemetry for the length of time we take to do something here:

https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-07-06&keys=__none__!__none__!__none__&max_channel_version=beta%252F48&measure=READER_MODE_DOWNLOAD_MS&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-06-06&table=0&trim=1&use_submission_date=0

this indicates that 99.5% of cases where we have to manually download the DOM complete in 40 seconds or fewer on desktop. It's 99.12% on mobile, and more importantly the distribution is not nearly as steep, which makes sense because there's a much higher chance the connection is rubbish.

I'm not opposed to adding a hard cut-off of 1 minute or something along those lines, but I don't think there's an urgent need to do this - the memory/resource implications will be very small, I think, and in any case I'd expect most users to give up before that (and/or the ones who know their network connection is / might be bad to be annoyed if we break the connection "too quickly").

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Yeah, let's not bother keeping this open then.