Implement timeout for XHR in Worker context

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
8 years ago
2 years ago

People

(Reporter: Mike Wilson, Assigned: khuey)

Tracking

({dev-doc-complete})

unspecified
mozilla13
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 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
Component: General → DOM
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → general
Hardware: x86 → All
Flags: wanted1.9.2+

Comment 1

8 years ago
Shouldn't the timeout be specified in XHR 2 spec?
(Reporter)

Comment 2

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

Comment 3

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

Comment 4

7 years ago
See also:

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

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
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
>  xhr.open("GET", url, true);
By the way, this is an example of *async* XHR.

Comment 7

6 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.
Unfortunately IE8+ already supports timeout for sync XHR ;(
The spec has been updated.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
Ah, Workers are still allowed set timeout for sync XHR. Morphing.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Implement timeout for synchronous XHR → Implement timeout for synchronous XHR in Woeker context
Summary: Implement timeout for synchronous XHR in Woeker context → Implement timeout for synchronous XHR in Worker context
Depends on: 525816
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...

Updated

5 years ago
Summary: Implement timeout for synchronous XHR in Worker context → Implement timeout for XHR in Worker context
Created attachment 605098 [details] [diff] [review]
Patch
Assignee: nobody → khuey
Status: REOPENED → ASSIGNED
Attachment #605098 - Flags: review?(bent.mozilla)
Keywords: dev-doc-needed
Attachment #605098 - Flags: review?(jonas)
Attachment #605098 - Flags: review?(jonas) → review+
Depends on: 735152
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.
Attachment #605098 - Flags: feedback+
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.
https://hg.mozilla.org/mozilla-central/rev/18ed43b863f1
Target Milestone: --- → mozilla13

Updated

5 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Attachment #605098 - Flags: review?(bent.mozilla)

Updated

5 years ago
Depends on: 754941
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.
Alex, you wrote these tests originally.  Are you ok with public domain licensing of them?
Gerv, I thought our tests were generally public domain, except the ones we import from the W3C.  Is that not the case?
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
(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.
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.
Somebody want to add the appropriate license headers to mozilla-central?  Ms2ger?
covered on https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.