Last Comment Bug 498998 - Implement timeout for XHR in Worker context
: Implement timeout for XHR in Worker context
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla13
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
Mentors:
Depends on: xhr-timeout 735152 754941
Blocks:
  Show dependency treegraph
 
Reported: 2009-06-17 14:01 PDT by Mike Wilson
Modified: 2015-01-29 10:54 PST (History)
14 users (show)
jonas: wanted1.9.2+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (24.98 KB, patch)
2012-03-12 14:06 PDT, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
ajvincent: feedback+
Details | Diff | Splinter Review

Description Mike Wilson 2009-06-17 14:01:58 PDT
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
Comment 1 Olli Pettay [:smaug] (vacation Aug 25-28) 2009-06-17 23:20:07 PDT
Shouldn't the timeout be specified in XHR 2 spec?
Comment 2 Mike Wilson 2009-06-18 01:28:14 PDT
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.
Comment 3 Anne (:annevk) 2009-06-18 07:37:15 PDT
If anything, those emails indicate that the proposal is not clear on the details and that we have to figure those out.
Comment 4 William J. Edney 2010-03-03 19:51:57 PST
See also:

https://bugzilla.mozilla.org/show_bug.cgi?id=525816
Comment 5 Masatoshi Kimura [:emk] 2011-11-17 18:06:01 PST
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
Comment 6 Masatoshi Kimura [:emk] 2011-11-17 18:09:02 PST
>  xhr.open("GET", url, true);
By the way, this is an example of *async* XHR.
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-18 02:55:33 PST
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.
Comment 8 Masatoshi Kimura [:emk] 2011-11-19 09:13:52 PST
Unfortunately IE8+ already supports timeout for sync XHR ;(
Comment 9 Masatoshi Kimura [:emk] 2011-11-21 20:19:47 PST
The spec has been updated.
Comment 10 Masatoshi Kimura [:emk] 2011-11-21 20:21:51 PST
Ah, Workers are still allowed set timeout for sync XHR. Morphing.
Comment 11 Alex Vincent [:WeirdAl] 2012-02-14 17:44:32 PST
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...
Comment 12 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-03-12 14:06:02 PDT
Created attachment 605098 [details] [diff] [review]
Patch
Comment 13 Alex Vincent [:WeirdAl] 2012-03-12 23:04:40 PDT
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.
Comment 14 Alex Vincent [:WeirdAl] 2012-03-12 23:12:27 PDT
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.
Comment 15 Marco Bonardo [::mak] 2012-03-13 05:45:02 PDT
https://hg.mozilla.org/mozilla-central/rev/18ed43b863f1
Comment 16 Dominik Röttsches (drott) 2012-09-05 05:59:43 PDT
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.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-09-05 06:01:56 PDT
Alex, you wrote these tests originally.  Are you ok with public domain licensing of them?
Comment 18 Boris Zbarsky [:bz] 2012-09-05 06:10:08 PDT
Gerv, I thought our tests were generally public domain, except the ones we import from the W3C.  Is that not the case?
Comment 19 Gervase Markham [:gerv] 2012-09-05 07:06:58 PDT
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
Comment 20 Alex Vincent [:WeirdAl] 2012-09-05 07:10:47 PDT
(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.
Comment 21 Dominik Röttsches (drott) 2012-09-05 15:32:45 PDT
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.
Comment 22 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-09-05 18:13:10 PDT
Somebody want to add the appropriate license headers to mozilla-central?  Ms2ger?
Comment 23 Chris Mills (Mozilla, MDN editor) [:cmills] 2015-01-29 10:54:02 PST
covered on https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers

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