Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla11

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: smaug, Assigned: emk)

Tracking

(Depends on: 1 bug, {dev-doc-complete})

unspecified
mozilla11
x86_64
All
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 17 obsolete attachments)

8.36 KB, patch
emk
: review+
Details | Diff | Splinter Review
21.99 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
See also https://bugs.webkit.org/show_bug.cgi?id=72154
(Reporter)

Comment 1

6 years ago
http://www.w3.org/Bugs/Public/show_bug.cgi?id=14773
(Assignee)

Updated

6 years ago
(Assignee)

Comment 2

6 years ago
Do new XHR responseTypes include "arraybuffer" and "blob"?
(Reporter)

Comment 3

6 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

6 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 5

6 years ago
yes.
(Reporter)

Comment 6

6 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)

Comment 7

6 years ago
Created attachment 575211 [details] [diff] [review]
patch

Taking.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #575211 - Flags: review?(bugs)
(Assignee)

Comment 8

6 years ago
Created attachment 575212 [details] [diff] [review]
tests fix
Attachment #575212 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Keywords: dev-doc-needed
(Reporter)

Comment 9

6 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

6 years ago
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.
Attachment #575211 - Attachment is obsolete: true
Attachment #575425 - Flags: review+
(Assignee)

Comment 11

6 years ago
Created attachment 575426 [details] [diff] [review]
Part 2: Worker part
Attachment #575426 - Flags: review?(bent.mozilla)
(Assignee)

Comment 12

6 years ago
Created attachment 575427 [details] [diff] [review]
Part 3: Tests fix

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

6 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

6 years ago
Attachment #575427 - Flags: review? → review?(bugs)
(Reporter)

Comment 13

6 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 14

6 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b46057874895
(Assignee)

Comment 15

6 years ago
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.
Attachment #575425 - Attachment is obsolete: true
Attachment #576405 - Flags: review?(bugs)
(Assignee)

Comment 16

6 years ago
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.
Attachment #575426 - Attachment is obsolete: true
Attachment #575426 - Flags: review?(bent.mozilla)
Attachment #576406 - Flags: review?(bent.mozilla)
(Assignee)

Comment 17

6 years ago
Created attachment 576407 [details] [diff] [review]
Part 2: Worker part

Sorry, the file was a bit old.
Attachment #576406 - Attachment is obsolete: true
Attachment #576407 - Flags: review?
Attachment #576406 - Flags: review?(bent.mozilla)
(Assignee)

Comment 18

6 years ago
Created attachment 576408 [details] [diff] [review]
Tests fix, r=smaug

Separated dom/workers test.
Attachment #575427 - Attachment is obsolete: true
Attachment #576408 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #576407 - Flags: review? → review?(bent.mozilla)
(Assignee)

Updated

6 years ago
Depends on: 704751
(Assignee)

Comment 19

6 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

6 years ago
Created attachment 576410 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

Updated warning messages a bit.
Attachment #576405 - Attachment is obsolete: true
Attachment #576405 - Flags: review?(bugs)
Attachment #576410 - Flags: review?(bugs)
(Reporter)

Comment 21

6 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

6 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

6 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

6 years ago
Created attachment 576437 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR

Resolved review comments
Attachment #576410 - Attachment is obsolete: true
Attachment #576437 - Flags: review?(bugs)
(Assignee)

Comment 25

6 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

6 years ago
Attachment #576437 - Flags: review?(bugs) → review+
(Assignee)

Updated

6 years ago
Blocks: 704820
(Assignee)

Comment 26

6 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

6 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

6 years ago
Created attachment 576737 [details] [diff] [review]
message change

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

6 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

6 years ago
Created attachment 576757 [details] [diff] [review]
message change, v2
Attachment #576737 - Attachment is obsolete: true
Attachment #576737 - Flags: review?(bugs)
Attachment #576757 - Flags: review?(bugs)
(Reporter)

Updated

6 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 34

6 years ago
Bent, ping?
(Assignee)

Comment 35

6 years ago
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.
Attachment #576437 - Attachment is obsolete: true
Attachment #576757 - Attachment is obsolete: true
Attachment #578885 - Flags: review?(bugs)
(Assignee)

Comment 36

6 years ago
Created attachment 578886 [details] [diff] [review]
Part 2: Tests

Added an xpcshell-test for non-window non-Worker context.
Attachment #576408 - Attachment is obsolete: true
Attachment #578886 - Flags: review?(bugs)
(Assignee)

Comment 37

6 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 38

6 years ago
Filed bug 707484 for separated part.
(Assignee)

Comment 39

6 years ago
Created attachment 578887 [details] [diff] [review]
Part 2: Tests

Sorry, forgot to remove test cases belonging to bug 707484.
Attachment #578886 - Attachment is obsolete: true
Attachment #578887 - Flags: review?(bugs)
Attachment #578886 - Flags: review?(bugs)
(Reporter)

Updated

6 years ago
Attachment #578885 - Flags: review?(bugs) → review+
(Reporter)

Updated

6 years ago
Attachment #578887 - Flags: review?(bugs) → review+
(Assignee)

Comment 40

6 years ago
Created attachment 579648 [details] [diff] [review]
Part 1: disallow responseType and withCredentials for sync XHR. r=smaug

Unbitrotted.
Attachment #578885 - Attachment is obsolete: true
Attachment #579648 - Flags: review+
(Assignee)

Updated

6 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

6 years ago
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
Attachment #578887 - Attachment is obsolete: true
Attachment #579842 - Flags: review?(bugs)
(Assignee)

Comment 44

6 years ago
Created attachment 579846 [details] [diff] [review]
Part 2: Tests

s/onload/onloadend/
Attachment #579842 - Attachment is obsolete: true
Attachment #579842 - Flags: review?(bugs)
Attachment #579846 - Flags: review?(bugs)
(Reporter)

Updated

6 years ago
Attachment #579846 - Flags: review?(bugs) → review+
(Assignee)

Updated

6 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
https://hg.mozilla.org/mozilla-central/rev/e21ee958e5fd
https://hg.mozilla.org/mozilla-central/rev/c175352a397b
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Blocks: 712621

Updated

6 years ago
Depends on: 713473

Comment 47

6 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.
(Assignee)

Updated

6 years ago
Depends on: 716765
(Reporter)

Comment 48

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

Comment 49

5 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.