Last Comment Bug 525816 - (xhr-timeout) XMLHttpRequest should allow to specify a network timeout in ms (for async requests)
(xhr-timeout)
: XMLHttpRequest should allow to specify a network timeout in ms (for async req...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Other Branch
: All All
: -- enhancement with 1 vote (vote)
: mozilla12
Assigned To: Alex Vincent [:WeirdAl]
:
Mentors:
http://dev.w3.org/2006/webapi/XMLHttp...
Depends on: 703380 749098
Blocks: 722076 498998
  Show dependency treegraph
 
Reported: 2009-11-01 22:39 PST by Ben Bucksch (:BenB)
Modified: 2013-05-05 06:24 PDT (History)
19 users (show)
dao+bmo: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
work in progress, completely untested (21.01 KB, patch)
2011-08-26 17:27 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
work in progress, mostly untested (24.14 KB, patch)
2011-10-12 17:39 PDT, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
work in progress, mostly untested (25.32 KB, patch)
2011-10-30 03:24 PDT, Alex Vincent [:WeirdAl]
bugs: feedback+
Details | Diff | Review
work in progress, test 1/2 done (20.72 KB, patch)
2011-11-16 22:00 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
Implement timeout on XMLHttpRequest (not counting workers) (35.03 KB, patch)
2011-12-03 01:44 PST, Alex Vincent [:WeirdAl]
bugs: review-
Details | Diff | Review
Implement timeout on XMLHttpRequest (not counting workers) (24.66 KB, patch)
2011-12-08 01:33 PST, Alex Vincent [:WeirdAl]
bugs: review+
Details | Diff | Review
revised patch per review comments (36.22 KB, patch)
2011-12-17 15:44 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review
IRC conversation on worker XHR timeouts (11.55 KB, text/plain)
2011-12-17 19:14 PST, Alex Vincent [:WeirdAl]
no flags Details
updated to tip (27.65 KB, patch)
2012-01-05 18:56 PST, Alex Vincent [:WeirdAl]
bugs: review+
jonas: superreview+
Details | Diff | Review
final patch for check-in (27.83 KB, patch)
2012-01-25 14:58 PST, Alex Vincent [:WeirdAl]
no flags Details | Diff | Review

Description Ben Bucksch (:BenB) 2009-11-01 22:39:24 PST
Make the HTTP timeout implemented in bug 407190 accessible to web pages.

Quite often, when you make an XMLHttpRequest request, you expect an answer (from your own server) quickly, where "quickly" means usually < 1s, but at most 10s. It may be something you need in the middle of the processing or display, e.g. a list of all countries.
The Mozilla default for timeouts is 2 minutes, which essentially is a hang, if an app were to be waiting that long. The app should be able to say "it should respond in 300ms, so if it's not completed after 10s, there's something seriously wrong and it probably never will be completed". In other words:

There should be a writable XMLHttprequest attribute .timeout (in ms).
If that time is reached, the onerror handler triggers. (I consider that an error, because it usually is a network error of some sorts, on client or server side or in DNS.)
Comment 1 Ben Bucksch (:BenB) 2009-11-01 22:40:09 PST
We should ping the W3C about that, too, to standardize it, if possible.
Comment 2 Olli Pettay [:smaug] 2009-11-02 01:31:39 PST
(In reply to comment #0)
> Quite often, when you make an XMLHttpRequest request, you expect an answer
> (from your own server) quickly, where "quickly" means usually < 1s, but at most
> 10s. It may be something you need in the middle of the processing or display,
> e.g. a list of all countries.
This sounds like you want timeout for synchronous XMLHttpRequest.
I think I wouldn't want to add any new features which makes it easier to
use sync XHR.

For async XHR one can use JS timeouts.
Something like:
var xhr = new XMLHttpRequest();
xhr.timeout = setTimeout("xhr.abort()", 300);
xhr.onload = function() { clearTimeout(this.timeout); /* Do something... */ }
xhr.onabort = function() { /* Do something else ... */}

But sure, if there is some good use case for async XHR + timeouts, and that use
case is not trivial to implement using JS timeouts, then something could be added to XHR.
Comment 3 Jonas Sicking (:sicking) 2009-11-02 01:56:33 PST
Microsoft added a .timeout property to their XDomainRequest object. It's been debated if it should be added to XMLHttpRequest Level 2. We might get to talk about it this week at TPAC.

I agree that this is mostly needed for synchronous XHR, and that we want synchronous XHR to go away. However I'm not sure that not having a timeout feature is actually going to cause people use synchronous XHR less. If it doesn't, then the result is an even worse experience for our users.

Also, a timeout property is a nice convenience feature even for asynchronous XHR.

However if we want to wait and see what XHR level 2 does, I'm fine with that.
Comment 4 Jonas Sicking (:sicking) 2009-11-02 01:58:41 PST
Oh, but in any event I would not recommend the example code from comment 2, as it would interact poorly if .timeout ever is specced in the future (be that in level 2 or some other future version)
Comment 5 Ben Bucksch (:BenB) 2009-11-02 03:12:37 PST
No, this has nothing to do with sync. I never use sync XMLHttpRequest!

Most of the time, however, the user inherently needs to wait for the data or response anyways, even if the UI does not block.

> For async XHR one can use JS timeouts.

I know (and wanted to add this to the initial description, but forgot).

But it's a hack (and considerably more code), for something that IMHO almost every request needs. Nobody will wait 2 minutes, for the result of a click to display.

> a timeout property is a nice convenience feature even for asynchronous XHR.

If you want to see it that way, fine.

Timeouts are quite frequent in practice. Esp. in case of DNS problems, servers being down etc..

> Microsoft added a .timeout property to their XDomainRequest object. It's
> been debated if it should be added to XMLHttpRequest Level 2. We might
> get to talk about it this week at TPAC.

Cool, thanks!
Comment 6 Olli Pettay [:smaug] 2009-11-02 03:37:43 PST
(In reply to comment #5)
> But it's a hack (and considerably more code),
'considerably'? Really?
And it is not a hack, IMHO.

But this discussion should happen in webapps wg mailing list.
Comment 7 William J. Edney 2010-03-03 19:54:01 PST
For synchronous XHR, this bug has also been filed:

https://bugzilla.mozilla.org/show_bug.cgi?id=498998

Also, re: https://bugzilla.mozilla.org/show_bug.cgi?id=525816#c3 here, this property has been added to IE8's 'standard XHR' object, not just their proprietary XDR object.
Comment 8 Alex Vincent [:WeirdAl] 2010-03-31 13:49:37 PDT
Peanut gallery comment:  This is the #1 thing that bothers me about XHR's - synchronous or asynchronous.  I'd be willing to go ahead and implement a custom mozTimeout IDL attribute on a new XHR interface, because the spec could easily take a couple more years to go through the standardization process.  This is something that affects web developers now.

With the moz prefix, that gives us the chance to deploy one version early and leave room for a standardized property name later.  Then, when the time comes to implement the new method, we can mark the Mozilla-specific one as DEPRECATED, and remove it in a future version.
Comment 9 Jonas Sicking (:sicking) 2010-03-31 16:38:22 PDT
Please do write an implementation. I would be in approval of that. I'd even simply call it .timeout since that is what it's called in IE.
Comment 10 Anne (:annevk) 2010-03-31 20:47:32 PDT
It has been part of XMLHttpRequest Level 2 for a while now by the way...
Comment 11 Alex Vincent [:WeirdAl] 2010-04-01 07:56:45 PDT
Anne: I notice no one's actually posted the URL to the part of the XHR2 spec defining .timeout.  It's not in http://www.w3.org/TR/XMLHttpRequest2/ .  :)
Comment 12 Jonas Sicking (:sicking) 2010-04-01 08:15:35 PDT
Never look at the latest "published" draft. Always look at the latest editor draft. Se link at the top of the spec.

http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-timeout-attribute
Comment 13 Olli Pettay [:smaug] 2011-03-14 11:20:09 PDT
We could implement this for Firefox 5.
Comment 14 Olli Pettay [:smaug] 2011-03-15 06:55:06 PDT
...if some spec issues are resolved soon.
Comment 15 Alex Vincent [:WeirdAl] 2011-08-18 16:15:57 PDT
(In reply to Olli Pettay [:smaug] from comment #14)
> ...if some spec issues are resolved soon.

I'm still interested in implementing.  What spec issues are you referring to?

One obvious concern is the user changing the timeout length after the request has been sent...
Comment 16 Alex Vincent [:WeirdAl] 2011-08-26 13:44:42 PDT
<smaug> WeirdAl: the spec has been fixed
Comment 17 Alex Vincent [:WeirdAl] 2011-08-26 17:27:10 PDT
Created attachment 556189 [details] [diff] [review]
work in progress, completely untested

This is a rough guess at how to implement the timeout.  I make no warranties other than that it compiles and links with no added warnings or errors.

I'm wondering how we're going to test this.  I haven't the foggiest idea what changes we'd have to make to netwerk/test/httpserver/httpd.js to add delays to HTTP responses...
Comment 18 Ben Bucksch (:BenB) 2011-08-27 00:27:37 PDT
WeirdAl, the first line of this bug states:

> Make the HTTP timeout implemented in bug 407190 accessible to web pages.

We already have a timeout on the socket implementation. Can you just set that, instead of using a timer to kill the request?
Comment 19 Alex Vincent [:WeirdAl] 2011-09-01 16:57:29 PDT
(In reply to Ben Bucksch (:BenB) from comment #18)
> We already have a timeout on the socket implementation. Can you just set
> that, instead of using a timer to kill the request?

I'm thinking about the case of a HTTP 302 response, or some other redirect.  Doesn't that close one socket and open another?

If so, I think it'd be more difficult to manage the timeouts through the sockets than at the XHR level.
Comment 20 Alex Vincent [:WeirdAl] 2011-09-14 10:19:42 PDT
Note to self:  Apparently httpd.js supports scripting of HTTP responses:
https://developer.mozilla.org/en/HTTP_server_for_unit_tests#SJS:_server-side_scripts
Comment 21 Olli Pettay [:smaug] 2011-09-14 10:20:33 PDT
What if the XHR's channel doesn't use socket for anything? I think we need a timer in XHR.

Or Ben, which timeout are you talking about?
Comment 22 Ben Bucksch (:BenB) 2011-09-14 17:17:56 PDT
As said, I was talking about bug 407190:

> Make the HTTP timeout implemented in bug 407190 accessible to web pages.

This is what this bug is about. For me, a timer is a hack that can use used as fallback, but shouldn't be the primary means.
Comment 23 Olli Pettay [:smaug] 2011-09-16 17:15:01 PDT
bug 407190 is only about http. XHR can handle also other protocols.
So, for XHR.timeout we should use a timer in XHR.
Comment 24 Alex Vincent [:WeirdAl] 2011-09-17 12:04:59 PDT
I see a lot of possibilities for tests:
* Setting the timeout twice to different values.
* Setting the timeout twice at different times in the XHR lifetime (before open, before send, after send, after the response comes back).
* Testing that aborts, errors and successful responses cancel the timeout.
* Testing the event dispatch actually happens when it's supposed to (and doesn't when it's not supposed to).
* Ensuring progress events still fire until the timeout.
* Setting the timeout to zero clears the timeout.

The possible combinations get very large, very fast.

I also see we have some XHR tests in content mochitest.  I'm not really comfortable with that, considering that Mochitest is much heavier than xpcshell-test.  https://developer.mozilla.org/en/Mochitest#Try_to_avoid_Mochitest

I'm actually thinking about writing a specialized xpchsell test function add_xhr_test(), where we could specify a sequence of actions and when they happen on the client side ("set error handler", "set timeout handler", "send", "set timeout", etc.), what the test server should do with the request and response ("delay 200 ms", "set response header", "finish response", etc.), and what the XHR should expect to happen ("error event dispatch", "timeout fires", "response received OK", etc.).

This function (like Jasmine) would queue up each test.  We'd call add_xhr_test for each combination we'd want to test.  Then when we call a fire_xhr_tests() function, all the XHR tests start, with do_test_pending() being called first to make sure xpcshell doesn't quit on us in the middle.  As the responses come in, we call do_test_finished() at the end of each sequence (try... finally { do_test_finished() }).

Queueing up all the tests first would mean running all the tests simultaneously - shortening the execution time of the test file and making it much less likely the test execution would time out.  

The downside is that it really isn't a test of the environment XHR is supposed to run in (an unprivileged web page).

Opinions?
Comment 25 Olli Pettay [:smaug] 2011-09-18 00:52:52 PDT
Please don't add xpcshell tests, we can't run them in other browsers.
Comment 26 Alex Vincent [:WeirdAl] 2011-09-18 22:32:30 PDT
Hm.  It's the "other browsers" testing that throws me off, not just in xpcshell...

As I understand it, the HTTP server available to the content HTML tests is Gecko specific.  So how would I test this, unless I had a server guaranteed to live forever and to respond the way I expect?
Comment 27 Olli Pettay [:smaug] 2011-09-18 22:39:51 PDT
(In reply to Alex Vincent [:WeirdAl] from comment #26)
> As I understand it, the HTTP server available to the content HTML tests is
> Gecko specific.
Nothing Gecko specific. It is just an HTTP server.
(Though, the testing framework does assume particular proxy settings.)

> So how would I test this, unless I had a server guaranteed
> to live forever and to respond the way I expect?
?

And even more importantly, we usually want to test things the way they are used in web pages.
xpcshell isn't such testing framework.
Comment 28 Alex Vincent [:WeirdAl] 2011-10-12 17:39:03 PDT
Created attachment 566702 [details] [diff] [review]
work in progress, mostly untested

I have a single test, and right now it's failing.  The error event is firing unexpectedly.
Comment 29 Alex Vincent [:WeirdAl] 2011-10-30 03:24:45 PDT
Created attachment 570535 [details] [diff] [review]
work in progress, mostly untested

It turns out Abort() was setting a special flag, and the OnStartRequest() code was expecting that special flag.  Something doesn't feel right about that logic, in the HTML5 world.

Olli, before I start going hog-wild on tests a la comment 24, could you give this a quick glance for anything obvious?
Comment 30 Olli Pettay [:smaug] 2011-11-11 12:30:36 PST
Comment on attachment 570535 [details] [diff] [review]
work in progress, mostly untested


> #define XML_HTTP_REQUEST_NEED_AC_PREFLIGHT (1 << 16) // Internal
> #define XML_HTTP_REQUEST_AC_WITH_CREDENTIALS (1 << 17) // Internal
>+#define XML_HTTP_REQUEST_TIMED_OUT (1 << 18) // Internal
I don't really like new flags, but looks like in this case it is quite handy


>-NS_IMETHODIMP
>-nsXMLHttpRequest::Abort()
>+  /* XXX ajvincent Apparently, Abort() should set XML_HTTP_REQUEST_UNSENT.  See
>+     bug 361773.  This should be reviewed against the HTML5 specification... */
HTML5 spec? XHR isn't defined in HTML5.


>+  // XXX ajvincent Should we have something in OnStopRequest for timeout timers?
>+
Probably yes. Does OnStopRequest get called in that case? If yes, OnStopRequest shouldn't really do
anything. But, check what abort() does.


What about workers?


Looks good.
Comment 31 Alex Vincent [:WeirdAl] 2011-11-12 01:06:43 PST
I need some advice on forcing an error event to fire on a XHR from a .sjs test script.  I'm currently using the following:

      response.setStatusLine(null, 302, "Moved");
      // this should trigger a cross-site redirect error
      response.setHeader("Location", "http://mozilla.org", false);

That said, it triggers a NS_WARNING every time I do that.  I'm wondering if there's some other viable way to force the error from inside handleRequest().
Comment 32 Alex Vincent [:WeirdAl] 2011-11-16 22:00:47 PST
Created attachment 575094 [details] [diff] [review]
work in progress, test 1/2 done

I'm writing my tests, and I think I've caught an additional bug, per the XHR2 spec.

One of my tests is about which events fire, and when.  Specifically, per my understanding of the spec, only one of the following 4 events should fire per XMLHttpRequest:  load, error, abort, timeout.  I haven't gotten to testing timeout yet.  I'm seeing that calling abort() after the load event has fired triggers an abort event.

This conflicts with the specification for the abort() method under step 5:
http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-abort-method

"If the state is UNSENT, OPENED with the send() flag being false, or DONE go to the next step.  Otherwise... Fire a progress event named abort."

DONE should be set before the load event fires.

Granted, my patch does change the path Abort() takes, but only slightly.  I reran the test with only content/base/test altered to include my new test - the original XHR code was unchanged - and it failed in exactly the same way for the same reason.

So where is the mistake?  Is an abort event allowed per the spec after a load event or error event fires?  If so, where does it say that?  If not, what should I do?
Comment 33 Alex Vincent [:WeirdAl] 2011-11-17 13:29:48 PST
Spec author confirms:  "they are mutual exclusive events".  So, that's a new bug to file.
Comment 34 Olli Pettay [:smaug] 2011-11-18 05:51:12 PST
FYI, the spec is about to change to prevent using timeout with sync XHR.
Comment 35 Olli Pettay [:smaug] 2011-11-18 07:41:12 PST
Apparently I accidentally assigned this to nobody.
Comment 36 Jonas Sicking (:sicking) 2011-11-18 09:05:34 PST
I don't think we have discussed limiting timeout to just asynchronous. Timeout is in fact mostly useful in sync mode since in asynchronous you can simply call .abort from a timeout.

I don't think we'll be successful in forcing everyone off of using the sync API. In the cases where we are not it's good for users if the timeout feature is available to prevent extended UI freeze.
Comment 37 Olli Pettay [:smaug] 2011-11-18 09:33:05 PST
ATM, I'm strongly against adding timeout for sync. That would make sync behave perhaps a bit
better in some cases, but would also hint that it is ok to use sync XHR.
Even 50ms sync XHR is bad for the UI.

Timeout is useful for async too. No need to create separate timers for all the XHRs.
Comment 38 Jonas Sicking (:sicking) 2011-11-18 12:18:38 PST
I honestly don't think people care about what "hints" we give them. If you're worried about giving the wrong impression we can always add a warning in the console every time a sync XHR is done (no matter if .timeout is used or not).

The whole reason we don't like sync XHR is that it gives users a bad experience. .timeout can't fix that, but can improve it.

In any case, you need to convince the WG and not just me that .timeout shouldn't work for sync.
Comment 39 Olli Pettay [:smaug] 2011-11-18 12:20:23 PST
I don't see *any* reasons to add any new features to sync XHR.

And I did discuss about this with Anne, and he was going to change the spec.
Comment 40 Jonas Sicking (:sicking) 2011-11-18 12:29:59 PST
I think this is going to hurt users. I hope I am wrong.
Comment 41 Olli Pettay [:smaug] 2011-11-18 12:41:19 PST
I don't understand how trying to reduce usage of sync XHR will hurt users.
If we add timeout, it is way easier for a web developer to think that, hey, blocking UI
for (for example) 50ms isn't that bad.
Comment 42 Jonas Sicking (:sicking) 2011-11-18 12:48:21 PST
I don't think having .timeout is going to increase sync-XHR usage significantly. However having it can change pages from locking up for 10 seconds to 1 second will be a big improvement for users where it's used.
Comment 43 Alex Vincent [:WeirdAl] 2011-11-18 13:19:12 PST
May I suggest taking this discussion to the working group, and when the XHR2 contributors resolve it, tell me what the decision is?
Comment 44 Olli Pettay [:smaug] 2011-11-18 13:23:00 PST
Sure. You could implement whatever the spec says, and if or when timeout will be disabled, we could
disabled that for sync XHR.
Comment 45 Olli Pettay [:smaug] 2011-11-18 13:23:39 PST
Ofc, the spec does say currently 
"If there is an associated XMLHttpRequest document and the synchronous flag is set, throw an "InvalidAccessError" exception and terminate these steps."
Comment 46 Ben Bucksch (:BenB) 2011-11-23 10:28:01 PST
Sicking, as the bug filer, I filed this for async request types.
And no, abort() is a hack that every XHR user needs to implement. It is a basic feature that everybody needs, thus it should be available.

Also, I still believe that we should first use the socket timeout (and do optionally abort only as a fallback when that one doesn't react/work). Socket timeout is the correct solution, abort() is the sledge hammer solution.

Also note that the error handling would be different: Whether it's 1) a Cancel by the user 2) a cancel by the app (call is superseded by something else) or 3) it's a network timeout makes a big difference in the error handling. Only the last case is a real error, the others are not. Lumping all cases together makes processing further down also more difficult.
Timeout should be treated like a network error, because it usually is, not like an abort from the application or user.
Comment 47 Alex Vincent [:WeirdAl] 2011-12-03 01:44:52 PST
Created attachment 578817 [details] [diff] [review]
Implement timeout on XMLHttpRequest (not counting workers)

As directed, I rewrote the test completely (simplifying it, making it very clear what the scenarios are).  I also added a code check for synchronous XMLHttpRequests with at-least-partial XML documents built, per the new spec requirement.  (This last part I haven't written a test yet for.)

The Web Workers support is not in this patch; I figured I'd do that separately, and get reviews going on this first part early.
Comment 48 Olli Pettay [:smaug] 2011-12-07 04:10:28 PST
Comment on attachment 578817 [details] [diff] [review]
Implement timeout on XMLHttpRequest (not counting workers)


>+void
>+nsXMLHttpRequest::CloseRequestWithError(const nsAString& aType,
>+                                        const PRUint32 aFlag) {
{ should be in the next line


>+  // XXX ajvincent So many things to cancel.  Should we have a stack of objects?
Perhaps a helper method for cancelling, especially if we cancel all the same things in many places.
>   // make sure to notify the listener if we were aborted
>   // XXX in fact, why don't we do the cleanup below in this case??
>-  if (mState & XML_HTTP_REQUEST_UNSENT) {
>+  // XML_HTTP_REQUEST_UNSENT is for abort calls.  See OnStartRequest above.
>+  if ((mState & XML_HTTP_REQUEST_UNSENT) ||
>+      (mState & XML_HTTP_REQUEST_TIMED_OUT)) {
>     if (mXMLParserStreamListener)
>       (void) mXMLParserStreamListener->OnStopRequest(request, ctxt, status);
>     return NS_OK;
>   }
> 
>+
Useless newline


>+nsXMLHttpRequest::SetTimeout(PRUint32 aTimeout)
>+{
>+  if ((mState & XML_HTTP_REQUEST_ASYNC) || !mResponseXML) {
I don't understand the " || !mResponseXML" part


>+nsXMLHttpRequest::StartTimeoutTimer()
>+{
>+  NS_ABORT_IF_FALSE(mRequestSentTime,
>+                    "StartTimeoutTimer shouldn't be called before the request was sent!");
Here you abort if mRequestSentTime == 0


>+  if (mState & XML_HTTP_REQUEST_DONE) {
>+    // do nothing!
>+    return;
>+  }
>+
>+  if (mTimeoutTimer) {
>+    mTimeoutTimer->Cancel();
>+  }
>+
>+  if (!mTimeoutMilliseconds) {
>+    return;
>+  }
>+
>+  PRUint32 elapsed = mRequestSentTime ?
>+                     (PRUint32)((PR_Now() - mRequestSentTime) / 1000) :
>+                     0;
But here you handle the case when mRequestSentTime is 0.


>+  if (!mTimeoutTimer) {
>+    mTimeoutTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
>+  }
>+  // Relying on infallible malloc to ensure mTimeoutTimer got created...
>+  mTimeoutTimer->InitWithCallback(
>+    this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT
>+  );
>+}
What if mTimeoutMilliseconds < elapsed?
Comment 49 Alex Vincent [:WeirdAl] 2011-12-07 15:52:18 PST
There was one other thing which slipped into the patch that shouldn't have:  a line which added nsIParser.h.

(In reply to Olli Pettay [:smaug] from comment #48)
> >+nsXMLHttpRequest::SetTimeout(PRUint32 aTimeout)
> >+{
> >+  if ((mState & XML_HTTP_REQUEST_ASYNC) || !mResponseXML) {
> I don't understand the " || !mResponseXML" part

This came from the spec for setting the timeout:
'If there is an associated XMLHttpRequest document and the synchronous flag is set, throw an "InvalidAccessError" exception and terminate these steps.'

Previously, I hadn't noticed the requirement for a document.  The document is created in nsXMLHttpRequest::OnStartRequest.

> >+nsXMLHttpRequest::StartTimeoutTimer()
> >+{
> >+  NS_ABORT_IF_FALSE(mRequestSentTime,
> >+                    "StartTimeoutTimer shouldn't be called before the request was sent!");
> Here you abort if mRequestSentTime == 0
> 
> ...
> >+  PRUint32 elapsed = mRequestSentTime ?
> >+                     (PRUint32)((PR_Now() - mRequestSentTime) / 1000) :
> >+                     0;
> But here you handle the case when mRequestSentTime is 0.

Deliberately so.  The abort is an assertion check (we're not supposed to use NS_ERROR anymore, and NS_PRECONDITION didn't seem appropriate).  mRequestSentTime should *never* be 0 when this function executes.

That said, it's a debug-only check:  it doesn't affect opt builds.

> >+  if (!mTimeoutTimer) {
> >+    mTimeoutTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
> >+  }
> >+  // Relying on infallible malloc to ensure mTimeoutTimer got created...
> >+  mTimeoutTimer->InitWithCallback(
> >+    this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT
> >+  );
> >+}
> What if mTimeoutMilliseconds < elapsed?

Yeah, that's a bug that I missed.  mTimeoutMilliseconds should *never* be less than elapsed, though (or the timer would have fired).  I'm not sure if I should have an NS_ABORT_IF_FALSE for that, or simply a NS_WARNING.  Certainly if mTimeoutMilliseconds < elapsed, I shouldn't init the timer.
Comment 50 Ben Bucksch (:BenB) 2011-12-07 16:00:43 PST
> mTimeoutMilliseconds should *never* be less than elapsed, though (or the timer
> would have fired).  I'm not sure if I should have an NS_ABORT_IF_FALSE for that,
> or simply a NS_WARNING.

If I remember correctly, nsITimer are not 100% exact, but "best effort". To my knowledge, it's impossible to guarantee exact timing, because there's the event loop for this thread and an event right before might take longer than 1ms to run and return to the loop. Or another thread might use CPU resources and there's only one core.
Comment 51 Jonas Sicking (:sicking) 2011-12-07 16:11:00 PST
(In reply to Alex Vincent [:WeirdAl] from comment #49)
> There was one other thing which slipped into the patch that shouldn't have: 
> a line which added nsIParser.h.
> 
> (In reply to Olli Pettay [:smaug] from comment #48)
> > >+nsXMLHttpRequest::SetTimeout(PRUint32 aTimeout)
> > >+{
> > >+  if ((mState & XML_HTTP_REQUEST_ASYNC) || !mResponseXML) {
> > I don't understand the " || !mResponseXML" part
> 
> This came from the spec for setting the timeout:
> 'If there is an associated XMLHttpRequest document and the synchronous flag
> is set, throw an "InvalidAccessError" exception and terminate these steps.'
> 
> Previously, I hadn't noticed the requirement for a document.  The document
> is created in nsXMLHttpRequest::OnStartRequest.

"associated XMLHttpRequest document" doesn't refer to the document created when loading a document resource. It refers to there being a context document when the XHR object is created. I.e. when it's created in a window rather than in a worker.
Comment 52 Masatoshi Kimura [:emk] 2011-12-07 19:45:43 PST
if ((mState & XML_HTTP_REQUEST_ASYNC) || !mOwner) {
should be the correct test condition. You can create a xpcshell test to make sure .timeout works for sync XHR in non-window context.
Comment 53 Alex Vincent [:WeirdAl] 2011-12-08 01:33:27 PST
Created attachment 579985 [details] [diff] [review]
Implement timeout on XMLHttpRequest (not counting workers)

Fixes applied based on review feedback and comments therein.  I've also used http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed to make life easier on others.
Comment 54 Ben Bucksch (:BenB) 2011-12-08 03:05:11 PST
> +    this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT

not addressed yet
Comment 55 Alex Vincent [:WeirdAl] 2011-12-08 08:25:43 PST
(In reply to Ben Bucksch (:BenB) from comment #54)
> > +    this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT
> 
> not addressed yet

What do you mean?  I now have these lines shortly before that:

+  if (elapsed > mTimeoutMilliseconds) {
+    /* This really, really shouldn't happen.  Theoretically, it can, because
+       the timer runs on a different thread than the request. */
+    NS_WARNING("Timeout timer hasn't fired, but time has expired!");
+    HandleTimeoutCallback();
+    return;
+  }
Comment 56 Ben Bucksch (:BenB) 2011-12-08 16:29:32 PST
Ale, as I said, this is *not* an exceptional situation. It is expected. You cannot expect a timer to be on a millisecond precise, on a multi-tasking, non-realtime OS, and with event handlers that may well take more than 1ms to execute.

See e.g. the notes at <https://developer.mozilla.org/en/nsITimer#Constants>. This also suggests that nsITimer cannot guarantee precise execution. It is logical, for above reasons.

So, no warning, no extra handling. Just treat elapsed > mTimeoutMilliseconds as if elapsed == mTimeoutMilliseconds .
Comment 57 Ben Bucksch (:BenB) 2011-12-08 16:34:55 PST
So, how about?
instead of
    this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT
do
    this, max(mTimeoutMilliseconds - elapsed, 0), nsITimer::TYPE_ONE_SHOT
Comment 58 Alex Vincent [:WeirdAl] 2011-12-08 16:57:52 PST
(In reply to Ben Bucksch (:BenB) from comment #57)
> So, how about?
> instead of
>     this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT
> do
>     this, max(mTimeoutMilliseconds - elapsed, 0), nsITimer::TYPE_ONE_SHOT

OK, I'll buy that.  :)

Incidentally, this patch has passed try-server (yay!)
https://tbpl.mozilla.org/?tree=Try&rev=0dfcd86957d7
Comment 59 Olli Pettay [:smaug] 2011-12-14 06:11:41 PST
Comment on attachment 579985 [details] [diff] [review]
Implement timeout on XMLHttpRequest (not counting workers)

>+  // Don't do anything if we have timed out either.
Strange sentence, or my English is bad. what "either"


>+  // Relying on infallible malloc to ensure mTimeoutTimer got created...
No need to add this comment

>+
>+  // XXX ajvincent This test suite does not cover synchronous XMLHttpRequests. 
Add some test to make sure timeout + sync XHR isn't possible.

r+ (Fix also BenB's comments)
Comment 60 Alex Vincent [:WeirdAl] 2011-12-14 10:43:42 PST
Comment on attachment 579985 [details] [diff] [review]
Implement timeout on XMLHttpRequest (not counting workers)

I'll fix review nits later this week.  Also, I'll try to deliver a workers patch (separate from this) by the weekend.

Jonas, is there a reasonable chance of getting super-review comments before Saturday?  That way, I would probably only have to resubmit a patch once.
Comment 61 Alex Vincent [:WeirdAl] 2011-12-14 12:55:23 PST
:( Something else I missed considering, when the user sets the timeout attribute before calling open().

My tests didn't cover that before (as noted above, I don't have sync tests yet), but they will.  The patch I attached missed that as well.  I'll fix that as part of my review nits patch.
Comment 62 Alex Vincent [:WeirdAl] 2011-12-17 15:44:14 PST
Created attachment 582603 [details] [diff] [review]
revised patch per review comments

Due to the missing XHR sync timeout cases - and the bug found therein - I'm resubmitting this patch for review by smaug as well.

Although the patch looks a lot bigger, it really isn't.  This is because hg qrefresh was giving me only 3 lines of context, and this patch has 8.
Comment 63 Alex Vincent [:WeirdAl] 2011-12-17 19:14:21 PST
Created attachment 582620 [details]
IRC conversation on worker XHR timeouts
Comment 64 Alex Vincent [:WeirdAl] 2011-12-17 19:56:58 PST
The workers part is looking very complicated.  Would there be any objection to landing that separately from the main XHR work, possibly as part of a new bug?
Comment 65 Alex Vincent [:WeirdAl] 2011-12-17 21:05:38 PST
Comment on attachment 582603 [details] [diff] [review]
revised patch per review comments

>diff -r 6785d3003414 content/base/src/nsXMLHttpRequest.cpp
>--- a/content/base/src/nsXMLHttpRequest.cpp	Thu Dec 08 13:57:41 2011 +1300
>+++ b/content/base/src/nsXMLHttpRequest.cpp	Sat Dec 17 15:36:22 2011 -0800
>@@ -1575,16 +1604,32 @@ nsXMLHttpRequest::Open(const nsACString&

>+  if (!async && mOwner) {
>+    /*
>+      From the XHR2 spec:
>+      if ... either the timeout attribute value is not zero, the withCredentials
>+      attribute value is true, or the responseType attribute value is not the
>+      empty string, throw an "InvalidAccessError" exception and terminate these
>+      steps.
>+
>+      We don't yet implement withCredentials.
>+    */
>+    if (mTimeoutMilliseconds ||
>+        (mResponseType != XML_HTTP_RESPONSE_TYPE_DEFAULT)) {
>+      return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+    }
>+  }

Whoops.  This broke content/base/test/test_XHR.html on bug 680816.
Comment 66 Masatoshi Kimura [:emk] 2011-12-17 21:12:40 PST
(In reply to Alex Vincent [:WeirdAl] from comment #64)
> The workers part is looking very complicated.  Would there be any objection
> to landing that separately from the main XHR work, possibly as part of a new
> bug?
It's already present (bug 498998).
Comment 67 Alex Vincent [:WeirdAl] 2012-01-05 18:56:00 PST
Created attachment 586317 [details] [diff] [review]
updated to tip
Comment 68 Olli Pettay [:smaug] 2012-01-19 15:19:16 PST
Please ping me on IRC if I don't review this tomorrow.
(and sorry for the delay)
Comment 69 Olli Pettay [:smaug] 2012-01-22 12:06:24 PST
/me kicks himself. I really should review this. Sorry for the delay.
Comment 70 Olli Pettay [:smaug] 2012-01-24 00:30:27 PST
Comment on attachment 586317 [details] [diff] [review]
updated to tip

>+  mTimeoutTimer->InitWithCallback(
>+    this, PR_MAX(mTimeoutMilliseconds - elapsed, 0), nsITimer::TYPE_ONE_SHOT
>+  );
So did you actually test this?
mTimeoutMilliseconds and elapsed are both unsigned, so mTimeoutMilliseconds - elapsed can be
some really big number.
And yes, BenB suggested that and I approved it, but it is wrong :)
  mTimeoutTimer->InitWithCallback(
    this, mTimeoutMilliseconds > elapsed ? mTimeoutMilliseconds - elapsed : 0, nsITimer::TYPE_ONE_SHOT
  );
should work.
Comment 71 Ben Bucksch (:BenB) 2012-01-24 04:00:59 PST
> mTimeoutMilliseconds > elapsed ? mTimeoutMilliseconds - elapsed : 0

fine with me. Thanks for the catch.
Comment 72 Jonas Sicking (:sicking) 2012-01-25 00:02:11 PST
Comment on attachment 586317 [details] [diff] [review]
updated to tip

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

sr=me with those things fixed.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1583,5 @@
>    }
>  
> +  if (mState & XML_HTTP_REQUEST_TIMED_OUT) {
> +    mState &= ~XML_HTTP_REQUEST_TIMED_OUT;
> +  }

No need for the if-check. Always do the boolean operation.

Actually, you can merge the two boolean operations into something like

mState &= ~XML_HTTP_REQUEST_ABORTED & ~XML_HTTP_REQUEST_TIMED_OUT;

And adjust the comment of course.

@@ +2905,5 @@
> +    return;
> +  }
> +
> +  PRUint32 elapsed =
> +    (PRUint32)((PR_Now() - mRequestSentTime) / PR_USEC_PER_MSEC);

I'd move this to below the code that instantiates the timer. Just so that we take that into account when measuring time. It's not a big deal, but I can't see a reason not to do it right.
Comment 73 Alex Vincent [:WeirdAl] 2012-01-25 09:05:37 PST
(In reply to Olli Pettay [:smaug] from comment #70)
> >+  mTimeoutTimer->InitWithCallback(
> >+    this, PR_MAX(mTimeoutMilliseconds - elapsed, 0), nsITimer::TYPE_ONE_SHOT
> >+  );
> So did you actually test this?

Whoops - according to the logic I'd written into my test, I actually missed a case:

new RequestTracker("timeout fires after initially set after the load would happen", 5000, 2000, 1000),

Stupid combinatorics.  :)  I think it'll still pass, but I will make the above adjustments and submit a final patch as soon as I can.
Comment 74 Alex Vincent [:WeirdAl] 2012-01-25 10:42:56 PST
Correction:  I did cover it:
new RequestTracker("timeout overrides load after a delay", 5000, 1000, 2000),
Comment 75 Alex Vincent [:WeirdAl] 2012-01-25 14:58:51 PST
Created attachment 591623 [details] [diff] [review]
final patch for check-in
Comment 77 Marco Bonardo [::mak] 2012-01-26 15:58:38 PST
https://hg.mozilla.org/mozilla-central/rev/22797d62aa14
Comment 78 Eric Shepherd [:sheppy] 2012-04-20 13:48:02 PDT
Documentation updated:

https://developer.mozilla.org/en/DOM/XMLHttpRequest/Using_XMLHttpRequest#Example:_using_a_timeout
https://developer.mozilla.org/en/DOM/XMLHttpRequest

Also added:

https://developer.mozilla.org/en/DOM/XMLHttpRequestEventTarget (although it needs more content added).

Listed on Firefox 12 for developers.
Comment 79 Marcel Ruff 2012-04-28 01:59:30 PDT
My Javascript code fails since Firefox 12 with this error:

A parameter or an operation is not supported by the underlying object" code: "15" nsresult: "0x8053000f (NS_ERROR_DOM_INVALID_ACCESS_ERR)"

The code is:
  var timeoutMillis = 5000;
  var timeoutNatively = typeof myXmlHttp.timeout!="undefined"; 
  try {
    if (timeoutNatively) {
      // Firefox 11.0 myXmlHttp.timeout is "undefined"
      // Firefox 12.0 myXmlHttp.timeout is 0 (Number) but throws exception
      myXmlHttp.timeout = timeoutMillis;
      myXmlHttp.ontimeout = function () {
        alert("timed out: " + myXmlHttp.timeout);
      }
    }
  }
  catch (ex) {
    alert(ex);
  }


How can I set the timeout?
Comment 80 Olli Pettay [:smaug] 2012-04-28 02:03:25 PDT
Are you using synchronous XMLHttpRequest? timeout is supported only in asynchronous mode.
Comment 82 Marcel Ruff 2012-04-28 02:30:16 PDT
Yes, my mistake, I was in synchronous mode
Sorry the noise.
Comment 83 :Ms2ger 2012-09-05 06:02:02 PDT
Alex, do you mind if I submit your tests to the XHR test suite under the 3-clause BSD license?
Comment 84 Alex Vincent [:WeirdAl] 2012-09-05 07:26:35 PDT
Ms2ger, I have no objection to spreading the tests around (see bug 498998 comment 20), but I don't know the "3-clause BSD license".  Can you clarify what that means?

Personally, all I really want is to be notified when someone uses the tests in their code.  It makes me feel all warm and fuzzy.
Comment 85 :Ms2ger 2012-09-05 07:31:05 PDT
http://www.w3.org/Consortium/Legal/2008/03-bsd-license.html; but PD is excellent too, of course.
Comment 86 Alex Vincent [:WeirdAl] 2012-09-05 07:42:11 PDT
Either PD or the 3-clause BSD license is fine with me.
Comment 87 Dominik Röttsches (drott) 2012-09-05 15:48:55 PDT
Will try to make sure you feel warm and fuzzy and give credit in the changelog.
Comment 88 Dominik Röttsches (drott) 2013-02-19 01:02:06 PST
(In reply to Alex Vincent [:WeirdAl] from comment #33)
> Spec author confirms:  "they are mutual exclusive events".  So, that's a new
> bug to file.

Anne filed bug 842357 - that's probably what you meant.
Comment 89 Alex Vincent [:WeirdAl] 2013-02-19 07:37:31 PST
(In reply to Dominik Röttsches (drott) from comment #88)
> (In reply to Alex Vincent [:WeirdAl] from comment #33)
> > Spec author confirms:  "they are mutual exclusive events".  So, that's a new
> > bug to file.
> 
> Anne filed bug 842357 - that's probably what you meant.

No, that was bug 703380.
Comment 90 Dominik Röttsches (drott) 2013-02-19 07:55:58 PST
(In reply to Alex Vincent [:WeirdAl] from comment #89)
> No, that was bug 703380.

Oh, okay - thanks for clarifying.

Note You need to log in before you can comment on or make changes to this bug.