471122 - test_CrossSiteXHR.html : improve thrown exceptions from file_CrossSiteXHR_server.sjs

Status: VERIFIED FIXED

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

Description

[Serge Gautherie (:sgautherie)]

I noticed, for example:
{
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1230204931.1230208856.19091.gz&fulltext=1
Linux mozilla-central unit test on 2008/12/25 03:35:31

*** 3740 INFO TEST-PASS | /tests/content/base/test/test_CrossSiteXHR.html | wrong responseText in test for ({pass:1, method:"GET", noCookie:1, withCred:0, allowCred:1})
Got cookies when didn't expect to
}
NB: It would look like the exception does not always happen at the same line, but that's not the main point.

***

1)
Code is
{
53   if ("noCookie" in query && request.hasHeader("Cookie")) {
54     throw "Got cookies when didn't expect to";
}

Compared to the other |throw| in this file, this is the only one which doesn't tell what the data is.
Is there a reason not to throw the header content too ?

2)
Code is
{
83   } catch (e) {
84     dump(e + "\n");
85     throw e;
86   }
}

It would be great if this could be logged (somehow/somewhere) as a full "INFO TEST-*" line,
so we could explicitly know where these "exceptions" come from and if they are PASS/FAIL/whatever.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Sure, we can include the unexpected cookie data, no harm in that. It'll be harder to right there tell if it's PASS or FAIL though as that is only known on the client side. Basically the best way to tell is to se if the mochitest passes or not.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Actually one thing that might be a good idea is to not let these exceptions be thrown all the way out of the sjs file, but instead catch them and create a 500 response. That'll prevent any expected exceptions from being logged.

Comment 3

[Serge Gautherie (:sgautherie)]

I'm working on a patch to fix this and probably a few other issues.

Throwing or using response.setStatusLine(), it seems the xhr only ever gets updated to either 200+text or 0+null:

the closest I have found to what I started doing is
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/authenticate.sjs?mark=1-9
and it would seem to implicitly confirm this.

Do you actually know how to "create a 500 response" ?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Even if the server returns a 500 result, a cross site request will for various security reasons turn that into a 0. The current tests even test that.

All you need to do is change the sjs file to call

response.setStatusLine(null, 500, <thrown error message>);

when an error is thrown. No changes to the tests are needed.

Comment 5

[Serge Gautherie (:sgautherie)]

Then I managed to guess as much.
And, if you say setStatusLine() is better than throw, I can only concur.

Yet, what disappointed me is that the end result seems to be the same in both cases (is it not ?):
there seems to be no way to send the error text to the test, as the latter always gets null in addition to 0, is there ?

(NB: Simply modifying the sjs code not to dump the error text "could" be done in other ways...)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> Yet, what disappointed me is that the end result seems to be the same in both
> cases (is it not ?):
> there seems to be no way to send the error text to the test, as the latter
> always gets null in addition to 0, is there ?

Yup, that is the desired behavior for security reasons. We don't want it to be possible to detect what the server responded as that would allow scanning password protected sites as well as sites behind firewalls.

> (NB: Simply modifying the sjs code not to dump the error text "could" be done
> in other ways...)

Oh? How?
The other advantage of not thowing is that it seems nicer towards the test suite. I.e. we won't throw errors for tests that run as intended.

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> Yup, that is the desired behavior for security reasons.

I do understand.
I simply was puzzled to discover that we build a meaningful error text which just gets discarded (unless the "security" would not do its job...).

> > (NB: Simply modifying the sjs code not to dump the error text "could" be done
> > in other ways...)
> 
> Oh? How?

For example, by setting a var in addition to the initial throw then use it in the catch to decide whether to dump or not.

> The other advantage of not thowing is that it seems nicer towards the test
> suite. I.e. we won't throw errors for tests that run as intended.

I fully agree: I simply wanted to be sure nothing more could be done.
Thanks for your confirmations.

Patch should follow...

Comment 8

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Use setStatusLine() and removeEventListener()

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20081224 SeaMonkey/2.0a3pre] (home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/967ae997cf22
 +http://hg.mozilla.org/comm-central/rev/3ee88b5a599b + bug 469606 patch)

*Remove unused |function d(s)|.
*Add removeEventListener() for bug 454717.
*Use setStatusLine() per comment 2.
 And rewrite the try+catch.
*Add |request.getHeader("Cookie")| to the error text.

Also,
file_CrossSiteXHR_cache_server.sjs, file_CrossSiteXHR_server.sjs and test_CrossSiteXHR.html have CrLf Eol:
do you want me to convert them to Lf only before checking in the patch ?

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av1a) Use setStatusLine() and removeEventListener()

Av1, using |onLoad()| everywhere.

Comment 10

[Jeff Walden [:Waldo]]

Line ending fixes are always fine, but make sure the revision that changes the line endings does nothing else but that -- so the relevant changeset would include a substantive revision and a line-ending revision, each distinct from the other.  I'm guessing the original commit that added all these tests had this problem, because I fixed this in test_CrossSiteXHR_cache.html a few weeks or so ago (in the two-step manner, of course, as I needed to tweak something else at the same time).

Comment 11

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> do you want me to convert them to Lf only before checking in the patch ?

(In reply to comment #10)
> each distinct from the other.

Yeah, that's what I meant by "before" ;-)

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 354477 [details] [diff] [review]
(Av1a) Use setStatusLine() and removeEventListener()

Ugh, I don't like this patch at all. First of all why do you create the reallyHandleRequest wrapping when there is nothing that is expected to throw. And if something does throw you just do exactly the same work as httpd.js does, which is to report the error to the console, except httpd.js does a better job of it.

And what's the purpose of the onLoad/onUnload functions? Seems to just make the test more cluttery. Also see my feelings regarding removeEventListener in bug 454717 comment 4.

>Bug 471122 - test_CrossSiteXHR.html : improve thrown exceptions from file_CrossSiteXHR_server.sjs
>(Av1a) Use setStatusLine() and removeEventListener()
>
>diff --git a/content/base/test/file_CrossSiteXHR_cache_server.sjs b/content/base/test/file_CrossSiteXHR_cache_server.sjs
>--- a/content/base/test/file_CrossSiteXHR_cache_server.sjs
>+++ b/content/base/test/file_CrossSiteXHR_cache_server.sjs
>@@ -1,10 +1,8 @@
>-function d(s) { dump(s + "\n"); }
>-
> function handleRequest(request, response)
> {
>   var query = {};
>   request.queryString.split('&').forEach(function (val) {
>     var [name, value] = val.split('=');
>     query[name] = unescape(value);
>   });
> 
>diff --git a/content/base/test/file_CrossSiteXHR_inner.html b/content/base/test/file_CrossSiteXHR_inner.html
>--- a/content/base/test/file_CrossSiteXHR_inner.html
>+++ b/content/base/test/file_CrossSiteXHR_inner.html
>@@ -1,14 +1,24 @@
> <!DOCTYPE HTML>
> <html>
> <head>
> <script>
>-window.addEventListener('message', function(e) {
> 
>+function onLoad() {
>+  window.addEventListener("message", el, false);
>+  window.addEventListener("unload", onUnload, false);
>+}
>+
>+function onUnload() {
>+  window.removeEventListener("unload", onUnload, false);
>+  window.removeEventListener("message", el, false);
>+}
>+
>+function el(e) {
>   sendData = null;
> 
>   req = eval(e.data);
>   var res = {
>     didFail: false,
>     events: [],
>     progressEvents: 0
>   };
>@@ -62,21 +72,21 @@ window.addEventListener('message', funct
>   xhr.open(req.method, req.url, true);
> 
>   for (header in req.headers) {
>     xhr.setRequestHeader(header, req.headers[header]);
>   }
> 
>   res.events.push("sending");
>   xhr.send(sendData);
>-
>-}, false);
>+}
> 
> function post(e, res) {
>   e.source.postMessage(res.toSource(), "http://localhost:8888");
> }
> 
> </script>
> </head>
>-<body>
>+
>+<body onload="onLoad();">
> Inner page
> </body>
>-</html>
>\ No newline at end of file
>+</html>
>diff --git a/content/base/test/file_CrossSiteXHR_server.sjs b/content/base/test/file_CrossSiteXHR_server.sjs
>--- a/content/base/test/file_CrossSiteXHR_server.sjs
>+++ b/content/base/test/file_CrossSiteXHR_server.sjs
>@@ -1,87 +1,115 @@
> function handleRequest(request, response)
> {
>   try {
>+    reallyHandleRequest(request, response);
>+  } catch (e) {
>+    // Display the (raw) exception here,
>+    // because it won't be sent to the client (for security reason).
>+    dump("\tException: " + e + "\n");
>+
>+    // Forward the exception, to let the server know something failed.
>+    throw e;
>+  }
>+}
>+
>+function reallyHandleRequest(request, response)
>+{
>   var query = {};
>   request.queryString.split('&').forEach(function (val) {
>     [name, value] = val.split('=');
>     query[name] = unescape(value);
>   });
> 
>   var isPreflight = request.method == "OPTIONS";
> 
>   // Check that request was correct
>+
>   if (!isPreflight && "headers" in query) {
>     headers = eval(query.headers);
>     for(headerName in headers) {
>       if (request.getHeader(headerName) != headers[headerName]) {
>-        throw "Header " + headerName + " had wrong value. Expected " +
>-              headers[headerName] + " got " + request.getHeader(headerName);
>+        sendHttp500(response,
>+          "Header " + headerName + " had wrong value. Expected " +
>+          headers[headerName] + " got " + request.getHeader(headerName));
>+        return;
>       }
>     }
>   }
>+
>   if (isPreflight && "requestHeaders" in query &&
>       request.getHeader("Access-Control-Request-Headers") != query.requestHeaders) {
>-    throw "Access-Control-Request-Headers had wrong value. Expected " +
>-          query.requestHeaders + " got " +
>-          request.getHeader("Access-Control-Request-Headers");
>+    sendHttp500(response,
>+      "Access-Control-Request-Headers had wrong value. Expected " +
>+      query.requestHeaders + " got " +
>+      request.getHeader("Access-Control-Request-Headers"));
>+    return;
>   }
>+
>   if (isPreflight && "requestMethod" in query &&
>       request.getHeader("Access-Control-Request-Method") != query.requestMethod) {
>-    throw "Access-Control-Request-Method had wrong value. Expected " +
>-          query.requestMethod + " got " +
>-          request.getHeader("Access-Control-Request-Method");
>+    sendHttp500(response,
>+      "Access-Control-Request-Method had wrong value. Expected " +
>+      query.requestMethod + " got " +
>+      request.getHeader("Access-Control-Request-Method"));
>+    return;
>   }
>+
>   if ("origin" in query && request.getHeader("Origin") != query.origin) {
>-    throw "Origin had wrong value. Expected " + query.origin + " got " +
>-          request.getHeader("Origin");
>+    sendHttp500(response,
>+      "Origin had wrong value. Expected " + query.origin + " got " +
>+      request.getHeader("Origin"));
>+    return;
>   }
>+
>   if ("cookie" in query) {
>     cookies = {};
>     request.getHeader("Cookie").split(/ *; */).forEach(function (val) {
>       [name, value] = val.split('=');
>       cookies[name] = unescape(value);
>     });
>     
>     query.cookie.split(",").forEach(function (val) {
>       [name, value] = val.split('=');
>       if (cookies[name] != value) {
>-        throw "Cookie " + name  + " had wrong value. Expected " + value +
>-              " got " + cookies[name];
>+        sendHttp500(response,
>+          "Cookie " + name  + " had wrong value. Expected " + value +
>+          " got " + cookies[name]);
>+        return;
>       }
>     });
>   }
>+
>   if ("noCookie" in query && request.hasHeader("Cookie")) {
>-    throw "Got cookies when didn't expect to";
>+    sendHttp500(response,
>+      "Got cookies when didn't expect to: " + request.getHeader("Cookie"));
>+    return;
>   }
> 
>-
>   // Send response
>        
>   if (query.allowOrigin && (!isPreflight || !query.noAllowPreflight))
>     response.setHeader("Access-Control-Allow-Origin", query.allowOrigin);
> 
>   if (query.allowCred)
>     response.setHeader("Access-Control-Allow-Credentials", "true");
> 
>   if (query.setCookie)
>     response.setHeader("Set-Cookie", query.setCookie + "; path=/");
> 
>-
>   if (isPreflight) {
>     if (query.allowHeaders)
>       response.setHeader("Access-Control-Allow-Headers", query.allowHeaders);
> 
>     if (query.allowMethods)
>       response.setHeader("Access-Control-Allow-Methods", query.allowMethods);
> 
>     return;
>   }
> 
>   response.setHeader("Content-Type", "application/xml", false);
>   response.write("<res>hello pass</res>\n");
>-  
>-  } catch (e) {
>-    dump(e + "\n");
>-    throw e;
>-  }
> }
>+
>+function sendHttp500(response, text) {
>+  response.setStatusLine(null, 500, text);
>+}
>diff --git a/content/base/test/test_CrossSiteXHR.html b/content/base/test/test_CrossSiteXHR.html
>--- a/content/base/test/test_CrossSiteXHR.html
>+++ b/content/base/test/test_CrossSiteXHR.html
>@@ -2,17 +2,18 @@
> <html>
> <head>
>   <META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=utf-8">
>   <title>Test for Cross Site XMLHttpRequest</title>
>   <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
>   <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        
>   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> </head>
>-<body onload="gen.next()">
>+
>+<body onload="onLoad();">
> <p id="display">
> <iframe id=loader></iframe>
> </p>
> <div id="content" style="display: none">
>   
> </div>
> <pre id="test">
> <script class="testbody" type="application/javascript;version=1.8">
>@@ -28,19 +29,31 @@ var origins =
>    //['https://sub1.test1.example.com:443'],
>    ['http://sub1.\xe4lt.example.org:8000', 'http://sub1.xn--lt-uia.example.org:8000'],
>    ['http://sub2.\xe4lt.example.org', 'http://sub2.xn--lt-uia.example.org'],
>    ['http://ex\xe4mple.test', 'http://xn--exmple-cua.test'],
>    ['http://\u03c0\u03b1\u03c1\u03ac\u03b4\u03b5\u03b9\u03b3\u03bc\u03b1.\u03b4\u03bf\u03ba\u03b9\u03bc\u03ae',
>     'http://xn--hxajbheg2az3al.xn--jxalpdlp'],
>    ];
> 
>-window.addEventListener("message", function(e) {
>+function onLoad() {
>+  window.addEventListener("message", el, false);
>+  window.addEventListener("unload", onUnload, false);
>+
>+  gen.next();
>+}
>+
>+function onUnload() {
>+  window.removeEventListener("unload", onUnload, false);
>+  window.removeEventListener("message", el, false);
>+}
>+
>+function el(e) {
>   gen.send(e.data);
>-}, false);
>+}
> 
> gen = runTest();
> 
> function runTest() {
>   var loader = document.getElementById('loader');
>   var loaderWindow = loader.contentWindow;
>   loader.onload = function () { gen.next() };
> 
>diff --git a/content/base/test/test_CrossSiteXHR_cache.html b/content/base/test/test_CrossSiteXHR_cache.html
>--- a/content/base/test/test_CrossSiteXHR_cache.html
>+++ b/content/base/test/test_CrossSiteXHR_cache.html
>@@ -2,31 +2,44 @@
> <html>
> <head>
>   <META HTTP-EQUIV="Content-Type" CONTENT="text/html; charset=utf-8">
>   <title>Test for Cross Site XMLHttpRequest</title>
>   <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
>   <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        
>   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> </head>
>-<body onload="gen.next()">
>+
>+<body onload="onLoad();">
> <p id="display">
> <iframe id=loader></iframe>
> </p>
> <div id="content" style="display: none">
>   
> </div>
> <pre id="test">
> <script class="testbody" type="application/javascript;version=1.7">
> 
> SimpleTest.waitForExplicitFinish();
> 
>-window.addEventListener("message", function(e) {
>+function onLoad() {
>+  window.addEventListener("message", el, false);
>+  window.addEventListener("unload", onUnload, false);
>+
>+  gen.next();
>+}
>+
>+function onUnload() {
>+  window.removeEventListener("unload", onUnload, false);
>+  window.removeEventListener("message", el, false);
>+}
>+
>+function el(e) {
>   gen.send(e.data);
>-}, false);
>+}
> 
> gen = runTest();
> 
> function runTest() {
>   var loader = document.getElementById('loader');
>   var loaderWindow = loader.contentWindow;
>   loader.onload = function () { gen.next() };
> 
>@@ -338,18 +351,16 @@ function runTest() {
>       is(res.events.join(","),
>          "opening,rs1,sending,rs1,loadstart,rs2,rs4,error",
>          "wrong events in test for " + testName);
>       is(res.progressEvents, 0,
>          "wrong events in test for " + testName);
>     }
>   }
> 
>-
>-
>   SimpleTest.finish();
> 
>   yield;
> }
> 
> </script>
> </pre>
> </body>

Comment 13

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Use setStatusLine() [Checkin: Comment 18 & 19]

Av1a, with comment 12 suggestion(s).

(In reply to comment #12)
> (From update of attachment 354477 [details] [diff] [review])
> why do you create the reallyHandleRequest wrapping

It seemed easier than to reindent the whole block ... and looked similar to some other tests.

> you just do exactly the same work as httpd.js does, which is to
> report the error to the console, except httpd.js does a better job of it.

Does it ? (Then, I may wonder why this try+catch was there in the first place...)

> And what's the purpose of the onLoad/onUnload functions? Seems to just make the
> test more cluttery. Also see my feelings regarding removeEventListener in bug
> 454717 comment 4.

So you've read that bug: I don't know what I can answer more :-|

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The old try/catch was there from before httpd.js did what it did (i might have fixed httpd.js at the same time as checking in this test though).

One thing though, why not fix file_CrossSiteXHR_cache_server.sjs the same way? (sorry, missed this in original comment)

Comment 15

[Serge Gautherie (:sgautherie)]

(In reply to comment #8)
> Also,
> file_CrossSiteXHR_cache_server.sjs, file_CrossSiteXHR_server.sjs and
> test_CrossSiteXHR.html have CrLf Eol:
> do you want me to convert them to Lf only before checking in the patch ?

What about this question ?

(In reply to comment #14)
> One thing though, why not fix file_CrossSiteXHR_cache_server.sjs the same way?

That file has no try nor throw, does it ?

Its only additional nit is
-    var [name, value] = val.split('=');
+    [name, value] = val.split('=');

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #15)
> (In reply to comment #8)
> > Also,
> > file_CrossSiteXHR_cache_server.sjs, file_CrossSiteXHR_server.sjs and
> > test_CrossSiteXHR.html have CrLf Eol:
> > do you want me to convert them to Lf only before checking in the patch ?
> 
> What about this question ?

Yes, please do.

> (In reply to comment #14)
> > One thing though, why not fix file_CrossSiteXHR_cache_server.sjs the same way?
> 
> That file has no try nor throw, does it ?

Ah, forgot that was the case.

> Its only additional nit is
> -    var [name, value] = val.split('=');
> +    [name, value] = val.split('=');

Why do this? That makes those vars global which seems unnecessary (though arguably not harmful)

Comment 17

[Serge Gautherie (:sgautherie)]

(In reply to comment #16)
> > Its only additional nit is
> > -    var [name, value] = val.split('=');
> > +    [name, value] = val.split('=');
> 
> Why do this? That makes those vars global which seems unnecessary (though
> arguably not harmful)

It's simply what you did in
http://hg.mozilla.org/mozilla-central/diff/ca957fbe0ff6/content/base/test/file_CrossSiteXHR_server.sjs
I don't know whether the two files should be the same, with/without the |var|:
you talked about synchronizing them, so I thought I would mention it.
Maybe the |var| should be added back to file_CrossSiteXHR_server.sjs ?

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 354524 [details] [diff] [review]
(Av2) Use setStatusLine()
[Checkin: Comment 18 & 19]

CrLf->Lf Eol:
http://hg.mozilla.org/mozilla-central/rev/320d48cdd0ee
This patch:
http://hg.mozilla.org/mozilla-central/rev/2bbf690dac7e

Comment 19

[Serge Gautherie (:sgautherie)]

Comment on attachment 354524 [details] [diff] [review]
(Av2) Use setStatusLine()
[Checkin: Comment 18 & 19]

CrLf->Lf Eol:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/19f1ecd9876f
This patch:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/3c34d18f5493

Comment 20

[Serge Gautherie (:sgautherie)]

V.Fixed, as the extra log line is not in the new logs.

Comment 21

[Serge Gautherie (:sgautherie)]

verified1.9.1 too.

***

(In reply to comment #13)
> > Also see my feelings regarding removeEventListener in bug 454717 comment 4.
> 
> So you've read that bug: I don't know what I can answer more :-|

Then, (it turned out) that bug doesn't apply to this (kind of) test.

Comment 22

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) file_CrossSiteXHR_server.sjs: Add missing |var| [Checkin: Comment 23 & 24]

Per comment 16.

Comment 23

[Serge Gautherie (:sgautherie)]

Comment on attachment 371100 [details] [diff] [review]
(Bv1) file_CrossSiteXHR_server.sjs: Add missing |var|
[Checkin: Comment 23 & 24]


http://hg.mozilla.org/mozilla-central/rev/e45f667d731d

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 371100 [details] [diff] [review]
(Bv1) file_CrossSiteXHR_server.sjs: Add missing |var|
[Checkin: Comment 23 & 24]


http://hg.mozilla.org/releases/mozilla-1.9.1/rev/064bc186003e

Comment 25

[Serge Gautherie (:sgautherie)]

(In reply to comment #23)
> http://hg.mozilla.org/mozilla-central/rev/e45f667d731d

V.Fixed too, per green tinderboxes.

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #24)
> (From update of attachment 371100 [details] [diff] [review])
> 
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/064bc186003e

verified1.9.1 too, per green tinderboxes.