Mike Wilson Reporter
	Description • 15 years ago

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/530.5 (KHTML, like Gecko) Chrome/2.0.172.31 Safari/530.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11

IE8 has implemented timeouts for synchronous requests as an extension to XMLHttpRequest, see: http://msdn.microsoft.com/en-us/library/cc304105(VS.85).aspx

Excerpt:
  xhr = new XMLHttpRequest();
  xhr.open("GET", url, true);
  xhr.timeout = 10000;
  xhr.ontimeout = timeoutFired;
  xhr.send(null);

Being able to specify a timeout is a great helper for scenarios that require sync XHR so it would be great to see Mozilla implement something similar.

Reproducible: Always

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 1 • 15 years ago

Shouldn't the timeout be specified in XHR 2 spec?

	Mike Wilson Reporter
	Comment 2 • 15 years ago

I actually brought this up for XHR2 back in 2007, see:
http://lists.w3.org/Archives/Public/public-webapi/2007Feb/0095.html
and chimed in on Jonas's suggestion in 2008
http://lists.w3.org/Archives/Public/public-webapps/2008JulSep/0565.html
here
http://lists.w3.org/Archives/Public/public-webapps/2008JulSep/0616.html
(note the change of subject title)

I think this track record shows that the XHR working group is not ready to add this feature to the spec, and with Microsoft's precedent I think it is now a good time to add this feature from the vendor side instead. If spreading to other vendors, it would probably be included in a future XHR spec version.

	Anne (:annevk)
	Comment 3 • 15 years ago

If anything, those emails indicate that the proposal is not clear on the details and that we have to figure those out.

	William J. Edney
	Comment 4 • 15 years ago

See also:

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

	Masatoshi Kimura [:emk]
	Comment 5 • 13 years ago

The timeout was added to the XHR spec and it will throw TimeoutError instead of firing a timeout event for sync XHR.
http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#timeout-error

	Masatoshi Kimura [:emk]
	Comment 6 • 13 years ago

>  xhr.open("GET", url, true);
By the way, this is an example of *async* XHR.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 13 years ago

Oh, we want to get rid of that, for Window context. We should not add any new features to
sync XHR. We should kill sync XHR in Window context if just possible.

	Masatoshi Kimura [:emk]
	Comment 8 • 13 years ago

Unfortunately IE8+ already supports timeout for sync XHR ;(

	Masatoshi Kimura [:emk]
	Comment 9 • 13 years ago

The spec has been updated.

	Masatoshi Kimura [:emk]
	Comment 10 • 13 years ago

Ah, Workers are still allowed set timeout for sync XHR. Morphing.

	Alex Vincent [:WeirdAl]
	Comment 11 • 13 years ago

If someone wants to take this on, I'd appreciate it.  I'm really quite busy and might not be able to have a patch for a couple months...

	Alex Vincent [:WeirdAl]
	Comment 13 • 13 years ago

Comment on attachment 605098 [details] [diff] [review]
Patch

Here's my feedback+ comments from the author of the original timeout test code.  My apologies if it's a bit pedantic.  (You can thank equally nitpicky Mozilla reviewers from a decade ago.  Bad habits...)

>diff --git a/content/base/test/test_XHR_timeout.html b/content/base/test/test_XHR_timeout.html
>+<script type="text/javascript">
>+  window.addEventListener("message", function (event) {
>+    if (event.data == "done") {
>+      SimpleTest.finish();
>+      return;
>+    }
>+    if (event.data == "start") {
>+      return;
>+    }
>+    if (event.data.type == "is") {
>+      SimpleTest.is(event.data.got, event.data.expected, event.data.msg);
>+      return;
>+    }
>+    if (event.data.type == "ok") {
>+      SimpleTest.ok(event.data.bool, event.data.msg);
>+      return;
>+    }
>+  });

This part looks like a pattern that might occur more than once in testing workers code in general.  Have you considered extracting this to a JS micro-library for tests?

>diff --git a/content/base/test/test_XHR_timeout.js b/content/base/test/test_XHR_timeout.js
>@@ -1,15 +1,41 @@
>+var inWorker = false;
>+try {
>+  inWorker = !(self instanceof Window);
>+} catch (e) {
>+  inWorker = true;
>+}

Declaring inWorker as false only to have it change value a moment later (either through try or through catch as true) is a bit odd.  Maybe:

var inWorker = true;
try {
  inWorker = !(self instanceof Window);
}
catch (e) {
  // do nothing
}

>@@ -18,46 +44,47 @@
>-function RequestTracker(id, timeLimit /*[, resetAfter, resetTo]*/) {
>+function RequestTracker(async, id, timeLimit /*[, resetAfter, resetTo]*/) {

Please add the async argument to the JavaDoc.  

>@@ -224,67 +251,80 @@ var SyncRequestSettingTimeoutBeforeOpen 

// ...

>   // Aborted requests.
>   new AbortedRequest(false),
>   new AbortedRequest(true, -1),
>   new AbortedRequest(true, 0),
>   new AbortedRequest(true, 1000),
>   new AbortedRequest(true, 5000),
>+];

Trailing comma.  I know, it doesn't mean much, but I think we won't lose much in hg blame there.

>+var MainThreadTestRequests = [
>   // Synchronous requests.
>   SyncRequestSettingTimeoutAfterOpen,
>   SyncRequestSettingTimeoutBeforeOpen
> ];
> 
>+var WorkerThreadTestRequests = [
>+  // Simple timeouts.
>+  new RequestTracker(false, "no time out scheduled, load fires normally", 0),
>+  new RequestTracker(false, "load fires normally", 5000),
>+  new RequestTracker(false, "timeout hit before load", 2000),
>+
>+  // Reset timeouts don't make much sense with a sync request ...
>+];
>+
>+if (inWorker) {
>+  TestRequests = TestRequests.concat(WorkerThreadTestRequests);
>+} else {
>+  TestRequests = TestRequests.concat(MainThreadTestRequests);
>+}

Just a thought:  Only one of these smaller arrays will be appended; why not define the smaller arrays inside the if...else block?

var ThreadTestRequests;
if (inWorker) {
  ThreadTestRequests = [ /* ... */ ];
}
else {
  ThreadTestRequests = [ /* ... */ ];
}
TestRequests = TestRequests.concat(ThreadTestRequests);

>diff --git a/dom/workers/XMLHttpRequestPrivate.cpp b/dom/workers/XMLHttpRequestPrivate.cpp
// ...
>+XMLHttpRequestPrivate::SetTimeout(JSContext* aCx, jsval aOldVal, jsval *aVp)
>+{
>+  mWorkerPrivate->AssertIsOnWorkerThread();
>+
>+  uint32_t timeout;
>+  if (!JS_ValueToECMAUint32(aCx, *aVp, &timeout)) {

What will happen if this method is called with NaN, Infinity or -Infinity?

>+  if (!runnable->Dispatch(aCx)) {
>+    return false;
>+  }
>+
>+  return true;

return runnable->Dispatch(aCx)?

>diff --git a/dom/workers/test/test_xhr_timeout.html b/dom/workers/test/test_xhr_timeout.html

>+  worker.addEventListener("message", function (event) {
>+    if (event.data == "done") {
>+      SimpleTest.finish();
>+      return;
>+    }
>+    if (event.data == "start") {
>+      return;
>+    }
>+    if (event.data.type == "is") {
>+      SimpleTest.is(event.data.got, event.data.expected, event.data.msg);
>+      return;
>+    }
>+    if (event.data.type == "ok") {
>+      SimpleTest.ok(event.data.bool, event.data.msg);
>+      return;
>+    }
>+  });

See first comment above.

I'm naturally concerned about bug 735152.

	Alex Vincent [:WeirdAl]
	Comment 14 • 13 years ago

Comment on attachment 605098 [details] [diff] [review]
Patch

Oh, I almost forgot:

>@@ -224,67 +251,80 @@ var SyncRequestSettingTimeoutBeforeOpen 


>+var WorkerThreadTestRequests = [
>+  // Simple timeouts.
>+  new RequestTracker(false, "no time out scheduled, load fires normally", 0),
>+  new RequestTracker(false, "load fires normally", 5000),
>+  new RequestTracker(false, "timeout hit before load", 2000),
>+
>+  // Reset timeouts don't make much sense with a sync request ...
>+];

Can you please change the string summaries of these bugs to indicate these are synchronous tests?  It's hard to tell the difference between a failure here and a failure in the async tests with the same message.

	Marco Bonardo [:mak]
	Comment 15 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/18ed43b863f1

	Dominik Röttsches (drott)
	Comment 16 • 12 years ago

Kyle, can you tell me what the license is for the JS tests that you wrote/extendend. I am implementing XHR timeout for WebKit in bug https://bugs.webkit.org/show_bug.cgi?id=74802 and I am curious to know whether I could import/adapt these tests for WebKit. Thanks.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 17 • 12 years ago

Alex, you wrote these tests originally.  Are you ok with public domain licensing of them?

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 12 years ago

Gerv, I thought our tests were generally public domain, except the ones we import from the W3C.  Is that not the case?

	Gervase Markham [:gerv]
	Comment 19 • 12 years ago

bz: tests have whatever licence the author gives them. In the past we've had conventions that particular groups of tests are PD, or we have relicensed them to PD or W3C Software (IIRC; whatever there definitely-open-source licence is) by getting permission, so we can share them. But we don't have a general rule "all tests must be PD".

So the licensing team has no problem with tests being made PD (use the boilerplate from http://www.mozilla.org/MPL/headers/).

Gerv

	Alex Vincent [:WeirdAl]
	Comment 20 • 12 years ago

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #17)
> Alex, you wrote these tests originally.  Are you ok with public domain
> licensing of them?

Absolutely.  That's one of the reasons (review request being the other) I rewrote them as HTML-based instead of xpcshell-based tests.

	Dominik Röttsches (drott)
	Comment 21 • 12 years ago

Great, sounds good, thanks, Alex & Gerv - I'll try to adopt them for WebKit XHR testing and see what I can do to port them to testharness.js.

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 22 • 12 years ago

Somebody want to add the appropriate license headers to mozilla-central?  Ms2ger?

	Chris Mills [:cmills]
	Comment 23 • 10 years ago

covered on https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers