nkbugzilla Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:54.0) Gecko/20100101 Firefox/54.0
Build ID: 20170628075643

Steps to reproduce:

Changing the responseType property after readyState transitions to 2 throws an exception when done on a web worker. See the attached examples.


Actual results:

In the Windows version of Firefox 54, main.html works correctly while worker.html fails with an exception.


Expected results:

Setting responseType when readyState is <= 2 should not throw an exception when done on an XHR object on the "main thread" or from a web worker

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 4 • 7 years ago

If I understand the spec correctly, this issue is valid and we should move the status from unconfirmed to new.
Shawn, wdyt?

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 5 • 7 years ago

Yes, I think we're wrong here.

This worker code fallout in [1].
Based on the specification xhr 4.6.8 [2], "When set: throws an InvalidStateError exception if state is loading or done.".
It seems we shall check mState to equivalent LOADING or DONE as in [3]. I'm a bit surprise xhr worker wpt test doesn't catch this.

 
[1] http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/dom/xhr/XMLHttpRequestWorker.cpp#2381
[2] https://xhr.spec.whatwg.org/#the-responsetype-attribute
[3] http://searchfox.org/mozilla-central/rev/0f16d437cce97733c6678d29982a6bcad49f817b/dom/xhr/XMLHttpRequestMainThread.cpp#731

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 6 • 7 years ago

Original change in bug 1063538:
https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1063538&attachment=8525286

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 7 • 7 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Original change in bug 1063538:
> https://bugzilla.mozilla.org/page.cgi?id=splinter.
> html&bug=1063538&attachment=8525286

Part 1: Allow XHR worker to set overrideMimeType/responseType after an aborted send

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 8 • 7 years ago

Hi Shian-Yow,
I saw you added code for xhr worker three years before.

-  if (!mProxy || SendInProgress()) {
+  if (!mProxy || (SendInProgress() &&
+                  (mProxy->mSeenLoadStart ||
+                   mStateData.mReadyState > nsIXMLHttpRequest::OPENED))) {

It seems a specification contradict [xhr 4.8.6] in Comment 5 for setting responseType, but I'm afraid this is on purpose for some reason.
Can you explain why you want to send InvalidState if readyState is HEADER_RECEIVED?

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 9 • 7 years ago

I also wonder that why mProxy->mSeenLoadStart is true, we have to send InvalidStateError here.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 10 • 7 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #9)
> I also wonder that why mProxy->mSeenLoadStart is true, we have to send
> InvalidStateError here.

+  if (!mProxy || (SendInProgress() &&
+                  (mProxy->mSeenLoadStart ||
+                   mStateData.mReadyState > nsIXMLHttpRequest::OPENED))) {

https://bugzilla.mozilla.org/show_bug.cgi?id=1063538#c14
Based on Comment 14, "When the send is aborted and before a new send, the mSeenLoadStart must be to false, and mReadyState could only be UNSENT/OPENED, we can use it to determined an aborted send.", however this could cause code fallout InvalidStateError even if we check state is LOADING or DONE as specification defined.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 12 • 7 years ago

Running bug1063538_worker test, it seems no crash.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 13 • 7 years ago

Discussing with swu, the fix is simple, but I have to check carefully the original crash issue. So I will run more tests.

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #8)
> Hi Shian-Yow,
> I saw you added code for xhr worker three years before.
> 
> -  if (!mProxy || SendInProgress()) {
> +  if (!mProxy || (SendInProgress() &&
> +                  (mProxy->mSeenLoadStart ||
> +                   mStateData.mReadyState > nsIXMLHttpRequest::OPENED))) {
> 
> It seems a specification contradict [xhr 4.8.6] in Comment 5 for setting
> responseType, but I'm afraid this is on purpose for some reason.
> Can you explain why you want to send InvalidState if readyState is
> HEADER_RECEIVED?

The reason was mentioned in bug 1063538 comment 14, intended to fix the crash issue caused by bug 1014466.  Please go ahead make it better as needed in this bug, and make sure to pass the test case of those two bugs.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 15 • 7 years ago

(In reply to Shian-Yow Wu [:swu] (56 Regression Engineering support) from comment #14)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #8)
> > Hi Shian-Yow,
> > I saw you added code for xhr worker three years before.
> > 
> > -  if (!mProxy || SendInProgress()) {
> > +  if (!mProxy || (SendInProgress() &&
> > +                  (mProxy->mSeenLoadStart ||
> > +                   mStateData.mReadyState > nsIXMLHttpRequest::OPENED))) {
> > 
> > It seems a specification contradict [xhr 4.8.6] in Comment 5 for setting
> > responseType, but I'm afraid this is on purpose for some reason.
> > Can you explain why you want to send InvalidState if readyState is
> > HEADER_RECEIVED?
> 
> The reason was mentioned in bug 1063538 comment 14, intended to fix the
> crash issue caused by bug 1014466.  Please go ahead make it better as needed
> in this bug, and make sure to pass the test case of those two bugs.

Thanks for reminders :)

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 16 • 7 years ago

Well, it turns out test case "dom/xhr/tests/test_worker_xhr2.html" expects exception to be thrown. See bug 658178.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 17 • 7 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #5)
> Yes, I think we're wrong here.
> 
> This worker code fallout in [1].
> Based on the specification xhr 4.6.8 [2], "When set: throws an
> InvalidStateError exception if state is loading or done.".
> It seems we shall check mState to equivalent LOADING or DONE as in [3]. I'm
> a bit surprise xhr worker wpt test doesn't catch this.

Actually wpt test did check, but it doesn't assert not throw InvalidStateError.

http://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/testing/web-platform/tests/XMLHttpRequest/responsetype.html#35

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)
	Comment 18 • 7 years ago

It seems changing this behavior breaks our xhr tests on worker :( Need more time to check this bug.