Closed Bug 701787 Opened 8 years ago Closed 8 years ago

Investigate if synchronous XHR in window context should not support new XHR responseTypes

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
All
defect
Not set

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)

8.36 KB, patch
emk
: review+
Details | Diff | Splinter Review
21.99 KB, patch
smaug
: review+
Details | Diff | Splinter Review
Do new XHR responseTypes include "arraybuffer" and "blob"?
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.
So, we could make .response async-only regardless of the .responseType in window context because legacy contents should be using .responseXML or .responseText.
yes.
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?
Attached patch patch (obsolete) — Splinter Review
Taking.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #575211 - Flags: review?(bugs)
Attached patch tests fix (obsolete) — Splinter Review
Attachment #575212 - Flags: review?(bugs)
Keywords: dev-doc-needed
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+
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+
Attached patch Part 2: Worker part (obsolete) — Splinter Review
Attachment #575426 - Flags: review?(bent.mozilla)
Attached patch Part 3: Tests fix (obsolete) — Splinter Review
Using loadend events instead of silly while() loops.
Attachment #575212 - Attachment is obsolete: true
Attachment #575212 - Flags: review?(bugs)
Attachment #575427 - Flags: review?
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
Attachment #575427 - Flags: review? → review?(bugs)
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+
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)
Attached patch Part 2: Worker part (obsolete) — Splinter Review
- 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)
Attached patch Part 2: Worker part (obsolete) — Splinter Review
Sorry, the file was a bit old.
Attachment #576406 - Attachment is obsolete: true
Attachment #576406 - Flags: review?(bent.mozilla)
Attachment #576407 - Flags: review?
Attached patch Tests fix, r=smaug (obsolete) — Splinter Review
Separated dom/workers test.
Attachment #575427 - Attachment is obsolete: true
Attachment #576408 - Flags: review+
Attachment #576407 - Flags: review? → review?(bent.mozilla)
Depends on: 704751
(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.
Updated warning messages a bit.
Attachment #576405 - Attachment is obsolete: true
Attachment #576405 - Flags: review?(bugs)
Attachment #576410 - Flags: review?(bugs)
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-
(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.
(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.
Resolved review comments
Attachment #576410 - Attachment is obsolete: true
Attachment #576437 - Flags: review?(bugs)
(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.
Attachment #576437 - Flags: review?(bugs) → review+
Blocks: 704820
> +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.
(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.
Attached patch message change (obsolete) — Splinter Review
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.
(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.
Attached patch message change, v2 (obsolete) — Splinter Review
Attachment #576737 - Attachment is obsolete: true
Attachment #576737 - Flags: review?(bugs)
Attachment #576757 - Flags: review?(bugs)
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.
Bent, ping?
- 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)
Attached patch Part 2: Tests (obsolete) — Splinter Review
Added an xpcshell-test for non-window non-Worker context.
Attachment #576408 - Attachment is obsolete: true
Attachment #578886 - Flags: review?(bugs)
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)
Filed bug 707484 for separated part.
Attached patch Part 2: Tests (obsolete) — Splinter Review
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)
Attachment #578885 - Flags: review?(bugs) → review+
Attachment #578887 - Flags: review?(bugs) → review+
Unbitrotted.
Attachment #578885 - Attachment is obsolete: true
Attachment #579648 - Flags: review+
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
Attached patch Part 2: Tests (obsolete) — Splinter Review
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)
Attached patch Part 2: TestsSplinter Review
s/onload/onloadend/
Attachment #579842 - Attachment is obsolete: true
Attachment #579842 - Flags: review?(bugs)
Attachment #579846 - Flags: review?(bugs)
Attachment #579846 - Flags: review?(bugs) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e21ee958e5fd
https://hg.mozilla.org/mozilla-central/rev/c175352a397b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 713473
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.
Depends on: 716765
Sounds like "Mandreel WebGL-based apps" are doing then something wrong then.
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=736340. Seems wrong to disallow setting withCredentials to false.
Documentation updated:

https://developer.mozilla.org/en/XMLHttpRequest#Properties

Listed on Firefox 11 for developers.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.