Shian-Yow Wu [:swu] Assignee
	Description • 10 years ago

+++ 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

	Shian-Yow Wu [:swu] Assignee
	Comment 1 • 10 years ago

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.

	Shian-Yow Wu [:swu] Assignee
	Comment 2 • 10 years ago

Fix the unexpected readystatechange event after abort() or new open().

	Shian-Yow Wu [:swu] Assignee
	Comment 3 • 10 years ago

Fixed the issue which unable to set responseType after new open().

	Shian-Yow Wu [:swu] Assignee
	Comment 4 • 10 years ago

The test case.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 5 • 10 years ago

khuey has agreed to be the reviewer here (thanks!).

	Shian-Yow Wu [:swu] Assignee
	Comment 6 • 10 years ago

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

	Shian-Yow Wu [:swu] Assignee
	Comment 7 • 10 years ago

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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 8 • 10 years ago

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.

	Shian-Yow Wu [:swu] Assignee
	Comment 9 • 10 years ago

Thank you Kyle!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b85f5ec21392
https://hg.mozilla.org/integration/mozilla-inbound/rev/355e895f7ae9
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ed776cbacd

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

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

	Ed Morley [:emorley]
	Comment 11 • 10 years ago

(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 :-)

	Ed Morley [:emorley]
	Comment 12 • 10 years ago

Relanded:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3c0151397b51
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5d3e9e9171bb
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94137dfc4520

	Ed Morley [:emorley]
	Comment 13 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3c0151397b51
https://hg.mozilla.org/mozilla-central/rev/5d3e9e9171bb
https://hg.mozilla.org/mozilla-central/rev/94137dfc4520