Closed Bug 1014466 Opened 6 years ago Closed 5 years ago

Unexpected response was received when reusing async XHR in worker.

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: swu, Assigned: swu)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch Test case for async XHR reuse. (obsolete) — Splinter Review
+++ This bug was initially created as a clone of Bug #1008126 +++

When reusing async XHR in worker, some issues occured and prevented us from receiving expected response.

In the following example, we'll get another response as soon as calling abort().  Removing abort() still triggers the unexpected response when calling new open().

var xhr = new XMLHttpRequest();
xhr.onreadystatechange = function() {
  if (xhr.readyState == xhr.DONE && xhr.status == 200) {
    // check response here
    ...
  }
};
xhr.open('GET', url1, true);
xhr.responseType = 'text';
xhr.send();
xhr.abort();
xhr.open('GET', url2, true);
xhr.responseType = 'text';
xhr.send();

In addition, set responseType after the 2nd open() caused exception InvalidStateError.

Below is the log using attached test case.

 0:08.35 ====== xhr.readyState=1
 0:08.35 ====== xhr.readyState=2
 0:08.35 ====== xhr.readyState=3
 0:08.35 ====== xhr.readyState=4
 0:08.35 ====== count=1, response=1234567890
 0:08.35 ====== xhr.readyState=4
 0:08.35 ====== count=2, response=1234567890
 0:08.36 TEST-PASS | unknown test url | Check data 1
 0:08.36 TEST-UNEXPECTED-FAIL | unknown test url | Check data 2
 0:08.36 ====== xhr.readyState=1
 0:08.36 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - : InvalidStateError: An attempt was made to use an object that is not, or is no longer, usable at http://mochi.test:8888/tests/dom/workers/test/abort_worker.js:51
Summary: Unexpected response was sent when reusing async XHR in worker. → Unexpected response was received when reusing async XHR in worker.
OK, the abort() problem occurs because when the first time readyState=DONE received, the loadend event has not arrived yet.  Calling abort() or new open() at this time would dispatch the readystatechange event again.  The unexpected response won't be received if we change to listen by xhr.onload instead of xhr.onreadystatechange to get response.  However, getting the readyState=DONE twice when listening by xhr.onreadystatechange should still be a problem.
Fix the unexpected readystatechange event after abort() or new open().
Assignee: nobody → swu
Status: NEW → ASSIGNED
Fixed the issue which unable to set responseType after new open().
Attachment #8429836 - Flags: review?(bent.mozilla)
Attachment #8429834 - Flags: review?(bent.mozilla)
The test case.
Attachment #8426896 - Attachment is obsolete: true
Attachment #8429837 - Flags: review?(bent.mozilla)
Attachment #8429834 - Flags: review?(bent.mozilla) → review?(khuey)
Attachment #8429836 - Flags: review?(bent.mozilla) → review?(khuey)
Attachment #8429837 - Flags: review?(bent.mozilla) → review?(khuey)
khuey has agreed to be the reviewer here (thanks!).
Comment on attachment 8429836 [details] [diff] [review]
Part 2: Allow setting responseType after new open().

Thank you Kyle.

Just found the patch hit a testing failure, so cancel review request for now.

524 INFO TEST-UNEXPECTED-FAIL | /tests/dom/workers/test/test_xhr2.html | uncaught exception - Error: Failed to throw when setting responseType after calling send() at http://mochi.test:8888/tests/dom/workers/test/xhr2_worker.js:172
Attachment #8429836 - Flags: review?(khuey)
Unpin the XHR after abort() or new open(), so that the responseType can be set.
Does it make sense?

I am not quite sure whether this could introduce a GC hazard.  But if it does, the first open() before send() should have the same concern?
Attachment #8429836 - Attachment is obsolete: true
Attachment #8430714 - Flags: feedback?(khuey)
Comment on attachment 8430714 [details] [diff] [review]
Part 2: Allow setting responseType after abort() or new open().

Review of attachment 8430714 [details] [diff] [review]:
-----------------------------------------------------------------

I'm always a little scared of touching this code because it was so hard to get right, but I think this is correct.
Attachment #8430714 - Flags: feedback?(khuey) → review+
sorry had to backout this changes for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=41182027&tree=Mozilla-Inbound on android 2.3
(In reply to Carsten Book [:Tomcat] from comment #10)
> sorry had to backout this changes for test failures like
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41182027&tree=Mozilla-
> Inbound on android 2.3

It turns out the test is broken & this landing just bumped it from one mochitest chunk to another causing it to fail (since that test must rely on previous state). The test will be disabled in bug 1021644, after which this can reland :-)
Depends on: 1063538
You need to log in before you can comment on or make changes to this bug.