Closed Bug 608735 Opened 14 years ago Closed 11 years ago

XMLHttpRequest's getAllResponseHeaders() returns null if the request is the result of CORS

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla21

People

(Reporter: ntoll, Assigned: florin.botis)

References

Details

(Whiteboard: [mentor=jdm][lang=c++][CORS-testsuite])

Attachments

(1 file, 6 obsolete files)

Proposed patch (hopefuly the final one ;) )
11.61 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US) AppleWebKit/534.7 (KHTML, like Gecko) Chrome/7.0.517.41 Safari/534.7
Build Identifier: Firefox 3.6.8 on Ubuntu Lucid (Mozilla Firefox for Ubuntu canonical - 1.0)

When making a cross-origin resource sharing based request (see: http://www.w3.org/TR/cors/) I should be able to get the response headers by calling the getAllResponseHeaders() method of the XMLHttpRequest class. Unfortunately, it returns null. :-(

Reproducible: Always

Steps to Reproduce:
xhr = new XMLHttpRequest();
xhr.open("GET", "http://cross-origin.site/foo/bar"); // replace the URI
xhr.send();
xhr.getAllResponseHeaders();

* NB - the URI MUST reference a resource that is NOT at the originating domain. See the w3c specification referenced in the details for more information.
Actual Results:  
The getAllResponseHeaders function returned null

Expected Results:  
The getAllResponseHeaders function should return something like this:

"Content-Type: application/json
Cache-Control: no-cache
"

etc...

I know this isn't useful for you guys but I wanted to make sure this was specific to Firefox and NOT the result of me misunderstanding the capabilities of the XHR class, so I checked these steps in a recent build of Chromium and it behaved as expected (i.e. I *could* get the headers with the getAllResponseHeaders function). Also, although I've not checked, this might also be impacting the getResponseHeader function too.

The upshot is that I can't make CORS based HTTP calls using the HEAD method in Firefox. :-(

I'm using an up-to-date version of Ubuntu Lucid.

Keep up the good work and here's hoping this gets fixed soon. Feel free to email me if you require further information.
Assignee: general → nobody
Component: JavaScript Engine → DOM
QA Contact: general → general
> although I've not checked, this might also be impacting the getResponseHeader

Based on code inspection, it's not.  Compare the block starting at http://hg.mozilla.org/mozilla-central/file/443276883c2d/content/base/src/nsXMLHttpRequest.cpp#l1438 (for getResponseHeader) to the block starting at http://hg.mozilla.org/mozilla-central/file/443276883c2d/content/base/src/nsXMLHttpRequest.cpp#l1390 for getAllResponseHeaders.

Jonas, is this something we need to block 2.0 on?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Huh, I could have sworn that the XHR Level 2 spec (which is the spec that defines the interaction between XHR and CORS) prescribed this behavior.

Doesn't appear like it does though (at least not any more).

I don't see a reason to block on this. We've behaved this way forever I believe.

You can still use getResponseHeader to inspect individual headers though.
Guys,

Thanks for looking into this. 

I suppose with the CORS specification in draft makes it a moving target and hard to keep track of prescribed behaviour.

From a web-developer's perspective (i.e. mine) WRT "You can still use getResponseHeader to inspect individual headers though" - if I'm using a HEAD method in a CORS based request to try to discover what headers there are on a resource then I'm still stymied. There's nothing in the CORS specification that says I'm limited to the choice of methods I'm allowed to use assuming they're validated by the pre-flight request. (But it's good to know that there is at least some way to get to known or expected headers.)

Anyway, many thanks once again for the awesomeness that is Mozilla.

Nicholas.
Actually, looking at this: http://www.w3.org/TR/XMLHttpRequest2/#the-getallresponseheaders-method the quote that seems most relevant is. "The Cross-Origin Resource Sharing specification filters the headers that are exposed by getAllResponseHeaders() for non same-origin requests."

I'm can only assume that they're referring to this: 

"User agents must filter out all response headers other than those that are a simple response header or of which the field name is an ASCII case-insensitive match for one of the values of the Access-Control-Expose-Headers headers (if any), before exposing response headers to APIs defined in CORS API specifications.

E.g. the getResponseHeader() method of XMLHttpRequest will therefore not expose any header not indicated above."

(source: http://dev.w3.org/2006/waf/access-control/#handling-a-response-to-a-cross-origin-re)

Although no mention is made of getResponseHeaders in the above quote I can only assume that the same conditions apply as implied by the XMLHttpRequest2 quote.

Hope this saves you some leg work checking this stuff.

Nicholas.
Hi,

I wanted to work around this bug in jQuery [1], and they wanted to know [2]: is there a reason that this hasn't been fixed yet?

Conrad

[1] http://bugs.jquery.com/ticket/10338
[2] https://github.com/jquery/jquery/pull/517#issuecomment-2184486
Lack of resources, mostly.
This has been fixed in WebKit already, see https://bugs.webkit.org/show_bug.cgi?id=41210.
Having come across this problem recently, here's how I believe it needs to be resolved.

According to https://developer.mozilla.org/en/http_access_control CORS boils down to being EXTREMELY specific about what elements are observable. Elements are made observable to the client using a number of specific "Access-Control-*" headers declared in the responses. (headers defined in the OPTIONS response specify permitted client outbound elements, headers defined in the method response specify permitted client inbound elements.)

In this case, you need to have your server specify which headers the client needs to be able to see as part of the response. (e.g.

   Access-Control-Expose-Headers: etag, my-nifty-header, content-type

) 

It is not possible for the CORS client to see any header that is not exposed in this way.
That won't help on its own; we need to actually change getAllResponseHeaders to look at that header.
Assignee: nobody → yati.sagade
Whiteboard: [mentor=jdm][lang=c++]
Resetting the owner of this task that hasn't seen any activity in a while.
Assignee: yati.sagade → nobody
Whiteboard: [mentor=jdm][lang=c++] → [mentor=jdm][lang=c++][CORS-testsuite]
Blocks: 817962
Attached patch Proposed patch (obsolete) — — Splinter Review 
I attached the output of git diff command. Please review.
Comment on attachment 694708 [details] [diff] [review]
Proposed patch

florin, thanks for the patch!  For future reference, you probably want to set the "review" flag on the attachment to someone who's reviewed things in the relevant file in the past, just so the review request doesn't get lost.

I'll review this tomorrow afternoon.
Attachment #694708 - Flags: review?(bzbarsky)
Comment on attachment 694708 [details] [diff] [review]
Proposed patch

Sorry for the lag here; holidays intervened. :(  Unfortunately, I'm also not going to be working much between Friday this week and Tuesday next week, but reviews should be quick after that.

There are various whitespace and indentation and formatting and naming nits here, but I figure we should get the logic right first and worry about those later.

So I see a few problems:

1)  We need to add some tests for the behavior here.  Let me know if you can't
    figure out where to add those.
2)  The patch is incorrectly blocking access to getAllResponseHeaders for
    same-origin error responses, as far as I can see.  A test would have caught
    this, I bet.
3)  getResponseHeader now seems to be doing the set-cookie checks twice,
    right?  Why?
Attachment #694708 - Flags: review?(bzbarsky)
Attached patch Patch update after code review (obsolete) — — Splinter Review 
Sorry for my lag too...
Thanks for the review Boris!
See my responses inline (<Florin>):

(In reply to Boris Zbarsky (:bz) from comment #14)
> Comment on attachment 694708 [details] [diff] [review]
> Proposed patch
> 
> Sorry for the lag here; holidays intervened. :(  Unfortunately, I'm also not
> going to be working much between Friday this week and Tuesday next week, but
> reviews should be quick after that.
> 
> There are various whitespace and indentation and formatting and naming nits
> here, but I figure we should get the logic right first and worry about those
> later.

<Florin> Do you have any tool that auto formats the code? If not I will take a look at your code style documentation (https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style)

> So I see a few problems:
> 
> 1)  We need to add some tests for the behavior here.  Let me know if you
> can't
>     figure out where to add those.

<Florin> Where do I need to add them? Any simple tests that could give me some guidance about how to create new ones?

> 2)  The patch is incorrectly blocking access to getAllResponseHeaders for
>     same-origin error responses, as far as I can see.  A test would have
> caught
>     this, I bet.

<Florin> See the new patch.


> 3)  getResponseHeader now seems to be doing the set-cookie checks twice,
>     right?  Why?

<Florin> Fixed this in the new patch.
Attachment #694708 - Attachment is obsolete: true
Attachment #697377 - Flags: review?(bzbarsky)
> <Florin> Do you have any tool that auto formats the code?

http://beaufour.dk/jst-review/ is a good start for a lint tool.  It won't auto format things for you, but it'll complain about issues.

> <Florin> Where do I need to add them?

Probably the best way to do this is to add a line to content/base/test/file_CrossSiteXHR_inner_data.sjs right around where it does getResponseHeader to call getAllResponseHeaders and store that as res.allResponseHeaders, and similar in content/base/test/file_CrossSiteXHR_inner.html and file_CrossSiteXHR_inner.jar.  Then change content/base/test/test_CrossSiteXHR.html to test for the correct results (see the existing expectedResponseHeaders bit and so forth it has, and the place where that's tested down in the "for (test of tests)" loop.

Looking at patch now.
Comment on attachment 697377 [details] [diff] [review]
Patch update after code review

>+++ b/content/base/src/nsXMLHttpRequest.cpp
>+/*
>+Method that checks if it is safe to expose a header value to the client. 
>+It is used to check what headers are exposed for CORS requests.
>+*/

Probably better to add '*' to the beginnings of the two comment lines too, and line the four '*' chars up.

>+nsXMLHttpRequest::IsSafeHeader(const nsACString& header){

Please put the '{' on the next line.

>+  nsCOMPtr<nsIHttpChannel> httpChannel = GetCurrentHttpChannel();

I think it's better to have callers pass in the nsIHttpChannel*, so we can avoid the work of getting it again for every header.  In particular, the GetRequestHeader caller already has an nsIHttpChannel in scope, and so does the place where we create the nsHeaderVisitor.  We could just store it in a member of nsHeaderVisitor, and pass to this method as needed.

>+    // See bug #380418. Hide "Set-Cookie" headers from non-chrome scripts.

This entire block is mis-indented: the "if" and trailing "}" are indented too far by two spaces, and the body by one space.

>+  if (!(mState & XML_HTTP_REQUEST_USE_XSITE_AC)){
>+    return true;
>+  } else {

Please don't do else-after-return.  Instead, do this:

  if (!(mState & XML_HTTP_REQUEST_USE_XSITE_AC)){
    return true;
  }

  // Check for dangerous headers

and then outdent what's now the body of the else as needed.

>+      if (header.LowerCaseEqualsASCII(kCrossOriginSafeHeaders[i])) {
>+        return true;
>+       }

The indent on that '}' is off.

>+    while(exposeTokens.hasMoreTokens()) {

The body of this while is indented too far.

>+      }
>+        return false;

And these bits are just indented completely wrong.

>+    nsRefPtr<nsHeaderVisitor> visitor = new nsHeaderVisitor(this);

So here you'd pass both httpChannel and this.

>@@ -1448,64 +1509,9 @@ nsXMLHttpRequest::GetResponseHeader(const nsACString& header,
>+  if (!IsSafeHeader(header)) {
>       return;
>   }

Please fix the indent on that "return;".

> nsHeaderVisitor::VisitHeader(const nsACString &header, const nsACString &value)
>+  if (mNsXMLHttpRequest->IsSafeHeader(header)){
>+     mHeaders.Append(header);
>+     mHeaders.Append(": ");
>+     mHeaders.Append(value);
>+     mHeaders.Append("\r\n");
>     }

Again, all the indentation is off....

>+++ b/content/base/src/nsXMLHttpRequest.h
>+  bool IsSafeHeader(const nsACString& aHeade);

aHeaderName, perhaps?

>+// used to implement getAllResponseHeaders()

Please don't break the indentation of existing stuff. ;)

>+  nsXMLHttpRequest *mNsXMLHttpRequest;

"nsXMLHttpRequest* mXHR;" is what I'd call this member.

And please put it down in the "private:" section below the public bits.

>+      :mNsXMLHttpRequest(aXMLHttpRequest) {}

Space after ':', please.

r=me with the above nits fixed and with some tests written.  Thank you for the patch!
Attachment #697377 - Flags: review?(bzbarsky) → review+
Hey Boris,

Thanks for the review and sry for all the formatting problems, hopefully now the code looks good.

I updated the code per your code review and added some tests. There were 2 tests in test_CrossSiteXHR.html that were failing after my code changes, but I think the expected results were incorrect. I modified the expected results: see the diff file for more details.

Waiting for your feedback!
Attachment #697377 - Attachment is obsolete: true
Attachment #697893 - Flags: review?(bzbarsky)
Now I'm seeing that I forgot to delete the alert() calls from from test_CrossSiteXHR.html and the first commented line from IsSafeHeader method. Please ignore them when you review the code.
Comment on attachment 697893 [details] [diff] [review]
Proposed patch 3 (code review feedback included)

Review of attachment 697893 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/test/test_CrossSiteXHR.html
@@ +686,2 @@
>                 test.toSource());
> +	    is(res.allResponseHeaders[header], null,

There's a tab in the beginning of this line which causes indentation to be off. Only use spaces.

I don't understand how this test could work though.

xhr.getAllResponseHeaders() doesn't return an object, it returns a string. But here you are accessing sub properties from the return value of getAllResponseHeaders.
Comment on attachment 697893 [details] [diff] [review]
Proposed patch 3 (code review feedback included)

> but I think the expected results were incorrect.

Hmm.  Those tests seem to be testing the "Access-Control-Expose-Headers has invalid tokens in the value" case, no?  Specifically, "y-my-header z" and "y-my-hea(er" are not valid header names.

So I would have expected those to end up with empty expectedResponseHeaders both before and after your patch...  I don't see why this patch would have changed behavior on those.  Can you please double-check that part?

Past that, comment 20 is right: the new test doesn't seem to be doing the right thing...  And we might still want a test for getResponseHeader on a same-origin error page.

Jonas, you know these tests way better than I do; any suggestions for where to add a test for that last?
Attachment #697893 - Flags: feedback?(jonas)
(In reply to Jonas Sicking (:sicking) from comment #20)
> Comment on attachment 697893 [details] [diff] [review]
> Proposed patch 3 (code review feedback included)
> 
> Review of attachment 697893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/test/test_CrossSiteXHR.html
> @@ +686,2 @@
> >                 test.toSource());
> > +	    is(res.allResponseHeaders[header], null,
> 
> There's a tab in the beginning of this line which causes indentation to be
> off. Only use spaces.
> 
> I don't understand how this test could work though.
> 
> xhr.getAllResponseHeaders() doesn't return an object, it returns a string.
> But here you are accessing sub properties from the return value of
> getAllResponseHeaders.

string.foo returns undefined, which is == null, and is() is too stupid to use ===, so this would pass.
(In reply to Boris Zbarsky (:bz) from comment #21)
> Comment on attachment 697893 [details] [diff] [review]
> Proposed patch 3 (code review feedback included)
> 
> > but I think the expected results were incorrect.
> 
> Hmm.  Those tests seem to be testing the "Access-Control-Expose-Headers has
> invalid tokens in the value" case, no?  Specifically, "y-my-header z" and
> "y-my-hea(er" are not valid header names.
> 
> So I would have expected those to end up with empty expectedResponseHeaders
> both before and after your patch...  I don't see why this patch would have
> changed behavior on those.  Can you please double-check that part?


Found the problem:

Without my patch, all tokens listed in Access-Control-Expose-Headers are checked for validity and if at least one of them is invalid getResponseHeader() doesn't return anything.  

With my patch, I iterate over the tokens and if the token passed to getResponseHeader as a parameter is found I return it. I don't check all the tokens for validity.

I'm not sure what is the correct behavior, do we need to ignore Access-Control-Expose-Headers if one of the tokens listed there is invalid ?

> Past that, comment 20 is right: the new test doesn't seem to be doing the
> right thing...  And we might still want a test for getResponseHeader on a
> same-origin error page.

I will update the tests: parse the string returned by getAllResponseHeaders and populate res.allResponseHeaders

> Jonas, you know these tests way better than I do; any suggestions for where
> to add a test for that last?
I would say that if any of the tokens in the header is not valid, we should consider the whole header as invalid. IIRC that's what we do elsewhere.
Attached patch Proposed patch 4 (obsolete) — — Splinter Review 
-updated isSafeHeader method to return false if any of the tokens is invalid
-updated the test code to populate the res.allResponseHeaders correctly by parsing the string returned by xhr.getAllResponseHeaders()
Attachment #697893 - Attachment is obsolete: true
Attachment #697893 - Flags: review?(bzbarsky)
Attachment #697893 - Flags: feedback?(jonas)
Attachment #698567 - Flags: review?(bzbarsky)
Attachment #698567 - Flags: feedback?(Ms2ger)
Comment on attachment 698567 [details] [diff] [review]
Proposed patch 4

Review of attachment 698567 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsXMLHttpRequest.cpp
@@ +1337,5 @@
>  }
>  
> +/*Method that checks if it is safe to expose a header value to the client.
> +It is used to check what headers are exposed for CORS requests.*/
> +bool 

Nit: trailing whitespace

@@ +1352,5 @@
> +  // if this is not a CORS call all headers are safe
> +  if (!(mState & XML_HTTP_REQUEST_USE_XSITE_AC)){
> +    return true;
> +  }
> +  

Trailing whitespace

@@ +1363,5 @@
> +    if (NS_FAILED(status)) {
> +      return false;
> +    }
> +  }
> +  

aTrailing whitespace

@@ +1364,5 @@
> +      return false;
> +    }
> +  }
> +  
> +  const char *kCrossOriginSafeHeaders[] = {

* to the left

@@ +1369,5 @@
> +    "cache-control", "content-language", "content-type", "expires",
> +    "last-modified", "pragma"
> +  };
> +  uint32_t i;
> +  for (i = 0; i < ArrayLength(kCrossOriginSafeHeaders); ++i) {

Move uint32_t into the for loop header.

@@ +1383,5 @@
> +      GetResponseHeader(NS_LITERAL_CSTRING("Access-Control-Expose-Headers"),
> +                        headerVal);
> +  nsCCharSeparatedTokenizer exposeTokens(headerVal, ',');
> +  bool isSafe = false;
> +  while(exposeTokens.hasMoreTokens()) {

'while ('

@@ +1389,5 @@
> +    if (token.IsEmpty()) {
> +      continue;
> +    }
> +    if (!IsValidHTTPToken(token)) {
> +      isSafe = false;

Do you want to return false here, rather than continuing the loop?

@@ +1392,5 @@
> +    if (!IsValidHTTPToken(token)) {
> +      isSafe = false;
> +    }
> +    if (header.Equals(token, nsCaseInsensitiveCStringComparator())) {
> +      isSafe = true;

And return true directly? Unless you want to make sure that every token is valid...

@@ +3975,4 @@
>  NS_IMETHODIMP nsXMLHttpRequest::
>  nsHeaderVisitor::VisitHeader(const nsACString &header, const nsACString &value)
>  {
> +  if (mXHR->IsSafeHeader(header, mHttpChannel)){

Space between ) and {

::: content/base/src/nsXMLHttpRequest.h
@@ +544,4 @@
>    public:
>      NS_DECL_ISUPPORTS
>      NS_DECL_NSIHTTPHEADERVISITOR
> +    nsHeaderVisitor(nsXMLHttpRequest *aXMLHttpRequest, nsIHttpChannel *aHttpChannel)

* to the left

@@ +549,5 @@
>      virtual ~nsHeaderVisitor() {}
>      const nsACString &Headers() { return mHeaders; }
>    private:
>      nsCString mHeaders;
> +    nsXMLHttpRequest *mXHR;

and here

::: content/base/test/file_CrossSiteXHR_inner.html
@@ +8,4 @@
>  <html>
>  <head>
>  <script>
> +String.prototype.trim=function(){return this.replace(/^\s+|\s+$/g, '');};

I'd prefer if you just added a local function rather than messing with prototypes

@@ +66,5 @@
>  
> +    res.allResponseHeaders = {};
> +    var splitHeaders = xhr.getAllResponseHeaders().split("\r\n");
> +    for (var i = 0; i < splitHeaders.length; i++) {
> +      var headerValuePair=splitHeaders[i].split(":");

Space before and after '='

@@ +67,5 @@
> +    res.allResponseHeaders = {};
> +    var splitHeaders = xhr.getAllResponseHeaders().split("\r\n");
> +    for (var i = 0; i < splitHeaders.length; i++) {
> +      var headerValuePair=splitHeaders[i].split(":");
> +        if(headerValuePair[1] != null){

'if (headerValuePair[1]) {', and indentation is off

@@ +69,5 @@
> +    for (var i = 0; i < splitHeaders.length; i++) {
> +      var headerValuePair=splitHeaders[i].split(":");
> +        if(headerValuePair[1] != null){
> +          var headerName = headerValuePair[0].trim();
> +          var headerValue = headerValuePair[1].trim(); 

Trailing whitespace

::: content/base/test/file_CrossSiteXHR_inner_data.sjs
@@ +2,4 @@
>  <html>\n\
>  <head>\n\
>  <script>\n\
> +String.prototype.trim=function(){return this.replace(/^\s+|\s+$/g, '');};

Same

@@ +52,4 @@
>        res.responseHeaders[responseHeader] =\n\
>          xhr.getResponseHeader(responseHeader);\n\
>      }\n\
> +   

Trailing whitespace

@@ +56,5 @@
> +    res.allResponseHeaders = {};\n\
> +    var splitHeaders = xhr.getAllResponseHeaders().split("\r\n");\n\
> +    for (var i = 0; i < splitHeaders.length; i++) {\n\
> +      var headerValuePair=splitHeaders[i].split(":");\n\
> +        if(headerValuePair[1] != null){\n\

Same
Attachment #698567 - Flags: feedback?(Ms2ger)
Attached patch Proposed patch 5 (obsolete) — — Splinter Review 
Updated the code per Ms2ger suggestions.
Attachment #698567 - Attachment is obsolete: true
Attachment #698567 - Flags: review?(bzbarsky)
Attachment #698611 - Flags: review?(bzbarsky)
Attachment #698611 - Flags: feedback?(Ms2ger)
Comment on attachment 698611 [details] [diff] [review]
Proposed patch 5

r=me

I pushed this to try as https://tbpl.mozilla.org/?tree=Try&rev=53ee2db4b3c7
Attachment #698611 - Flags: review?(bzbarsky) → review+
Assignee: nobody → florin.botis
Comment on attachment 698611 [details] [diff] [review]
Proposed patch 5

bz's review should be sufficient
Attachment #698611 - Flags: feedback?(Ms2ger)
There's a test failure: 

6828 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_CrossSiteXHR_origin.html | Test timed out.

Florin, are you willing to look into that?
I ran test_CrossSiteXHR_origin.html separately by using:

TEST_PATH=content/base/test/test_CrossSiteXHR.html make -C $(OBJDIR) mochitest-plain

but all 830 tests are passing. 

When I ran the tests using ./mach mochitest-plain it froze at one of test_CrossSiteXHR test. 

I will investigate more tomorrow...
> TEST_PATH=content/base/test/test_CrossSiteXHR.html make -C $(OBJDIR) mochitest-plain

You want test_CrossSiteXHR_origin.html.  That's a different test from test_CrossSiteXHR.html.
There was a problem in the way I wrote the trimString function in file_CrossSiteXHR_inner_data.sjs. I updated it and test it on my machine.
Attachment #698611 - Attachment is obsolete: true
Attachment #700265 - Flags: review?(bzbarsky)
Comment on attachment 700265 [details] [diff] [review]
Proposed patch 6 (fix for the failing tests)

>+    var splitHeaders = xhr.getAllResponseHeaders().split(\r\n);\n\

Why the change to this line?  Shouldn't that still have the double-quotes around the \r\n?  Or more likely, look something like split("\\r\\n") ?
I'm not sure how the sjs files are used for testing purpose but it seems that if I use split("\r\n") I get some kind of error. Using split(\r\n) all works fine...
Do you want me to change it to ("\\r\\n")?
> I'm not sure how the sjs files are used for testing purpose

It's a script that runs on the web server.  In this case it's building a string to send as the HTTP response.

If you use \r\n, that just puts a literal newline in the string, so the script ends up looking like this:

  var splitHeaders = xhr.getAllResponseHeaders().split(
);

As far as I can tell that will create a single-element array whose value is the string that split() was called on...

> I get some kind of error.

What error?

> Do you want me to change it to ("\\r\\n")?

I'd like you to see what happens if you do that, yes.
(In reply to Boris Zbarsky (:bz) from comment #36)
> > I'm not sure how the sjs files are used for testing purpose
> 
> It's a script that runs on the web server.  In this case it's building a
> string to send as the HTTP response.
> 
> If you use \r\n, that just puts a literal newline in the string, so the
> script ends up looking like this:
> 
>   var splitHeaders = xhr.getAllResponseHeaders().split(
> );
> 
> As far as I can tell that will create a single-element array whose value is
> the string that split() was called on...
Yup, that what happens...
> 
> > I get some kind of error.
> 
> What error?
Syntax error: Unterminated quoted string
> 
> > Do you want me to change it to ("\\r\\n")?
> 
> I'd like you to see what happens if you do that, yes.
I will fix this in the next patch.
using split("\\r\\n")
Attachment #700265 - Attachment is obsolete: true
Attachment #700265 - Flags: review?(bzbarsky)
Attachment #700440 - Flags: review?(bzbarsky)
> Syntax error: Unterminated quoted string

Yes, that part makes sense.

I assume you ran the test_*XHR* tests on that last patch?
(In reply to Boris Zbarsky (:bz) from comment #39)
> > Syntax error: Unterminated quoted string
> 
> Yes, that part makes sense.
> 
> I assume you ran the test_*XHR* tests on that last patch?

Yes I did. Let's see what happens on tinderbox.
Attachment #700440 - Attachment is patch: true
Comment on attachment 700440 [details] [diff] [review]
Proposed patch (hopefuly the final one ;) )

r=me. Pushing to try.
Attachment #700440 - Flags: review?(bzbarsky) → review+
Try push at https://tbpl.mozilla.org/?tree=Try&rev=dae35b999af3 looks good.  Once the tree reopens I'll check this in.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc4bc8745b7

Thank you for sticking with this!
Flags: in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/edc4bc8745b7
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Has this been released yet?  I am experiencing this problem on firefox 20.0.  I get the headers as expected on chromium "Version 26.0.1410.43 (189671)".
According to the Target Milestone field, it will be present in Firefox 21.
Oops, I couldn't see that. Thanks
Any update on when this will make it in? moz21 has come and gone and I am still experiencing the issue.
This should have been fixed in Firefox 21. Please provide a small testcase if you can still reproduce.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: