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

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

See also https://bugs.webkit.org/show_bug.cgi?id=72154

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

http://www.w3.org/Bugs/Public/show_bug.cgi?id=14773

Comment 2

[Masatoshi Kimura [:emk]]

Do new XHR responseTypes include "arraybuffer" and "blob"?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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]]

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][[e-mail]]]

yes.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

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]]

Attachment: patch

Taking.

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: tests fix

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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]]

Attachment: 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]]

Attachment: Part 2: Worker part

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: Part 3: Tests fix

Using loadend events instead of silly while() loops.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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]]

https://tbpl.mozilla.org/?tree=Try&rev=b46057874895

Comment 15

[Masatoshi Kimura [:emk]]

Attachment: 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]]

Attachment: 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]]

Attachment: Part 2: Worker part

Sorry, the file was a bit old.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Tests fix, r=smaug

Separated dom/workers test.

Comment 19

[Masatoshi Kimura [:emk]]

(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]]

Attachment: Part 1: disallow responseType and withCredentials for sync XHR

Updated warning messages a bit.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 576410 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

># HG changeset patch
># Parent 4284f862270d1439a05f5c09f0ee764e82014f13
># User Masatoshi Kimura <[e-mail]>
>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]]

(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][[e-mail]]]

(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]]

Attachment: Part 1: disallow responseType and withCredentials for sync XHR

Resolved review comments

Comment 25

[Masatoshi Kimura [:emk]]

(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]]

> +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][[e-mail]]]

(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]]

Attachment: message change

I'll fold this patch to part 1 after passed the review.

Comment 29

[Henri Sivonen (:hsivonen)]

(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]]

(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]]

Attachment: message change, v2

Comment 32

[Henri Sivonen (:hsivonen)]

Should this be treated as checkin-needed now?

Comment 33

[Henri Sivonen (:hsivonen)]

(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]]

Bent, ping?

Comment 35

[Masatoshi Kimura [:emk]]

Attachment: 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]]

Attachment: Part 2: Tests

Added an xpcshell-test for non-window non-Worker context.

Comment 37

[Masatoshi Kimura [:emk]]

Comment on attachment 576407 [details] [diff] [review]
Part 2: Worker part

This part is no longer required.

Comment 38

[Masatoshi Kimura [:emk]]

Filed bug 707484 for separated part.

Comment 39

[Masatoshi Kimura [:emk]]

Attachment: Part 2: Tests

Sorry, forgot to remove test cases belonging to bug 707484.

Comment 40

[Masatoshi Kimura [:emk]]

Attachment: Part 1: disallow responseType and withCredentials for sync XHR. r=smaug

Unbitrotted.

Comment 41

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f72d14b6b5e
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9935b5b57d1

Thank you for fixing this!

Comment 42

[Henri Sivonen (:hsivonen)]

...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]]

Attachment: 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]]

Attachment: Part 2: Tests

s/onload/onloadend/

Comment 45

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e21ee958e5fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/c175352a397b

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e21ee958e5fd
https://hg.mozilla.org/mozilla-central/rev/c175352a397b

Comment 47

[Darin Fisher]

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][[e-mail]]]

Sounds like "Mandreel WebGL-based apps" are doing then something wrong then.

Comment 49

[hugh winkler]

Please see https://bugzilla.mozilla.org/show_bug.cgi?id=736340. Seems wrong to disallow setting withCredentials to false.

Comment 50

[Eric Shepherd [:sheppy]]

Documentation updated:

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

Listed on Firefox 11 for developers.