Bug 525816 (xhr-timeout)

XMLHttpRequest should allow to specify a network timeout in ms (for async requests)

RESOLVED FIXED in mozilla12

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
8 years ago
4 months ago

People

(Reporter: BenB, Assigned: WeirdAl)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

Other Branch
mozilla12
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

8 years ago
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.)
(Reporter)

Comment 1

8 years ago
We should ping the W3C about that, too, to standardize it, if possible.
(Reporter)

Updated

8 years ago
Summary: XMLHttpRequest should allow for a network timeout → XMLHttpRequest should allow to specify a network timeout in ms

Comment 2

8 years ago
(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)
(Reporter)

Comment 5

8 years ago
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!
(Reporter)

Updated

8 years ago
Severity: normal → enhancement
OS: Linux → All
Hardware: x86 → All

Comment 6

8 years ago
(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

7 years ago
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.
(Assignee)

Comment 8

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

Comment 10

7 years ago
It has been part of XMLHttpRequest Level 2 for a while now by the way...
(Assignee)

Comment 11

7 years ago
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.
Assignee: nobody → Olli.Pettay
...if some spec issues are resolved soon.
(Assignee)

Comment 15

6 years ago
(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...
(Assignee)

Comment 16

6 years ago
<smaug> WeirdAl: the spec has been fixed
(Assignee)

Comment 17

6 years ago
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...
(Reporter)

Comment 18

6 years ago
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?
(Assignee)

Comment 19

6 years ago
(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.
(Assignee)

Comment 20

6 years ago
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?
(Reporter)

Comment 22

6 years ago
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.
(Assignee)

Updated

6 years ago
Alias: xhr-timeout
(Assignee)

Comment 24

6 years ago
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.
(Assignee)

Comment 26

6 years ago
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.
(Assignee)

Comment 28

6 years ago
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.
Attachment #556189 - Attachment is obsolete: true
(Assignee)

Comment 29

6 years ago
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?
Attachment #566702 - Attachment is obsolete: true
Attachment #570535 - Flags: feedback?(Olli.Pettay)
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.
Attachment #570535 - Flags: feedback?(bugs) → feedback+
(Assignee)

Comment 31

6 years ago
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().
(Assignee)

Comment 32

6 years ago
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?
Attachment #570535 - Attachment is obsolete: true
(Assignee)

Comment 33

6 years ago
Spec author confirms:  "they are mutual exclusive events".  So, that's a new bug to file.
(Assignee)

Updated

6 years ago
(Assignee)

Updated

6 years ago
Assignee: bugs → ajvincent
(Assignee)

Updated

6 years ago
Depends on: 703380
FYI, the spec is about to change to prevent using timeout with sync XHR.
Assignee: ajvincent → nobody
Target Milestone: --- → mozilla10
Version: Trunk → Other Branch
Apparently I accidentally assigned this to nobody.
Assignee: nobody → ajvincent
Target Milestone: mozilla10 → ---
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.
(Assignee)

Comment 43

6 years ago
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."
(Reporter)

Comment 46

6 years ago
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.
Summary: XMLHttpRequest should allow to specify a network timeout in ms → XMLHttpRequest should allow to specify a network timeout in ms (for async requests)
(Assignee)

Comment 47

6 years ago
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.
Attachment #575094 - Attachment is obsolete: true
Attachment #578817 - Flags: review?(bugs)
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?
Attachment #578817 - Flags: review?(bugs) → review-
(Assignee)

Comment 49

6 years ago
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.
(Reporter)

Comment 50

6 years ago
> 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.
(Assignee)

Comment 53

6 years ago
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.
Attachment #578817 - Attachment is obsolete: true
Attachment #579985 - Flags: review?(bugs)
(Reporter)

Comment 54

6 years ago
> +    this, mTimeoutMilliseconds - elapsed, nsITimer::TYPE_ONE_SHOT

not addressed yet
(Assignee)

Comment 55

6 years ago
(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;
+  }
(Reporter)

Comment 56

6 years ago
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 .
(Reporter)

Comment 57

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

Comment 58

6 years ago
(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)
Attachment #579985 - Flags: review?(bugs) → review+
(Assignee)

Comment 60

6 years ago
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.
Attachment #579985 - Flags: superreview?(jonas)
(Assignee)

Comment 61

6 years ago
:( 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.
(Assignee)

Comment 62

6 years ago
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.
Attachment #579985 - Attachment is obsolete: true
Attachment #582603 - Flags: superreview?(jonas)
Attachment #582603 - Flags: review?(bugs)
Attachment #579985 - Flags: superreview?(jonas)
(Assignee)

Comment 63

6 years ago
Created attachment 582620 [details]
IRC conversation on worker XHR timeouts
(Assignee)

Comment 64

6 years ago
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?
(Assignee)

Comment 65

6 years ago
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.
Attachment #582603 - Flags: superreview?(jonas)
Attachment #582603 - Flags: review?(bugs)
(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).
Blocks: 498998
(Assignee)

Comment 67

6 years ago
Created attachment 586317 [details] [diff] [review]
updated to tip
Attachment #582603 - Attachment is obsolete: true
Attachment #586317 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Attachment #586317 - Flags: superreview?(jonas)
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.
Attachment #586317 - Flags: review?(bugs) → review+
(Reporter)

Comment 71

5 years ago
> 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.
Attachment #586317 - Flags: superreview?(jonas) → superreview+
(Assignee)

Comment 73

5 years ago
(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.
(Assignee)

Comment 74

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

Comment 75

5 years ago
Created attachment 591623 [details] [diff] [review]
final patch for check-in
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/22797d62aa14
Flags: in-testsuite+
Keywords: checkin-needed → dev-doc-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/22797d62aa14
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 722076
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.
Keywords: dev-doc-needed → dev-doc-complete

Updated

5 years ago
Depends on: 749098

Comment 79

5 years ago
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.
http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#dom-xmlhttprequest-timeout

Comment 82

5 years ago
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?
(Assignee)

Comment 84

5 years ago
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.
(Assignee)

Comment 86

5 years ago
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.
(Assignee)

Comment 89

4 years ago
(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.
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
(Assignee)

Updated

4 months ago
Blocks: 1332251
You need to log in before you can comment on or make changes to this bug.