Last Comment Bug 608735 - XMLHttpRequest's getAllResponseHeaders() returns null if the request is the result of CORS
: XMLHttpRequest's getAllResponseHeaders() returns null if the request is the r...
Status: RESOLVED FIXED
[mentor=jdm][lang=c++][CORS-testsuite]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Linux
: -- normal with 12 votes (vote)
: mozilla21
Assigned To: florin.botis
:
Mentors:
Depends on:
Blocks: 817962
  Show dependency treegraph
 
Reported: 2010-11-01 08:44 PDT by Nicholas H.Tollervey
Modified: 2013-08-17 23:15 PDT (History)
23 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch (6.58 KB, patch)
2012-12-20 22:33 PST, florin.botis
no flags Details | Diff | Splinter Review
Patch update after code review (6.79 KB, patch)
2013-01-03 02:27 PST, florin.botis
bzbarsky: review+
Details | Diff | Splinter Review
Proposed patch 3 (code review feedback included) (11.08 KB, patch)
2013-01-04 06:20 PST, florin.botis
no flags Details | Diff | Splinter Review
Proposed patch 4 (11.50 KB, patch)
2013-01-07 01:14 PST, florin.botis
no flags Details | Diff | Splinter Review
Proposed patch 5 (11.59 KB, patch)
2013-01-07 03:32 PST, florin.botis
bzbarsky: review+
Details | Diff | Splinter Review
Proposed patch 6 (fix for the failing tests) (11.60 KB, patch)
2013-01-10 02:35 PST, florin.botis
no flags Details | Diff | Splinter Review
Proposed patch (hopefuly the final one ;) ) (11.61 KB, patch)
2013-01-10 08:05 PST, florin.botis
bzbarsky: review+
Details | Diff | Splinter Review

Description Nicholas H.Tollervey 2010-11-01 08:44:32 PDT
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.
Comment 1 Boris Zbarsky [:bz] 2010-11-01 09:08:38 PDT
> 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?
Comment 2 Jonas Sicking (:sicking) PTO Until July 5th 2010-11-02 20:00:38 PDT
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.
Comment 3 Nicholas H.Tollervey 2010-11-03 02:31:02 PDT
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.
Comment 4 Nicholas H.Tollervey 2010-11-03 02:46:26 PDT
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.
Comment 5 Conrad Irwin 2011-09-23 19:17:16 PDT
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
Comment 6 Boris Zbarsky [:bz] 2011-09-23 20:15:33 PDT
Lack of resources, mostly.
Comment 7 František Hába 2012-01-19 08:35:57 PST
This has been fixed in WebKit already, see https://bugs.webkit.org/show_bug.cgi?id=41210.
Comment 8 JR Conlin [:jrconlin,:jconlin] 2012-05-30 15:04:45 PDT
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.
Comment 9 Boris Zbarsky [:bz] 2012-05-30 18:27:43 PDT
That won't help on its own; we need to actually change getAllResponseHeaders to look at that header.
Comment 10 Yati Sagade 2012-09-03 10:36:22 PDT
The relevant area in code is here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#1302
Comment 11 Josh Matthews [:jdm] 2012-10-29 15:52:25 PDT
Resetting the owner of this task that hasn't seen any activity in a while.
Comment 12 florin.botis 2012-12-20 22:33:59 PST
Created attachment 694708 [details] [diff] [review]
Proposed patch

I attached the output of git diff command. Please review.
Comment 13 Boris Zbarsky [:bz] 2012-12-20 23:40:05 PST
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.
Comment 14 Boris Zbarsky [:bz] 2012-12-26 14:20:31 PST
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?
Comment 15 florin.botis 2013-01-03 02:27:27 PST
Created attachment 697377 [details] [diff] [review]
Patch update after code 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.
Comment 16 Boris Zbarsky [:bz] 2013-01-03 14:32:09 PST
> <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 17 Boris Zbarsky [:bz] 2013-01-03 22:01:05 PST
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!
Comment 18 florin.botis 2013-01-04 06:20:32 PST
Created attachment 697893 [details] [diff] [review]
Proposed patch 3 (code review feedback included)

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!
Comment 19 florin.botis 2013-01-04 06:27:19 PST
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 20 Jonas Sicking (:sicking) PTO Until July 5th 2013-01-04 11:56:21 PST
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 21 Boris Zbarsky [:bz] 2013-01-04 22:10:40 PST
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?
Comment 22 :Ms2ger 2013-01-05 02:07:06 PST
(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.
Comment 23 florin.botis 2013-01-07 00:03:09 PST
(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?
Comment 24 Jonas Sicking (:sicking) PTO Until July 5th 2013-01-07 01:02:23 PST
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.
Comment 25 florin.botis 2013-01-07 01:14:06 PST
Created attachment 698567 [details] [diff] [review]
Proposed patch 4

-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()
Comment 26 :Ms2ger 2013-01-07 01:57:10 PST
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
Comment 27 florin.botis 2013-01-07 03:32:32 PST
Created attachment 698611 [details] [diff] [review]
Proposed patch 5

Updated the code per Ms2ger suggestions.
Comment 28 Boris Zbarsky [:bz] 2013-01-08 21:21:34 PST
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
Comment 29 :Ms2ger 2013-01-09 00:18:19 PST
Comment on attachment 698611 [details] [diff] [review]
Proposed patch 5

bz's review should be sufficient
Comment 30 Boris Zbarsky [:bz] 2013-01-09 07:03:42 PST
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?
Comment 31 florin.botis 2013-01-09 08:06:05 PST
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...
Comment 32 Boris Zbarsky [:bz] 2013-01-09 10:00:11 PST
> 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.
Comment 33 florin.botis 2013-01-10 02:35:08 PST
Created attachment 700265 [details] [diff] [review]
Proposed patch 6 (fix for the failing tests)

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.
Comment 34 Boris Zbarsky [:bz] 2013-01-10 05:58:03 PST
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") ?
Comment 35 florin.botis 2013-01-10 07:21:40 PST
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")?
Comment 36 Boris Zbarsky [:bz] 2013-01-10 07:28:12 PST
> 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.
Comment 37 florin.botis 2013-01-10 08:02:16 PST
(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.
Comment 38 florin.botis 2013-01-10 08:05:40 PST
Created attachment 700440 [details] [diff] [review]
Proposed patch (hopefuly the final one ;) )

using split("\\r\\n")
Comment 39 Boris Zbarsky [:bz] 2013-01-10 08:22:53 PST
> Syntax error: Unterminated quoted string

Yes, that part makes sense.

I assume you ran the test_*XHR* tests on that last patch?
Comment 40 florin.botis 2013-01-10 09:14:58 PST
(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.
Comment 41 Boris Zbarsky [:bz] 2013-01-10 09:59:39 PST
Comment on attachment 700440 [details] [diff] [review]
Proposed patch (hopefuly the final one ;) )

r=me. Pushing to try.
Comment 42 Boris Zbarsky [:bz] 2013-01-10 11:57:58 PST
Try push at https://tbpl.mozilla.org/?tree=Try&rev=dae35b999af3 looks good.  Once the tree reopens I'll check this in.
Comment 43 Boris Zbarsky [:bz] 2013-01-10 14:49:35 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/edc4bc8745b7

Thank you for sticking with this!
Comment 44 Ed Morley [:emorley] 2013-01-11 06:30:10 PST
https://hg.mozilla.org/mozilla-central/rev/edc4bc8745b7
Comment 45 Ed Morley [:emorley] 2013-01-11 06:58:49 PST
https://hg.mozilla.org/mozilla-central/rev/edc4bc8745b7
Comment 46 Ed Morley [:emorley] 2013-01-11 07:47:02 PST
https://hg.mozilla.org/mozilla-central/rev/edc4bc8745b7
Comment 47 Kevin Cox [:kevincox] 2013-04-10 06:31:06 PDT
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)".
Comment 48 Josh Matthews [:jdm] 2013-04-10 06:34:30 PDT
According to the Target Milestone field, it will be present in Firefox 21.
Comment 49 Kevin Cox [:kevincox] 2013-04-10 06:39:31 PDT
Oops, I couldn't see that. Thanks
Comment 50 jim.for.cy 2013-08-14 15:24:43 PDT
Any update on when this will make it in? moz21 has come and gone and I am still experiencing the issue.
Comment 51 Jonas Sicking (:sicking) PTO Until July 5th 2013-08-17 23:15:43 PDT
This should have been fixed in Firefox 21. Please provide a small testcase if you can still reproduce.

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