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.)

We should ping the W3C about that, too, to standardize it, if possible.

(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.

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.

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)

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!

(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.

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.

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.

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.

It has been part of XMLHttpRequest Level 2 for a while now by the way...

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/ . :)

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

We could implement this for Firefox 5.

...if some spec issues are resolved soon.

(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...

<smaug> WeirdAl: the spec has been fixed

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...

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?

(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.

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

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?

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.

bug 407190 is only about http. XHR can handle also other protocols. So, for XHR.timeout we should use a timer in XHR.

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?

Please don't add xpcshell tests, we can't run them in other browsers.

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?

(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.

I have a single test, and right now it's failing. The error event is firing unexpectedly.

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 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.

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().

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?

Spec author confirms: "they are mutual exclusive events". So, that's a new bug to file.

FYI, the spec is about to change to prevent using timeout with sync XHR.

Apparently I accidentally assigned this to nobody.

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.

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.

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.

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.

I think this is going to hurt users. I hope I am wrong.

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.

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.

May I suggest taking this discussion to the working group, and when the XHR2 contributors resolve it, tell me what the decision is?

Sure. You could implement whatever the spec says, and if or when timeout will be disabled, we could disabled that for sync XHR.

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."

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.

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 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?

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.

> 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.

(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.

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.

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.

> + this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT not addressed yet

(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; + }

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 .

So, how about? instead of this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT do this, max(mTimeoutMilliseconds - elapsed, 0), nsITimer::TYPE_ONE_SHOT

(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 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 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.

:( 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.

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.

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 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.

(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).

Please ping me on IRC if I don't review this tomorrow. (and sorry for the delay)

/me kicks himself. I really should review this. Sorry for the delay.

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.

> mTimeoutMilliseconds > elapsed ? mTimeoutMilliseconds - elapsed : 0 fine with me. Thanks for the catch.

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.

(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.

Correction: I did cover it: new RequestTracker("timeout overrides load after a delay", 5000, 1000, 2000),

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.

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?

Are you using synchronous XMLHttpRequest? timeout is supported only in asynchronous mode.

Yes, my mistake, I was in synchronous mode Sorry the noise.

Alex, do you mind if I submit your tests to the XHR test suite under the 3-clause BSD license?

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.

http://www.w3.org/Consortium/Legal/2008/03-bsd-license.html; but PD is excellent too, of course.

Either PD or the 3-clause BSD license is fine with me.

Will try to make sure you feel warm and fuzzy and give credit in the changelog.

(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.

(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.

(In reply to Alex Vincent [:WeirdAl] from comment #89) > No, that was bug 703380. Oh, okay - thanks for clarifying.