Last Comment Bug 701787 - Investigate if synchronous XHR in window context should not support new XHR responseTypes
: Investigate if synchronous XHR in window context should not support new XHR r...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 All
: -- normal (vote)
: mozilla11
Assigned To: Masatoshi Kimura [:emk]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 716765 704751 713473
Blocks: 704820 712621
  Show dependency treegraph
 
Reported: 2011-11-11 11:01 PST by Olli Pettay [:smaug]
Modified: 2012-05-07 08:43 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (10.87 KB, patch)
2011-11-17 09:29 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review
tests fix (13.31 KB, patch)
2011-11-17 09:30 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 1: disallow responseType and withCredentials for sync XHR; r=smaug (5.08 KB, patch)
2011-11-18 04:57 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 2: Worker part (5.94 KB, patch)
2011-11-18 04:58 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 3: Tests fix (12.94 KB, patch)
2011-11-18 05:00 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review
Part 1: disallow responseType and withCredentials for sync XHR (10.01 KB, patch)
2011-11-22 21:46 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Worker part (7.36 KB, patch)
2011-11-22 21:48 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Worker part (7.32 KB, patch)
2011-11-22 21:50 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Tests fix, r=smaug (12.16 KB, patch)
2011-11-22 21:52 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 1: disallow responseType and withCredentials for sync XHR (10.15 KB, patch)
2011-11-22 22:11 PST, Masatoshi Kimura [:emk]
bugs: review-
Details | Diff | Splinter Review
Part 1: disallow responseType and withCredentials for sync XHR (9.97 KB, patch)
2011-11-23 03:14 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review
message change (2.26 KB, patch)
2011-11-24 05:37 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
message change, v2 (2.24 KB, patch)
2011-11-24 06:47 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review
Part 1: disallow responseType and withCredentials for sync XHR (8.34 KB, patch)
2011-12-03 22:05 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review
Part 2: Tests (13.37 KB, patch)
2011-12-03 22:06 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Tests (13.09 KB, patch)
2011-12-03 22:19 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review
Part 1: disallow responseType and withCredentials for sync XHR. r=smaug (8.36 KB, patch)
2011-12-07 03:22 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review
Part 2: Tests (21.98 KB, patch)
2011-12-07 14:41 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Tests (21.99 KB, patch)
2011-12-07 14:47 PST, Masatoshi Kimura [:emk]
bugs: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2011-11-11 11:01:56 PST
See also https://bugs.webkit.org/show_bug.cgi?id=72154
Comment 1 Olli Pettay [:smaug] 2011-11-11 11:04:54 PST
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14773
Comment 2 Masatoshi Kimura [:emk] 2011-11-14 02:04:56 PST
Do new XHR responseTypes include "arraybuffer" and "blob"?
Comment 3 Olli Pettay [:smaug] 2011-11-14 02:07:52 PST
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.
Comment 4 Masatoshi Kimura [:emk] 2011-11-14 02:17:46 PST
So, we could make .response async-only regardless of the .responseType in window context because legacy contents should be using .responseXML or .responseText.
Comment 5 Olli Pettay [:smaug] 2011-11-14 02:20:34 PST
yes.
Comment 6 Olli Pettay [:smaug] 2011-11-16 09:42:11 PST
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?
Comment 7 Masatoshi Kimura [:emk] 2011-11-17 09:29:39 PST
Created attachment 575211 [details] [diff] [review]
patch

Taking.
Comment 8 Masatoshi Kimura [:emk] 2011-11-17 09:30:12 PST
Created attachment 575212 [details] [diff] [review]
tests fix
Comment 9 Olli Pettay [:smaug] 2011-11-18 03:20:50 PST
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.
Comment 10 Masatoshi Kimura [:emk] 2011-11-18 04:57:11 PST
Created attachment 575425 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR; r=smaug

The Workers part was separated from this patch.
> Could you add {} for the if
Fixed.
Comment 11 Masatoshi Kimura [:emk] 2011-11-18 04:58:52 PST
Created attachment 575426 [details] [diff] [review]
Part 2: Worker part
Comment 12 Masatoshi Kimura [:emk] 2011-11-18 05:00:29 PST
Created attachment 575427 [details] [diff] [review]
Part 3: Tests fix

Using loadend events instead of silly while() loops.
Comment 13 Olli Pettay [:smaug] 2011-11-18 09:46:15 PST
Comment on attachment 575427 [details] [diff] [review]
Part 3: Tests fix

Please run the patch and test on tryserver before landing.
Comment 14 Masatoshi Kimura [:emk] 2011-11-19 07:23:35 PST
https://tbpl.mozilla.org/?tree=Try&rev=b46057874895
Comment 15 Masatoshi Kimura [:emk] 2011-11-22 21:46:24 PST
Created attachment 576405 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

Added warnings to Error Console. Authors may wonder why .responseType and .withCredentials suddenly start to fail.
Requesting review again because of actual code change.
Comment 16 Masatoshi Kimura [:emk] 2011-11-22 21:48:18 PST
Created attachment 576406 [details] [diff] [review]
Part 2: Worker part

- Added a .responseType validation before open().
- replace empty .responseType with "text" to suppress useless document parsing.
Comment 17 Masatoshi Kimura [:emk] 2011-11-22 21:50:34 PST
Created attachment 576407 [details] [diff] [review]
Part 2: Worker part

Sorry, the file was a bit old.
Comment 18 Masatoshi Kimura [:emk] 2011-11-22 21:52:12 PST
Created attachment 576408 [details] [diff] [review]
Tests fix, r=smaug

Separated dom/workers test.
Comment 19 Masatoshi Kimura [:emk] 2011-11-22 21:59:31 PST
(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.
Comment 20 Masatoshi Kimura [:emk] 2011-11-22 22:11:35 PST
Created attachment 576410 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

Updated warning messages a bit.
Comment 21 Olli Pettay [:smaug] 2011-11-23 01:42:16 PST
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;
}
Comment 22 Masatoshi Kimura [:emk] 2011-11-23 01:53:49 PST
(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.
Comment 23 Olli Pettay [:smaug] 2011-11-23 02:01:29 PST
(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.
Comment 24 Masatoshi Kimura [:emk] 2011-11-23 03:14:49 PST
Created attachment 576437 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

Resolved review comments
Comment 25 Masatoshi Kimura [:emk] 2011-11-23 03:20:43 PST
(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.
Comment 26 Masatoshi Kimura [:emk] 2011-11-23 17:13:29 PST
> +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.
Comment 27 Olli Pettay [:smaug] 2011-11-24 03:25:15 PST
(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.
Comment 28 Masatoshi Kimura [:emk] 2011-11-24 05:37:03 PST
Created attachment 576737 [details] [diff] [review]
message change

I'll fold this patch to part 1 after passed the review.
Comment 29 Henri Sivonen (:hsivonen) 2011-11-24 06:39:15 PST
(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.
Comment 30 Masatoshi Kimura [:emk] 2011-11-24 06:46:31 PST
(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.
Comment 31 Masatoshi Kimura [:emk] 2011-11-24 06:47:10 PST
Created attachment 576757 [details] [diff] [review]
message change, v2
Comment 32 Henri Sivonen (:hsivonen) 2011-11-25 03:28:30 PST
Should this be treated as checkin-needed now?
Comment 33 Henri Sivonen (:hsivonen) 2011-11-25 03:29:04 PST
(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.
Comment 34 Masatoshi Kimura [:emk] 2011-11-30 22:38:16 PST
Bent, ping?
Comment 35 Masatoshi Kimura [:emk] 2011-12-03 22:05:05 PST
Created attachment 578885 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

- 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.
Comment 36 Masatoshi Kimura [:emk] 2011-12-03 22:06:00 PST
Created attachment 578886 [details] [diff] [review]
Part 2: Tests

Added an xpcshell-test for non-window non-Worker context.
Comment 37 Masatoshi Kimura [:emk] 2011-12-03 22:06:49 PST
Comment on attachment 576407 [details] [diff] [review]
Part 2: Worker part

This part is no longer required.
Comment 38 Masatoshi Kimura [:emk] 2011-12-03 22:10:42 PST
Filed bug 707484 for separated part.
Comment 39 Masatoshi Kimura [:emk] 2011-12-03 22:19:45 PST
Created attachment 578887 [details] [diff] [review]
Part 2: Tests

Sorry, forgot to remove test cases belonging to bug 707484.
Comment 40 Masatoshi Kimura [:emk] 2011-12-07 03:22:53 PST
Created attachment 579648 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR. r=smaug

Unbitrotted.
Comment 42 Henri Sivonen (:hsivonen) 2011-12-07 05:14:40 PST
...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
Comment 43 Masatoshi Kimura [:emk] 2011-12-07 14:41:34 PST
Created attachment 579842 [details] [diff] [review]
Part 2: Tests

Added a test fix for bug 664179.
Try result:
https://tbpl.mozilla.org/?tree=Try&rev=4bc9ccac0aee
Comment 44 Masatoshi Kimura [:emk] 2011-12-07 14:47:08 PST
Created attachment 579846 [details] [diff] [review]
Part 2: Tests

s/onload/onloadend/
Comment 47 Darin Fisher 2012-01-09 15:09:25 PST
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.
Comment 48 Olli Pettay [:smaug] 2012-01-10 02:06:33 PST
Sounds like "Mandreel WebGL-based apps" are doing then something wrong then.
Comment 49 hugh winkler 2012-03-15 19:16:13 PDT
Please see https://bugzilla.mozilla.org/show_bug.cgi?id=736340. Seems wrong to disallow setting withCredentials to false.
Comment 50 Eric Shepherd [:sheppy] 2012-05-07 08:43:25 PDT
Documentation updated:

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

Listed on Firefox 11 for developers.

Note You need to log in before you can comment on or make changes to this bug.