Open
Bug 1387075
Opened 7 years ago
Updated 2 years ago
Setting responseType on an XMLHttpRequest object when readyState is 2 from a web worker throws an InvalidStateError
Categories
(Core :: DOM: Workers, defect, P2)
Tracking
()
UNCONFIRMED
People
(Reporter: nkbugzilla, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(5 files)
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
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Comment 3•7 years ago
|
||
Updated•7 years ago
|
Component: Untriaged → DOM: Workers
Product: Firefox → Core
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?
Flags: needinfo?(shuang)
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
Flags: needinfo?(shuang)
Assignee: nobody → shuang
Original change in bug 1063538: https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=1063538&attachment=8525286
(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
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?
Flags: needinfo?(swu)
I also wonder that why mProxy->mSeenLoadStart is true, we have to send InvalidStateError here.
(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.
See Also: → 1063538
Running bug1063538_worker test, it seems no crash.
Discussing with swu, the fix is simple, but I have to check carefully the original crash issue. So I will run more tests.
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.
Flags: needinfo?(swu)
(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 :)
Well, it turns out test case "dom/xhr/tests/test_worker_xhr2.html" expects exception to be thrown. See bug 658178.
(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
It seems changing this behavior breaks our xhr tests on worker :( Need more time to check this bug.
Updated•7 years ago
|
Priority: -- → P2
Assignee: shuang → nobody
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•