1328470 - Reused XMLHttpRequest objects may produce unexpected timeouts

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Christopher Lund]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.87 Safari/537.36

Steps to reproduce:

JavaScript code that (1) reused one XMLHttpRequest object for multiple requests, (2) set a timeout for each request, and (3) had a delay between setting the timeout and sending always resulted in a timeout for the second request without ever making the request (as seen in the Network tab of the web developer tools).

The scenario has been simplified to the following self-contained script which first GETs mozilla.org immediately, then repeats the GET after a 300 ms delay.

  function getMozilla() {
    xhr.open('GET', 'http://mozilla.org');
    xhr.timeout = 2000;
    setTimeout(function () {
      start = Date.now();
      console.log('sending');
      xhr.send();
    }, 1000);
  }

  start = 0;
  xhr = new XMLHttpRequest();
  xhr.onload = function () { console.log('onload after ' + (Date.now() - start) + ' ms'); }
  xhr.ontimeout = function () { console.log('ontimeout after ' + (Date.now() - start) + ' ms'); }
  getMozilla();
  setTimeout(getMozilla, 3500);

The steps to reproduce with this script are:

1. Open a new tab in Firefox.
2. Open the developer tools and switch to the JavaScript console.
3. Paste the script into the console and hit Enter. (If the new tab is on an existing web page, update the URL to match the schema and domain.)
4. Observe the console output.


Actual results:

The output of the script from the steps to reproduce was:

sending
onload after 70
sending
ontimeout after 2004

The numbers vary, of course, but the second, delayed GET always reports a timeout after the timeout period. If the delay (3500 ms in the sample script) is smaller than the request timeout (2000 ms), then the timeout does not occur with two "onloads" reported. Likewise, if the send immediately follows the setting of the timeout value (rather than being delayed itself), the timeout does not occur.


Expected results:

In all cases, both requests should have been made and responses returned, barring, of course, an actual timeout due to a slow network.

Comment 1

[Loic]

Attachment: 1328470.html

Comment 2

[Loic]

Attachment: 1328470.html

Comment 3

[Loic]

Attachment: 1328470.html

Comment 4

[Alex Vincent [:WeirdAl]]

https://xhr.spec.whatwg.org/#the-send()-method

I'm pretty sure this bug is invalid.  Rereading the send() method in the spec, I see nothing in there about resetting the timeout when send is invoked twice.  (As opposed to open().)

Comment 5

[Alex Vincent [:WeirdAl]]

Oh, I missed that xhr.open line.  My apologies.  It would be interesting to know when that call happens, via a log statement.

Comment 6

[Christopher Lund]

At the risk of looking in the wrong direction...

I forgot to mention that I looked at the source (as far as I can tell) that when the timeout is set, if we have a request send time, the timeout timer is started. The code I am looking at is below. The mRequestSentTime is set on each send() call but is not changed otherwise. Clearing it (on closing the request? on the subsequent open()?) seems like it might address the issue.


  void
  XMLHttpRequestMainThread::SetTimeout(uint32_t aTimeout, ErrorResult& aRv)
  {
    /* unrelated code removed */
    mTimeoutMilliseconds = aTimeout;
    if (mRequestSentTime) {
      StartTimeoutTimer();
    }
  }

Comment 7

[Andrew Overholt [:overholt]]

baku, is this something we need to fix?

Comment 8

[Andrea Marchesini [:baku]]

Christopher you are right. Do you want to take this bug?
I would reset the mRequestSentTime in XMLHttpRequestMainThread::OnStopRequest

Comment 9

[Andrea Marchesini [:baku]]

Attachment: xhr_timeout.patch

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8835940 [details] [diff] [review]
xhr_timeout.patch

We need a test for this, hopefully in wpt

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a88ccbdbeb9
XMLHttpRequest should reset timers when send() is executed again, r=smaug

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/6a88ccbdbeb9

Comment 13

[Boris Zbarsky [:bzbarsky]]

Did you manually add this to the WPT manifest or something?  You didn't add the hash bits....

Comment 14

[Andrea Marchesini [:baku]]

Oh, right. Thanks for adding it.