Closed
Bug 701787
Opened 13 years ago
Closed 13 years ago
Investigate if synchronous XHR in window context should not support new XHR responseTypes
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: smaug, Assigned: emk)
References
(Depends on 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files, 17 obsolete files)
(deleted),
patch
|
emk
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Updated•13 years ago
|
Reporter | ||
Comment 3•13 years ago
|
||
Yes. Only the old types which can be read using responseText and responseXML should be, IMO, supported with sync XHR. One can access HTML document (text/html) from responseXML, but IMO that shouldn't be supported with sync XHR.
Assignee | ||
Comment 4•13 years ago
|
||
So, we could make .response async-only regardless of the .responseType in window context because legacy contents should be using .responseXML or .responseText.
Reporter | ||
Comment 6•13 years ago
|
||
http://dev.w3.org/cvsweb/2006/webapi/XMLHttpRequest-2/xhr-source.diff?r1=1.126;r2=1.127;f=h is probably good enough, and should be easy to implement. emk, want to take this bug?
Assignee | ||
Updated•13 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 9•13 years ago
|
||
Comment on attachment 575211 [details] [diff] [review] patch > NS_IMETHODIMP nsXMLHttpRequest::SetResponseType(const nsAString& aResponseType) > { >- // If the state is not OPENED or HEADERS_RECEIVED raise an >- // INVALID_STATE_ERR exception and terminate these steps. >- if (!(mState & (XML_HTTP_REQUEST_OPENED | XML_HTTP_REQUEST_SENT | >- XML_HTTP_REQUEST_HEADERS_RECEIVED))) >+ // If the state is LOADING or DONE, throw an "InvalidStateError" >+ // exception and terminate these steps. >+ if (mState & (XML_HTTP_REQUEST_LOADING | XML_HTTP_REQUEST_STOPPED | >+ XML_HTTP_REQUEST_DONE)) > return NS_ERROR_DOM_INVALID_STATE_ERR; Could you add {} for the if >+ // sync request is not allowed setting responseType >+ if (!(mState & (XML_HTTP_REQUEST_UNSENT | XML_HTTP_REQUEST_ASYNC))) { >+ return NS_ERROR_DOM_INVALID_ACCESS_ERR; >+ } Ah, worker doesn't actually ever create sync XHR, but does the sync part by itself. bent should review worker parts. >+++ b/toolkit/mozapps/extensions/content/extensions.js >@@ -2854,17 +2854,16 @@ var gDetailView = { > } > return text; > } > > var rows = document.getElementById("detail-downloads").parentNode; > > var xhr = new XMLHttpRequest(); > xhr.open("GET", this._addon.optionsURL, false); Grr... please file a followup bug. Someone should change this to not use sync XHR.
Attachment #575211 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 10•13 years ago
|
||
The Workers part was separated from this patch.
> Could you add {} for the if
Fixed.
Attachment #575211 -
Attachment is obsolete: true
Attachment #575425 -
Flags: review+
Assignee | ||
Comment 12•13 years ago
|
||
Using loadend events instead of silly while() loops.
Attachment #575212 -
Attachment is obsolete: true
Attachment #575212 -
Flags: review?(bugs)
Attachment #575427 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #575425 -
Attachment description: Bug 701787 - Part 1: disallow responseType and withCredentials for sync XHR; r=smaug → Part 1: disallow responseType and withCredentials for sync XHR; r=smaug
Assignee | ||
Updated•13 years ago
|
Attachment #575427 -
Flags: review? → review?(bugs)
Reporter | ||
Comment 13•13 years ago
|
||
Comment on attachment 575427 [details] [diff] [review] Part 3: Tests fix Please run the patch and test on tryserver before landing.
Attachment #575427 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 15•13 years ago
|
||
Added warnings to Error Console. Authors may wonder why .responseType and .withCredentials suddenly start to fail. Requesting review again because of actual code change.
Attachment #575425 -
Attachment is obsolete: true
Attachment #576405 -
Flags: review?(bugs)
Assignee | ||
Comment 16•13 years ago
|
||
- Added a .responseType validation before open(). - replace empty .responseType with "text" to suppress useless document parsing.
Attachment #575426 -
Attachment is obsolete: true
Attachment #575426 -
Flags: review?(bent.mozilla)
Attachment #576406 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 17•13 years ago
|
||
Sorry, the file was a bit old.
Attachment #576406 -
Attachment is obsolete: true
Attachment #576406 -
Flags: review?(bent.mozilla)
Attachment #576407 -
Flags: review?
Assignee | ||
Comment 18•13 years ago
|
||
Separated dom/workers test.
Attachment #575427 -
Attachment is obsolete: true
Attachment #576408 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Attachment #576407 -
Flags: review? → review?(bent.mozilla)
Assignee | ||
Comment 19•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #9) > >+++ b/toolkit/mozapps/extensions/content/extensions.js > >@@ -2854,17 +2854,16 @@ var gDetailView = { > > } > > return text; > > } > > > > var rows = document.getElementById("detail-downloads").parentNode; > > > > var xhr = new XMLHttpRequest(); > > xhr.open("GET", this._addon.optionsURL, false); > Grr... please file a followup bug. Someone should change this to not use > sync XHR. Filed bug 704751.
Assignee | ||
Comment 20•13 years ago
|
||
Updated warning messages a bit.
Attachment #576405 -
Attachment is obsolete: true
Attachment #576405 -
Flags: review?(bugs)
Attachment #576410 -
Flags: review?(bugs)
Reporter | ||
Comment 21•13 years ago
|
||
Comment on attachment 576410 [details] [diff] [review] Part 1: disallow responseType and withCredentials for sync XHR ># HG changeset patch ># Parent 4284f862270d1439a05f5c09f0ee764e82014f13 ># User Masatoshi Kimura <VYV03354@nifty.ne.jp> >Bug 701787 - Part 1: disallow responseType and withCredentials for sync XHR > >diff --git a/content/base/src/nsXMLHttpRequest.cpp b/content/base/src/nsXMLHttpRequest.cpp >--- a/content/base/src/nsXMLHttpRequest.cpp >+++ b/content/base/src/nsXMLHttpRequest.cpp >@@ -707,57 +707,54 @@ NS_IMETHODIMP > nsXMLHttpRequest::GetChannel(nsIChannel **aChannel) > { > NS_ENSURE_ARG_POINTER(aChannel); > NS_IF_ADDREF(*aChannel = mChannel); > > return NS_OK; > } > >+static void LogMessage(const char* aWarning, nsPIDOMWindow* aWindow) >+{ >+ nsCOMPtr<nsIDocument> doc = >+ do_QueryInterface(aWindow->GetExtantDocument()); >+ nsContentUtils::ReportToConsole(nsContentUtils::eDOM_PROPERTIES, >+ aWarning, >+ nsnull, >+ 0, >+ nsnull, // Response URL not kept around >+ EmptyString(), >+ 0, >+ 0, >+ nsIScriptError::warningFlag, >+ "DOM", >+ doc); >+} Why doc as the last parameter? nsContentUtils will get anyway window from it and the windowID. So, please pass windowID. But actually, please check that aWindow is not null. > // Disallow HTTP/1.1 TRACE method (see bug 302489) > // and MS IIS equivalent TRACK (see bug 381264) >- if (method.LowerCaseEqualsLiteral("trace") || >+ if (method.LowerCaseEqualsLiteral("connect") || >+ method.LowerCaseEqualsLiteral("trace") || > method.LowerCaseEqualsLiteral("track")) { >- return NS_ERROR_INVALID_ARG; >+ return NS_ERROR_DOM_SECURITY_ERR; >+ } What is this change. Looks like nothing to do with this bug. >+ >+ // sync request is not allowed using withCredential or responseType >+ if (!async && (mState & XML_HTTP_REQUEST_AC_WITH_CREDENTIALS || >+ mResponseType != XML_HTTP_RESPONSE_TYPE_DEFAULT)) { >+ if (mState & XML_HTTP_REQUEST_AC_WITH_CREDENTIALS) >+ LogMessage("WithCredentialsSyncXHRWarning", mOwner); >+ if (mResponseType != XML_HTTP_RESPONSE_TYPE_DEFAULT) >+ LogMessage("ResponseTypeSyncXHRWarning", mOwner); >+ return NS_ERROR_DOM_INVALID_ACCESS_ERR; > } if (expr) { stmt; }
Attachment #576410 -
Flags: review?(bugs) → review-
Assignee | ||
Comment 22•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #21) > Why doc as the last parameter? nsContentUtils will get anyway window from it > and the windowID. Because the URL was not contained in the warning message if I passed only the windowID. nsContentUtils will get not only the windowID but also the URI from the doc. https://hg.mozilla.org/mozilla-central/annotate/3c8147998124/content/base/src/nsContentUtils.cpp#l2839 > What is this change. Looks like nothing to do with this bug. Implementing XHR2 "The open() method" clause Step 4. http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-open-method But yes, it is unrelated to this bug. I'll file it as a separate bug.
Reporter | ||
Comment 23•13 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #22) > (In reply to Olli Pettay [:smaug] from comment #21) > > Why doc as the last parameter? nsContentUtils will get anyway window from it > > and the windowID. > Because the URL was not contained in the warning message if I passed only > the windowID. nsContentUtils will get not only the windowID but also the URI > from the doc. > https://hg.mozilla.org/mozilla-central/annotate/3c8147998124/content/base/ > src/nsContentUtils.cpp#l2839 Ah, ok. But please null check window anyway. > But yes, it is unrelated to this bug. I'll file it as a separate bug. please.
Assignee | ||
Comment 24•13 years ago
|
||
Resolved review comments
Attachment #576410 -
Attachment is obsolete: true
Attachment #576437 -
Flags: review?(bugs)
Assignee | ||
Comment 25•13 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #23) > > But yes, it is unrelated to this bug. I'll file it as a separate bug. > please. Filed bug 704787.
Reporter | ||
Updated•13 years ago
|
Attachment #576437 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 26•13 years ago
|
||
> +ResponseTypeSyncXHRWarning=Use of XMLHttpRequest's responseType attribute is no longer supported in the synchronous mode and window context. Use the asynchronous mode or Workers instead.
Is this English OK? I consider changing this to:
ResponseTypeSyncXHRWarning=Use of XMLHttpRequest's responseType attribute is no longer supported in the synchronous mode in window context. Use the asynchronous mode or Workers instead.
Because the synchronous mode is still supported in Workers.
Reporter | ||
Comment 27•13 years ago
|
||
(In reply to Masatoshi Kimura [:emk] from comment #26) > > +ResponseTypeSyncXHRWarning=Use of XMLHttpRequest's responseType attribute is no longer supported in the synchronous mode and window context. Use the asynchronous mode or Workers instead. > Is this English OK? I consider changing this to: > ResponseTypeSyncXHRWarning=Use of XMLHttpRequest's responseType attribute is > no longer supported in the synchronous mode in window context. Use the > asynchronous mode or Workers instead. > Because the synchronous mode is still supported in Workers. IMO you could drop the last sentence.
Assignee | ||
Comment 28•13 years ago
|
||
I'll fold this patch to part 1 after passed the review.
Attachment #576737 -
Flags: review?(bugs)
(In reply to Masatoshi Kimura [:emk] from comment #26) > Because the synchronous mode is still supported in Workers. Note that parsing documents isn't supported in Workers.
Assignee | ||
Comment 30•13 years ago
|
||
(In reply to Henri Sivonen (:hsivonen) from comment #29) > (In reply to Masatoshi Kimura [:emk] from comment #26) > > Because the synchronous mode is still supported in Workers. > Note that parsing documents isn't supported in Workers. Oh, good point. I'll update the patch accordingly.
Assignee | ||
Comment 31•13 years ago
|
||
Attachment #576737 -
Attachment is obsolete: true
Attachment #576737 -
Flags: review?(bugs)
Attachment #576757 -
Flags: review?(bugs)
Reporter | ||
Updated•13 years ago
|
Attachment #576757 -
Flags: review?(bugs) → review+
Should this be treated as checkin-needed now?
(In reply to Henri Sivonen (:hsivonen) from comment #32) > Should this be treated as checkin-needed now? Nevermind. I didn't notice that the worker part was still waiting for review.
Assignee | ||
Comment 35•13 years ago
|
||
- Folding the message change. - Separated anothor unrelated part (allow setting .responseType/.withCredentials before send()). - Added mOwner check to avoid overblocking in non-window non-Worker context.
Attachment #576437 -
Attachment is obsolete: true
Attachment #576757 -
Attachment is obsolete: true
Attachment #578885 -
Flags: review?(bugs)
Assignee | ||
Comment 36•13 years ago
|
||
Added an xpcshell-test for non-window non-Worker context.
Attachment #576408 -
Attachment is obsolete: true
Attachment #578886 -
Flags: review?(bugs)
Assignee | ||
Comment 37•13 years ago
|
||
Comment on attachment 576407 [details] [diff] [review] Part 2: Worker part This part is no longer required.
Attachment #576407 -
Attachment is obsolete: true
Attachment #576407 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 39•13 years ago
|
||
Sorry, forgot to remove test cases belonging to bug 707484.
Attachment #578886 -
Attachment is obsolete: true
Attachment #578886 -
Flags: review?(bugs)
Attachment #578887 -
Flags: review?(bugs)
Reporter | ||
Updated•13 years ago
|
Attachment #578885 -
Flags: review?(bugs) → review+
Reporter | ||
Updated•13 years ago
|
Attachment #578887 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 40•13 years ago
|
||
Unbitrotted.
Attachment #578885 -
Attachment is obsolete: true
Attachment #579648 -
Flags: review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f72d14b6b5e https://hg.mozilla.org/integration/mozilla-inbound/rev/b9935b5b57d1 Thank you for fixing this!
Keywords: checkin-needed
...and backed out due to sync withCredential test code added to content/base/test/test_bug338583.html by bug 664179 going orange. https://hg.mozilla.org/integration/mozilla-inbound/rev/2b11310f327c
Assignee | ||
Comment 43•13 years ago
|
||
Added a test fix for bug 664179. Try result: https://tbpl.mozilla.org/?tree=Try&rev=4bc9ccac0aee
Attachment #578887 -
Attachment is obsolete: true
Attachment #579842 -
Flags: review?(bugs)
Assignee | ||
Comment 44•13 years ago
|
||
s/onload/onloadend/
Attachment #579842 -
Attachment is obsolete: true
Attachment #579842 -
Flags: review?(bugs)
Attachment #579846 -
Flags: review?(bugs)
Reporter | ||
Updated•13 years ago
|
Attachment #579846 -
Flags: review?(bugs) → review+
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/e21ee958e5fd https://hg.mozilla.org/integration/mozilla-inbound/rev/c175352a397b
Keywords: checkin-needed
Comment 46•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e21ee958e5fd https://hg.mozilla.org/mozilla-central/rev/c175352a397b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Blocks: 712621
Comment 47•13 years ago
|
||
This appears to break Mandreel WebGL-based apps: I see the following error when loading chrome.monsterdashgame.com, which used to work in Firefox: Error: uncaught exception: [Exception... "A parameter or an operation is not supported by the underlying object" code: "15" nsresult: "0x8053000f (NS_ERROR_DOM_INVALID_ACCESS_ERR)" location: "http://chrome.monsterdashgame.com/ZombieDash/mandreel.js Line: 1"] Mandreel provides an emulation layer for binaries. As such, it needs to emmulate fopen. They appear to do so using synchronous XHR with responseType assigned to "ArrayBuffer". They point the XHR at a local resource to provide decent performance it looks like.
Reporter | ||
Comment 48•13 years ago
|
||
Sounds like "Mandreel WebGL-based apps" are doing then something wrong then.
Comment 49•13 years ago
|
||
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=736340. Seems wrong to disallow setting withCredentials to false.
Comment 50•13 years ago
|
||
Documentation updated: https://developer.mozilla.org/en/XMLHttpRequest#Properties Listed on Firefox 11 for developers.
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•