Last Comment Bug 664179 - Allow Cross-Origin URLs in EventSource (Server-Sent Events)
: Allow Cross-Origin URLs in EventSource (Server-Sent Events)
Status: VERIFIED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Wellington Fernando de Macedo
:
Mentors:
Depends on: 673296 716841
Blocks: 338583
  Show dependency treegraph
 
Reported: 2011-06-14 09:49 PDT by Louis-Rémi BABE
Modified: 2014-11-09 05:46 PST (History)
21 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1 (17.76 KB, patch)
2011-06-26 08:25 PDT, Wellington Fernando de Macedo
bugs: review+
jonas: review-
Details | Diff | Review
patch v2 (28.42 KB, patch)
2011-07-17 19:17 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
patch v2 (28.43 KB, patch)
2011-07-17 19:23 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
patch v3 (28.87 KB, patch)
2011-07-17 20:59 PDT, Wellington Fernando de Macedo
no flags Details | Diff | Review
patch v3 (29.14 KB, patch)
2011-07-17 21:37 PDT, Wellington Fernando de Macedo
bugs: review-
jonas: review+
Details | Diff | Review
patch v4 (36.67 KB, patch)
2011-07-19 18:03 PDT, Wellington Fernando de Macedo
bugs: review+
jonas: review+
Details | Diff | Review
patch v5 (34.36 KB, patch)
2011-07-21 17:15 PDT, Wellington Fernando de Macedo
bugs: review+
jonas: review+
Details | Diff | Review
v6 (34.25 KB, patch)
2011-10-28 18:49 PDT, Wellington Fernando de Macedo
jonas: review+
jonas: checkin+
Details | Diff | Review
fixes on v6 (11.79 KB, patch)
2011-11-05 10:09 PDT, Wellington Fernando de Macedo
jonas: review+
jonas: checkin+
Details | Diff | Review

Description Louis-Rémi BABE 2011-06-14 09:49:37 PDT
Currently the draft specification of EventSource does not allow Cross-Origin URLs parameters: http://www.w3.org/TR/eventsource/#eventsource

The authors of the source agree that this will probably be added to the spec, but nothing has happened yet: http://www.w3.org/Bugs/Public/show_bug.cgi?id=11835

I think we shouldn't wait for the specification, and move the Web forward, allowing developers to experiment with Cross-Origin URLs. This could only feed the debate and help improve both EventSource and CORS specs.
Comment 1 Olli Pettay [:smaug] 2011-06-14 10:32:35 PDT
Yeah, we probably could add support for CORS.

Sicking, you know our CORS implementation. Is it hard to add support
for CORS to some http connection?

Wellington, would you be interested to implement this?
Comment 2 Jonas Sicking (:sicking) 2011-06-14 11:26:11 PDT
It should be trivial to add CORS support. The one issue is that we might need to add a flag on the API which lets the page choose between loading a stream of private data (i.e. the load happens with cookies), or public data (no cookies involved).

On XHR this property is call .withCredentials
Comment 3 Jonas Sicking (:sicking) 2011-06-14 11:32:49 PDT
Here is a good example of code using our CORS implementation:

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsXMLHttpRequest.cpp#2348

Basically all you need to do is:

listener = new nsCORSListenerProxy(listener, mPrincipal, mChannel,
                                   withCredentials, &rv);
NS_ENSURE_SUCCESS(rv, rv);

where |listener| is the streamlistener passed to nsIChannel::AsyncOpen
Comment 4 Wellington Fernando de Macedo 2011-06-15 12:43:57 PDT
> Wellington, would you be interested to implement this?
Yes. I will try to implement it this weekend.
Comment 5 Anne (:annevk) 2011-06-17 11:19:57 PDT
Can we maybe hold off on withCredentials until we know people actually want to use it? I.e. just do the most straightforward implementation that does not involve changes to the API.
Comment 6 Hixie (not reading bugmail) 2011-06-17 12:59:03 PDT
I've updated the spec to define how CORS works in EventSource.
Comment 7 Wellington Fernando de Macedo 2011-06-19 06:48:07 PDT
(In reply to comment #6)
> I've updated the spec to define how CORS works in EventSource.

Hmmm, reading the construction steps of the specification, the steps 3 and 4 force, apparently, SSE to use CORS only after a http redirection (301, 302, 303, 307 http responses). It seems strange... Have I misunderstood it?
Comment 8 Wellington Fernando de Macedo 2011-06-19 06:49:47 PDT
> reading the construction steps of the specification, the steps 3 and 4 (...)
Sorry, steps 3 and 4 -> steps 4 and 5
Comment 9 Hixie (not reading bugmail) 2011-06-20 15:18:01 PDT
I forgot to remove step 4, my apologies. It's gone now.
Comment 10 Wellington Fernando de Macedo 2011-06-26 08:25:21 PDT
Created attachment 542022 [details] [diff] [review]
patch v1
Comment 11 Olli Pettay [:smaug] 2011-06-27 07:08:22 PDT
Comment on attachment 542022 [details] [diff] [review]
patch v1

>diff --git a/content/base/src/nsEventSource.cpp b/content/base/src/nsEventSource.cpp
>--- a/content/base/src/nsEventSource.cpp
>+++ b/content/base/src/nsEventSource.cpp
>@@ -53,16 +53,17 @@
> #include "jsdbgapi.h"
> #include "nsJSUtils.h"
> #include "nsIAsyncVerifyRedirectCallback.h"
> #include "nsIScriptError.h"
> #include "nsICharsetConverterManager.h"
> #include "nsIChannelPolicy.h"
> #include "nsIContentSecurityPolicy.h"
> #include "mozilla/Preferences.h"
>+#include "nsCrossSiteListenerProxy.h"
> 
> using namespace mozilla;
> 
> #define REPLACEMENT_CHAR     (PRUnichar)0xFFFD
> #define BOM_CHAR             (PRUnichar)0xFEFF
> #define SPACE_CHAR           (PRUnichar)0x0020
> #define CR_CHAR              (PRUnichar)0x000D
> #define LF_CHAR              (PRUnichar)0x000A
>@@ -877,18 +878,23 @@ nsEventSource::InitChannelAndRequestEven
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   mHttpChannel = do_QueryInterface(channel);
>   NS_ENSURE_TRUE(mHttpChannel, NS_ERROR_NO_INTERFACE);
> 
>   rv = SetupHttpChannel();
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>+  nsCOMPtr<nsIStreamListener> listener =
>+    new nsCORSListenerProxy(this, mPrincipal, mHttpChannel, PR_TRUE, &rv);
>+  NS_ENSURE_TRUE(listener, NS_ERROR_OUT_OF_MEMORY);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>   // Start reading from the channel
>-  return mHttpChannel->AsyncOpen(this, nsnull);
>+  return mHttpChannel->AsyncOpen(listener, nsnull);
> }
> 
> void
> nsEventSource::AnnounceConnection()
> {
>   if (mReadyState == nsIEventSource::CLOSED) {
>     return;
>   }
>@@ -1164,26 +1170,23 @@ nsEventSource::FailConnection()
> 
> PRBool
> nsEventSource::CheckCanRequestSrc(nsIURI* aSrc)
> {
>   if (mReadyState == nsIEventSource::CLOSED) {
>     return PR_FALSE;
>   }
> 
>-  PRBool isSameOrigin = PR_FALSE;
>   PRBool isValidURI = PR_FALSE;
>   PRBool isValidContentLoadPolicy = PR_FALSE;
>   PRBool isValidProtocol = PR_FALSE;
> 
>   nsCOMPtr<nsIURI> srcToTest = aSrc ? aSrc : mSrc.get();
>   NS_ENSURE_TRUE(srcToTest, PR_FALSE);
> 
>-  isSameOrigin = NS_SUCCEEDED(mPrincipal->CheckMayLoad(srcToTest, PR_FALSE));
>-
>   PRUint32 aCheckURIFlags =
>     nsIScriptSecurityManager::DISALLOW_INHERIT_PRINCIPAL |
>     nsIScriptSecurityManager::DISALLOW_SCRIPT;
> 
>   nsresult rv = nsContentUtils::GetSecurityManager()->
>     CheckLoadURIWithPrincipal(mPrincipal,
>                               srcToTest,
>                               aCheckURIFlags);
>@@ -1213,18 +1216,17 @@ nsEventSource::CheckCanRequestSrc(nsIURI
>   nsCAutoString targetURIScheme;
>   rv = srcToTest->GetScheme(targetURIScheme);
>   if (NS_SUCCEEDED(rv)) {
>     // We only have the http support for now
>     isValidProtocol = targetURIScheme.EqualsLiteral("http") ||
>                       targetURIScheme.EqualsLiteral("https");
>   }
> 
>-  return isSameOrigin && isValidURI && isValidContentLoadPolicy &&
>-         isValidProtocol;
>+  return isValidURI && isValidContentLoadPolicy && isValidProtocol;
> }
> 
> // static
> void
> nsEventSource::TimerCallback(nsITimer* aTimer, void* aClosure)
> {
>   nsRefPtr<nsEventSource> thisObject = static_cast<nsEventSource*>(aClosure);
> 
>diff --git a/content/base/test/accesscontrol.resource^headers^ b/content/base/test/accesscontrol.resource^headers^
>--- a/content/base/test/accesscontrol.resource^headers^
>+++ b/content/base/test/accesscontrol.resource^headers^
>@@ -1,4 +1,5 @@
>-Access-Control-Allow-Origin: http://localhost:8888
>+Access-Control-Allow-Origin: http://mochi.test:8888
>+Access-Control-Allow-Credentials: true
> Content-Type: text/event-stream
> Cache-Control: no-cache, must-revalidate
> 
>diff --git a/content/base/test/test_bug338583.html b/content/base/test/test_bug338583.html
>--- a/content/base/test/test_bug338583.html
>+++ b/content/base/test/test_bug338583.html
>@@ -27,34 +27,46 @@ https://bugzilla.mozilla.org/show_bug.cg
> //   3) possible invalid eventsources
> //   4) the close method when the object is just been used
> //   5) access-control
> //   6) the data parameter
> //   7) delayed server responses
> 
> // --
> 
>+  var gTestsHaveFinished = [];
>+
>+  function setTestHasFinished(test_id)
>+  {
>+    gTestsHaveFinished[test_id] = true;
>+    for (var i=0; i < gTestsHaveFinished.length; ++i) {
>+      if (!gTestsHaveFinished[i]) {
>+        return;
>+      }
>+    }
>+    SimpleTest.finish();
>+  }
>+
>   function runAllTests() {
>-    // these tests run asynchronously
>-    doTest1();    // this will take 8000 ms
>-    doTest2();    // this will take 5000 ms
>-    doTest3();    // this will take 1500 ms
>-    doTest3_b();  // this will take 1500 ms
>-    doTest3_c();  // this will take 1500 ms
>-    doTest3_d();  // this will take 1500 ms
>-    doTest3_e();  // this will take 1500 ms
>-    doTest3_f();  // this will take 1500 ms
>-    doTest3_g();  // this will take 1500 ms
>-    doTest3_h();  // this will take 1500 ms
>-    doTest4();    // this will take 3000 ms
>-    doTest4_b();  // this will take 3000 ms
>-    doTest5();    // this will take 3000 ms
>-    doTest5_b();  // this will take 3000 ms
>-    doTest6();    // this will take 2500 ms
>-    doTest7();    // this will take 8000 ms
>+    // these tests run asynchronously, and they will take 8000 ms
>+    var all_tests = [
>+      doTest1, doTest2, doTest3, doTest3_b, doTest3_c, doTest3_d,
>+      doTest3_e, doTest3_f, doTest3_g, doTest3_h, doTest4, doTest4_b,
>+      doTest5, doTest5_b, doTest6, doTest7
>+    ];
>+    for (var test_id=0; test_id < all_tests.length; ++test_id) {
>+      gTestsHaveFinished[test_id] = false;
>+      var fn = all_tests[test_id];
>+      fn(test_id);
>+      setTimeout(new Function(
>+        "if (!gTestsHaveFinished[" + test_id + "]) { " +
>+          "ok(false, 'Test " + test_id + " took too long'); " +
>+          "setTestHasFinished(" + test_id + "); " +
>+        "}"), 15000);
>+    }
>   }
> 
>   function fn_onmessage(e) {
>     if (e.currentTarget == e.target && e.target.hits != null)
>       e.target.hits['fn_onmessage']++;
>   }
> 
>   function fn_event_listener_message(e) {
>@@ -92,204 +104,214 @@ https://bugzilla.mozilla.org/show_bug.cg
>   }
> 
> // in order to test (1):
> //   a) if the EventSource constructor parameter is equal to its url attribute
> //   b) let its fn_onmessage, fn_event_listener_message, and fn_other_event_name functions listeners be hit four times each
> //   c) the close method (we expect readyState == CLOSED)
> //   d) the close method (we expect no message events anymore)
> 
>-  function doTest1() {
>+  function doTest1(test_id) {
>     gEventSourceObj1 = new EventSource("eventsource.resource");
>     ok(gEventSourceObj1.url == "http://mochi.test:8888/tests/content/base/test/eventsource.resource", "Test 1.a failed.");
>     ok(gEventSourceObj1.readyState == 0 || gEventSourceObj1.readyState == 1, "Test 1.a failed.");
> 
>-    doTest1_b();
>+    doTest1_b(test_id);
>   }
> 
>-  function doTest1_b() {
>+  function doTest1_b(test_id) {
>     gEventSourceObj1.hits = [];
>     gEventSourceObj1.hits['fn_onmessage'] = 0;
>     gEventSourceObj1.onmessage = fn_onmessage;
>     gEventSourceObj1.hits['fn_event_listener_message'] = 0;
>     gEventSourceObj1.addEventListener('message', fn_event_listener_message, true);
>     gEventSourceObj1.hits['fn_other_event_name'] = 0;
>     gEventSourceObj1.addEventListener('other_event_name', fn_other_event_name, true);
> 
>     // the eventsources.res always use a retry of 0.5 second, so for four hits a timeout of 6 seconds is enough
>     setTimeout(function(){
>       bhits = hasBeenHitFor1And2(gEventSourceObj1, 4);
>       ok(bhits, "Test 1.b failed.");
> 
>-      doTest1_c();
>+      doTest1_c(test_id);
>     }, parseInt(6000*stress_factor));
>   }
> 
>-  function doTest1_c() {
>+  function doTest1_c(test_id) {
>     gEventSourceObj1.close();
>     ok(gEventSourceObj1.readyState == 2, "Test 1.c failed.");
> 
>-    doTest1_d();
>+    doTest1_d(test_id);
>   }
> 
>-  function doTest1_d() {
>+  function doTest1_d(test_id) {
>     gEventSourceObj1.hits['fn_onmessage'] = 0;
>     gEventSourceObj1.hits['fn_event_listener_message'] = 0;
>     gEventSourceObj1.hits['fn_other_event_name'] = 0;
> 
>     setTimeout(function(){
>       bhits = hasBeenHitFor1And2(gEventSourceObj1, 1);
>       ok(!bhits, "Test 1.d failed.");
>       gEventSourceObj1.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(2000*stress_factor));
>   }
> 
> // in order to test (2)
> //   a) set a eventsource that give the dom events messages
> //   b) expect trusted events
> 
>-  function doTest2() {
>+  function doTest2(test_id) {
>     var func = function(e) {
>       ok(e.isTrusted, "Test 2 failed");
>       gEventSourceObj2.close();
>     };
> 
>     gEventSourceObj2 = new EventSource("eventsource.resource");
>     gEventSourceObj2.onmessage = func;
> 
>-    setTimeout(function(){  // just to clean...
>+    setTimeout(function() {  // just to clean...
>       gEventSourceObj2.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(5000*stress_factor));
>   }
> 
> // in order to test (3)
> //   a) XSite domain error test
> //   b) protocol file:// test
> //   c) protocol javascript: test
> //   d) wrong Content-Type test
> //   e) bad http response code test
> //   f) message eventsource without a data test
> //   g) eventsource with invalid NCName char in the event field test
> //   h) DNS error
> 
>-  function doTest3() {
>+  function doTest3(test_id) {
>     gEventSourceObj3_a = new EventSource("http://example.org/tests/content/base/test/eventsource.resource");
> 
>     gEventSourceObj3_a.onmessage = fn_onmessage;
>     gEventSourceObj3_a.hits = [];
>     gEventSourceObj3_a.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_a.hits['fn_onmessage'] == 0, "Test 3.a failed");
>       gEventSourceObj3_a.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>-  function doTest3_b() {
>+  function doTest3_b(test_id) {
>     var xhr = new XMLHttpRequest;
>     xhr.open("GET", "/dynamic/getMyDirectory.sjs", false);
>     xhr.send();
>     var basePath = xhr.responseText;
> 
>     gEventSourceObj3_b = new EventSource("file://" + basePath + "eventsource.resource");
> 
>     gEventSourceObj3_b.onmessage = fn_onmessage;
>     gEventSourceObj3_b.hits = [];
>     gEventSourceObj3_b.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_b.hits['fn_onmessage'] == 0, "Test 3.b failed");
>       gEventSourceObj3_b.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>   function jsEvtSource()
>   {
>     return "event: message\n" +
>            "data: 1\n\n";
>   }
> 
>-  function doTest3_c() {
>+  function doTest3_c(test_id) {
>     gEventSourceObj3_c = new EventSource("javascript: return jsEvtSource()");
> 
>     gEventSourceObj3_c.onmessage = fn_onmessage;
>     gEventSourceObj3_c.hits = [];
>     gEventSourceObj3_c.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_c.hits['fn_onmessage'] == 0, "Test 3.c failed");
>       gEventSourceObj3_c.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>-  function doTest3_d() {
>+  function doTest3_d(test_id) {
>     gEventSourceObj3_d = new EventSource("badContentType.eventsource");
> 
>     gEventSourceObj3_d.onmessage = fn_onmessage;
>     gEventSourceObj3_d.hits = [];
>     gEventSourceObj3_d.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_d.hits['fn_onmessage'] == 0, "Test 3.d failed");
>       gEventSourceObj3_d.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>-  function doTest3_e() {
>+  function doTest3_e(test_id) {
>     gEventSourceObj3_e = new EventSource("badHTTPResponseCode.eventsource");
> 
>     gEventSourceObj3_e.onmessage = fn_onmessage;
>     gEventSourceObj3_e.hits = [];
>     gEventSourceObj3_e.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_e.hits['fn_onmessage'] == 0, "Test 3.e failed");
>       gEventSourceObj3_e.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>-  function doTest3_f() {
>+  function doTest3_f(test_id) {
>     gEventSourceObj3_f = new EventSource("badMessageEvent.eventsource");
> 
>     gEventSourceObj3_f.onmessage = fn_onmessage;
>     gEventSourceObj3_f.hits = [];
>     gEventSourceObj3_f.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_f.hits['fn_onmessage'] == 0, "Test 3.f failed");
>       gEventSourceObj3_f.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>   function fnInvalidNCName() {
>     fnInvalidNCName.hits++;
>   }
> 
>-  function doTest3_g() {
>+  function doTest3_g(test_id) {
>     gEventSourceObj3_g = new EventSource("badEventFieldName.eventsource");
> 
>     fnInvalidNCName.hits = 0;
>     gEventSourceObj3_g.addEventListener('message event', fnInvalidNCName, true);
> 
>     setTimeout(function() {
>       ok(fnInvalidNCName.hits != 0, "Test 3.g failed");
>       gEventSourceObj3_g.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
>-  function doTest3_h() {
>+  function doTest3_h(test_id) {
>     gEventSourceObj3_h = new EventSource("http://hdfskjghsbg.jtiyoejowe.dafsgbhjab.com");
> 
>     gEventSourceObj3_h.onmessage = fn_onmessage;
>     gEventSourceObj3_h.hits = [];
>     gEventSourceObj3_h.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj3_h.hits['fn_onmessage'] == 0, "Test 3.h failed");
>       gEventSourceObj3_h.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(1500*stress_factor));
>   }
> 
> // in order to test (4)
> //   a) close the object when it is in use, which is being processed and that is expected
> //      to dispatch more eventlisteners
> //   b) remove an eventlistener in use
> 
>@@ -304,72 +326,76 @@ https://bugzilla.mozilla.org/show_bug.cg
>   function fn_onmessage4_b(e)
>   {
>     if (e.data > gEventSourceObj4_b.lastData)
>       gEventSourceObj4_b.lastData = e.data;
>     if (e.data == 2)
>       gEventSourceObj4_b.removeEventListener('message', fn_onmessage4_b, true);
>   }
> 
>-  function doTest4() {
>+  function doTest4(test_id) {
>     gEventSourceObj4_a = new EventSource("forRemoval.resource");
>     gEventSourceObj4_a.lastData = 0;
>     gEventSourceObj4_a.onmessage = fn_onmessage4_a;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj4_a.lastData == 2, "Test 4.a failed");
>       gEventSourceObj4_a.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(3000*stress_factor));
>   }
> 
>-  function doTest4_b()
>+  function doTest4_b(test_id)
>   {
>     gEventSourceObj4_b = new EventSource("forRemoval.resource");
>     gEventSourceObj4_b.lastData = 0;
>     gEventSourceObj4_b.addEventListener('message', fn_onmessage4_b, true);
> 
>     setTimeout(function() {
>       ok(gEventSourceObj4_b.lastData == 2, "Test 4.b failed");
>       gEventSourceObj4_b.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(3000*stress_factor));
>   }
> 
> // in order to test (5)
>-//   a) valid access-control xsite request (but must fail)
>+//   a) valid access-control xsite request
> //   b) invalid access-control xsite request
> 
>-  function doTest5()
>+  function doTest5(test_id)
>   {
>     gEventSourceObj5_a = new EventSource("http://example.org/tests/content/base/test/accesscontrol.resource");
> 
>     gEventSourceObj5_a.onmessage = fn_onmessage;
>     gEventSourceObj5_a.hits = [];
>     gEventSourceObj5_a.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>-      ok(gEventSourceObj5_a.hits['fn_onmessage'] == 0, "Test 5.a failed");
>+      ok(gEventSourceObj5_a.hits['fn_onmessage'] != 0, "Test 5.a failed");
>       gEventSourceObj5_a.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(3000*stress_factor));
>   }
> 
>-  function doTest5_b()
>+  function doTest5_b(test_id)
>   {
>     gEventSourceObj5_b = new EventSource("http://example.org/tests/content/base/test/invalid_accesscontrol.resource");
> 
>     gEventSourceObj5_b.onmessage = fn_onmessage;
>     gEventSourceObj5_b.hits = [];
>     gEventSourceObj5_b.hits['fn_onmessage'] = 0;
> 
>     setTimeout(function() {
>       ok(gEventSourceObj5_b.hits['fn_onmessage'] == 0, "Test 5.b failed");
>       gEventSourceObj5_b.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(3000*stress_factor));
>   }
> 
>-  function doTest6()
>+  function doTest6(test_id)
>   {
>     gEventSourceObj6 = new EventSource("somedatas.resource");
>     var fn_somedata = function(e) {
>       if (fn_somedata.expected == 0) {
>         ok(e.data == "123456789\n123456789123456789\n123456789123456789123456789123456789\n 123456789123456789123456789123456789123456789123456789123456789123456789\nçãá\"\'@`~Ý Ḿyyyy",
>           "Test 6.a failed");
>       } else if (fn_somedata.expected == 1) {
>         ok(e.data == " :xxabcdefghij\nçãá\"\'@`~Ý Ḿyyyy : zz",
>@@ -382,20 +408,21 @@ https://bugzilla.mozilla.org/show_bug.cg
>       }
>       fn_somedata.expected++;
>     }
>     fn_somedata.expected = 0;
>     gEventSourceObj6.onmessage = fn_somedata;
> 
>     setTimeout(function() {
>       gEventSourceObj6.close();
>+      setTestHasFinished(test_id);
>     }, parseInt(2500*stress_factor));
>   }
> 
>-  function doTest7()
>+  function doTest7(test_id)
>   {
>     gEventSourceObj7 = new EventSource("delayedServerEvents.sjs");
>     gEventSourceObj7.msg_received = [];
>     gEventSourceObj7.onmessage = function(e)
>     {
>       e.target.msg_received.push(e.data);
>     }
>     
>@@ -403,21 +430,21 @@ https://bugzilla.mozilla.org/show_bug.cg
>       gEventSourceObj7.close();
>       
>       ok(gEventSourceObj7.msg_received[0] == "" &&
>          gEventSourceObj7.msg_received[1] == "delayed1" &&
>          gEventSourceObj7.msg_received[2] == "delayed2", "Test 7 failed");
> 
>       SpecialPowers.setBoolPref("dom.server-events.enabled", oldPrefVal);
>       document.getElementById('waitSpan').innerHTML = '';
>-      SimpleTest.finish();
>+      setTestHasFinished(test_id);
>     }, parseInt(8000*stress_factor));
>   }
> 
>-  function doTest()
>+  function doTest(test_id)
>   {
>     oldPrefVal = SpecialPowers.getBoolPref("dom.server-events.enabled");
>     SpecialPowers.setBoolPref("dom.server-events.enabled", true);
> 
>     // we get a good stress_factor by testing 10 setTimeouts and some float
>     // arithmetic taking my machine as stress_factor==1 (time=589)
> 
>     var begin_time = (new Date()).getTime();
Comment 12 Olli Pettay [:smaug] 2011-06-27 07:09:38 PDT
Comment on attachment 542022 [details] [diff] [review]
patch v1


>+  nsCOMPtr<nsIStreamListener> listener =
>+    new nsCORSListenerProxy(this, mPrincipal, mHttpChannel, PR_TRUE, &rv);
>+  NS_ENSURE_TRUE(listener, NS_ERROR_OUT_OF_MEMORY);
No need for OOM check.
Comment 13 Olli Pettay [:smaug] 2011-06-27 10:12:49 PDT
We must check that how mixed mode UI works with this.
Comment 14 Wellington Fernando de Macedo 2011-06-28 04:03:37 PDT
Hmm, I think the message events are using a wrong origin. Instead of using the event stream url origin they are being dispatched with the script principal origin.

Olli, what is the mixed mode UI?
Comment 15 Olli Pettay [:smaug] 2011-06-28 04:33:42 PDT
I mean the case when some part of the page is using https and some
part is using http.
Comment 16 Jonas Sicking (:sicking) 2011-06-28 09:47:20 PDT
Comment on attachment 542022 [details] [diff] [review]
patch v1

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

::: content/base/src/nsEventSource.cpp
@@ +883,5 @@
>    rv = SetupHttpChannel();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCOMPtr<nsIStreamListener> listener =
> +    new nsCORSListenerProxy(this, mPrincipal, mHttpChannel, PR_TRUE, &rv);

I actually don't think that we should be passing PR_TRUE here by default. I know Hixie agrees with me, but this is a matter of security so this is something that we need to decide within mozilla what we're comfortable with.

Requests with cookies is a significantly more complex security model and one that people should only use if really needed. There's already been people speaking up on the list saying that they have products which use cookie-less requests. It would IMHO be much better if we could let them use the simpler and safer security model of not using cookies.

So I think we should add a constructor argument.

Alternatively, we should do a mozilla security review and decide there if this security default is ok.
Comment 17 Olli Pettay [:smaug] 2011-06-28 09:53:31 PDT
> I actually don't think that we should be passing PR_TRUE here by default. I
> know Hixie agrees with me, 
You mean disagrees, right?
Comment 18 Jonas Sicking (:sicking) 2011-06-28 09:57:53 PDT
Yes, sorry, that should say:

I actually don't think that we should be passing PR_TRUE here by default. I know Hixie disagrees with me, but this is a matter of security so this is something that we need to decide within mozilla what we're comfortable with.
Comment 19 Olli Pettay [:smaug] 2011-06-28 10:00:51 PDT
And I agree with you. I think EventSource should default to the same what
XMLHttpRequest has, by default.
Comment 20 Ian Melven :imelven 2011-06-28 10:08:07 PDT
FWIW, i also agree with you both. i thought this was supposed to be 'XHR like' in terms of security so i don't understand why the spec'd default would be to always pass cookies. perhaps to be consistent with websockets which always sends cookies during the HTTP handshake but i think it's much better to keep EventSource consistent with XHR.
Comment 21 Jonas Sicking (:sicking) 2011-06-28 10:41:26 PDT
Given that it seems like most people agree (and thus that'd likely be the way the security review would go), should we just go with adding the constructor argument and push back against the spec?
Comment 22 Olli Pettay [:smaug] 2011-06-28 10:45:28 PDT
I'd be ok with that. The parameter should be optional, and false by default.
Comment 23 Daniel Veditz [:dveditz] 2011-06-28 11:40:28 PDT
I agree with this plan, but it's incompatible with a Last Call spec. Do we now need to moz-prefix the constructor for this feature?
Comment 24 Hixie (not reading bugmail) 2011-06-28 14:48:33 PDT
(In reply to comment #16)
> There's already been people
> speaking up on the list saying that they have products which use cookie-less
> requests.

One person, as far as I'm aware, and their system only doesn't use cookies because they send the user's ID using a different mechanism.

What's the security risk here?
Comment 25 Jonas Sicking (:sicking) 2011-06-28 14:55:50 PDT
The security risk is that people opt in to enabling cookie-enabled transfers on a too big set of URIs on their site, thus exposing sensitive personalized data.
Comment 26 Jonas Sicking (:sicking) 2011-06-28 15:02:15 PDT
I'm not entirely sure how to prefix other than prefixing all of EventSource which would be unfortunate since we have released an unprefixed EventSource (which at the time was up-to-date with the spec) in previous releases.

I'd rather see us trying to get the spec changed, especially given that it seems like we have no intention of implementing the spec as written at all (i.e. it's not just a matter of limited time)
Comment 27 Hixie (not reading bugmail) 2011-06-28 15:08:13 PDT
You're concerned that sites will have cookie-enabled text/event-source resources that they don't intend to have available cross-site but that send back an explicit opt-in header that echoes the origin in the request?

If that's the attack, I don't understand how a flag under the control of the attacker that decides whether or not to send cookies is going to help.

Can you elaborate?
Comment 28 Jonas Sicking (:sicking) 2011-06-28 15:21:31 PDT
Sigh, this is the exact same discussion as we had for cross site XHR. But ok.

The goal is that people that only need to share cookie-less data can do that without risk of accidentally sharing personalized cookie-based data.

CORS allows that to be done in a very safe and easy manner, by simply adding a "access-control-allow-origin:*" header. The nice thing about that header is that it can be essentially added on a site-wide basis without the risk of leaking any sensitive data.

We want people to choose that option if they are able to, precisely because it's so easy to do without risking leaking other data.

If people that want to share cookie-less data are forced to enable cookies, then there's a risk that they'll do so on too large set of URIs, accidentally leaking private data.


On top of that, aligning the XHR security model with the EventSource one makes a lot of sense since EventSource is basically just a parsing library on top of XHR. It's somewhat surprising that using a parsing library all of a sudden forces you to use a different security model. Especially one that's more complex.

It'd be bad if people end up enabling cookies just because they want the convenience of using EventSource. It'd also be bad if people use XHR just so that they can get the simpler security model, when EventSource would otherwise be a good fit for them.
Comment 29 Hixie (not reading bugmail) 2011-06-28 15:36:50 PDT
So I agree with all that, except that you'll basically never want to use EventSource for anything _but_ sensitive data. The whole point of the feature is to get user-specific data. (Even in the guy who mentioned not using cookies said he used user-specific data, he just used hand-rolled cookies in the URL instead of actual cookies, for some reason.)

The security model here is the same as XHR's. It literally uses the same spec to define it. It's just that one of the knobs is fixed in one position. XHR has a much bigger set of use cases; EventSource only works with one specific MIME type.

Note that you can't just set "access-control-allow-origin:*" and opt an entire server in to cross-origin cookies. That header only works when you have cookies disabled, and so would never work with EventSource. The simplest way of exposing an EventSource stream in this way is just to have the code that creates the event stream be the one that sends back the header — it's very different from XHR, when many of the resources (even the personalised ones) will be static. There's much less motivation to use a site-wide opt-in that could screw up with EventSource resources than there is with XHR.

Is there any evidence to suggest that sufficient numbers of people will want to use EventSource with completely user-nonspecific data to make it worth complicating the API for the people who want to use it with cookies? (If most people use EventSource with cookies, then what you're arguing for doesn't help many people, since the majority will have to enable cookies anyway.)
Comment 30 Jonas Sicking (:sicking) 2011-06-28 17:55:03 PDT
(In reply to comment #29)
> So I agree with all that, except that you'll basically never want to use
> EventSource for anything _but_ sensitive data.

Even if we agree that the majority use case is sensitive data, no one is suggesting not supporting that use case. I'm simply suggesting that that should be something you explicitly opt in to.

Additionally, what's the basis cross-site EventSource being so much more commonly sensitive data than cross-site XHR?

> Note that you can't just set "access-control-allow-origin:*" and opt an
> entire server in to cross-origin cookies. That header only works when you
> have cookies disabled, and so would never work with EventSource.

In your suggested API yes. Not in mine.

> There's much less motivation to use a
> site-wide opt-in that could screw up with EventSource resources than there
> is with XHR.

I don't understand this.

> Is there any evidence to suggest that sufficient numbers of people will want
> to use EventSource with completely user-nonspecific data to make it worth
> complicating the API for the people who want to use it with cookies? (If
> most people use EventSource with cookies, then what you're arguing for
> doesn't help many people, since the majority will have to enable cookies
> anyway.)

I'm aware that it might not help the majority of people. That's quite possibly the case with cross-site XHR too. I'm trying to help as many people as I can.
Comment 31 Hixie (not reading bugmail) 2011-06-29 15:22:53 PDT
> Additionally, what's the basis cross-site EventSource being so much more
> commonly sensitive data than cross-site XHR?

XHR is used for a wide variety of purposes; it's a generic HTTP mechanism. So it's used for grabbing public scripts, for grabbing public state information, for communicating with a server in a request/response mode on behalf of the user, etc. Some of these are for private data, some are for public data.

EventSource is only useful for one thing: a stream of notifications from the server. This is almost always user-specific. Even public data like news or financial markets will tend to be personalised.


> > Note that you can't just set "access-control-allow-origin:*" and opt an
> > entire server in to cross-origin cookies. That header only works when you
> > have cookies disabled, and so would never work with EventSource.
> 
> In your suggested API yes. Not in mine.

I'm talking about CORS as specified here. The problem scenario you describe, where a server administrator accidentally opts an entire server into cross-origin requests using a static non-CORS-aware configuration by adding one header to all pages, can only happen in CORS today if cookies aren't sent. In a situation where cookies are sent, you can't do it that easily.

 
> > There's much less motivation to use a
> > site-wide opt-in that could screw up with EventSource resources than there
> > is with XHR.
> 
> I don't understand this.

Since the simple site-wide opt-in doesn't work with cookie-sending CORS requests, if EventSource requests send cookies, then EventSource would not motivate people to do the simple site-wide opt-in. With XHR, since there are many use cases that don't involve cookies, and since the API therefore supports not sending cookies, there is incentive to do the site-wide opt-in, but that doesn't apply in the proposed EventSource case.


> I'm aware that it might not help the majority of people. That's quite
> possibly the case with cross-site XHR too. I'm trying to help as many people
> as I can.

The problem is that complicating APIs also hurts people. I argue that in the case of EventSource it hurts more people than it helps.

That is, more people will be confused by the more complicated API than will be helped by us not sending cookies by default. I base this on the assumption that most requests will want cookies, therefore the API will merely get in the way of most people without helping them. So the number of people helped will be small, and the number of people hurt will be big.

(I agree that with XHR the tradeoffs are different, I'm not arguing that we should change XHR.)
Comment 32 Jonas Sicking (:sicking) 2011-06-29 17:47:10 PDT
(In reply to comment #31)
> > Additionally, what's the basis cross-site EventSource being so much more
> > commonly sensitive data than cross-site XHR?
> 
> XHR is used for a wide variety of purposes; it's a generic HTTP mechanism.
> So it's used for grabbing public scripts, for grabbing public state
> information, for communicating with a server in a request/response mode on
> behalf of the user, etc. Some of these are for private data, some are for
> public data.
> 
> EventSource is only useful for one thing: a stream of notifications from the
> server. This is almost always user-specific. Even public data like news or
> financial markets will tend to be personalised.

I'd like to see data backing this up. Especially since you're arguing that we should go with a less secure API.

> I'm talking about CORS as specified here. The problem scenario you describe,
> where a server administrator accidentally opts an entire server into
> cross-origin requests using a static non-CORS-aware configuration by adding
> one header to all pages, can only happen in CORS today if cookies aren't
> sent. In a situation where cookies are sent, you can't do it that easily.

I agree that it's much harder, but I'd be surprised if it'll never happen. Especially in scenarios where the data that people intend to share is not security sensitive and so will receive much less security review.


> > I'm aware that it might not help the majority of people. That's quite
> > possibly the case with cross-site XHR too. I'm trying to help as many people
> > as I can.
> 
> The problem is that complicating APIs also hurts people. I argue that in the
> case of EventSource it hurts more people than it helps.

I'd much rather that they hurt in the sense of "Why doesn't this work" than "Opps, we just shared all customer data with the world". Especially since the former is much more easily detected and fixed.

> That is, more people will be confused by the more complicated API than will
> be helped by us not sending cookies by default. I base this on the
> assumption that most requests will want cookies, therefore the API will
> merely get in the way of most people without helping them. So the number of
> people helped will be small, and the number of people hurt will be big.

But you have to multiply those factors by the amount of hurt involved with adding ", true" to the code, compared to the hurt of accidentally leaking data.

Ultimately I suspect you and I won't be able to convince each other. One way or another we should have a security review of this (which will need to happen before this ships). You're more than welcome to call in to that review.
Comment 33 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-07-13 11:24:50 PDT
sec review occurred on 2011.07.12 https://wiki.mozilla.org/Security/Reviews/CrossOriginEventSource#Introduce_Feature

marking sec-review-complete
Comment 34 Wellington Fernando de Macedo 2011-07-17 19:17:42 PDT
Created attachment 546453 [details] [diff] [review]
patch v2
Comment 35 Wellington Fernando de Macedo 2011-07-17 19:23:40 PDT
Created attachment 546454 [details] [diff] [review]
patch v2
Comment 36 Jonas Sicking (:sicking) 2011-07-17 19:26:41 PDT
I think I would prefer to use syntax like:

new EventSource(url, { withCredentials: true });

This is both more clear than a plain boolean argument, and is more future proof in case we need to add further arguments.

(In general functions that take plain boolean arguments are hard to decipher)
Comment 37 Wellington Fernando de Macedo 2011-07-17 20:26:56 PDT
if second argument is invalid, should the constructor fail and raise an exception, or should continue with withCredentials=false?
Comment 38 Wellington Fernando de Macedo 2011-07-17 20:27:20 PDT
if the second argument is invalid, should the constructor fail and raise an exception, or should continue with withCredentials=false?
Comment 39 Jonas Sicking (:sicking) 2011-07-17 20:43:35 PDT
What do you mean by "invalid"? If it's something other than an object?

If it's something other than an object I suspect we should throw yes.

If it's an object with properties other than withCredentials, we should ignore those options. This goes with the WebIDL "dictionary" feature.
Comment 40 Wellington Fernando de Macedo 2011-07-17 20:54:45 PDT
> If it's something other than an object I suspect we should throw yes.
Yes, that I meant. Thanks.
Comment 41 Wellington Fernando de Macedo 2011-07-17 20:59:08 PDT
Created attachment 546458 [details] [diff] [review]
patch v3
Comment 42 Wellington Fernando de Macedo 2011-07-17 21:37:04 PDT
Created attachment 546463 [details] [diff] [review]
patch v3

A fix. This is the last one :)
Comment 43 Olli Pettay [:smaug] 2011-07-18 04:33:33 PDT
Comment on attachment 546463 [details] [diff] [review]
patch v3


>-  return Init(principal, scriptContext, ownerWindow, urlParam);
>+  PRBool withCredentialsParam = PR_FALSE;
>+  if (aArgc >= 2) {
>+    JSObject *obj;
>+    if (JS_TypeOfValue(aContext, aArgv[1]) != JSTYPE_OBJECT ||
>+        !JS_ValueToObject(aContext, aArgv[1], &obj)) {
>+      return NS_ERROR_INVALID_ARG;
>+    }tru
>+
>+    JSBool hasProperty = JS_FALSE;
>+    jsval withCredentialsVal;
>+    if (JS_HasProperty(aContext, obj, "withCredentials", &hasProperty) &&
>+        hasProperty &&
>+        JS_GetProperty(aContext, obj, "withCredentials", &withCredentialsVal)) {
>+      JSBool withCredentials = JS_FALSE;
>+      if (JS_TypeOfValue(aContext, withCredentialsVal) != JSTYPE_BOOLEAN ||
>+          !JS_ValueToBoolean(aContext, withCredentialsVal, &withCredentials)) {
>+        return NS_ERROR_INVALID_ARG;
Does this work if withCredentials is a getter?

Would be good to have a testcase for { get withCredentials() { return true; } }

Also, I'm not sure about the explicit type check. Other values should be converted to
boolean.

I think EventSource could have readonly attribute withCredentials so that one could check
afterwards whether credentials were used for connection.
Comment 44 :Ms2ger 2011-07-18 06:16:51 PDT
I'd think something more like

PRBool withCredentialsParam = PR_FALSE;
if (aArgc >= 2) {
  JSObject *obj;
  NS_ENSURE_TRUE(JS_ValueToObject(aContext, aArgv[1], &obj), NS_ERROR_FAILURE);

  JSBool hasProperty = JS_FALSE;
  NS_ENSURE_TRUE(JS_HasProperty(aContext, obj, "withCredentials", &hasProperty), NS_ERROR_FAILURE);

  if (hasProperty) {
    jsval withCredentialsVal;
    NS_ENSURE_TRUE(JS_GetProperty(aContext, obj, "withCredentials", &withCredentialsVal), NS_ERROR_FAILURE);
    JSBool withCredentials = JS_FALSE;
    NS_ENSURE_TRUE(JS_ValueToBoolean(aContext, withCredentialsVal, &withCredentials), NS_ERROR_FAILURE);
    withCredentialsParam = !!withCredentials;
  }
}

JS_GetProperty does call getters; but would be good to test indeed.
Comment 45 Olli Pettay [:smaug] 2011-07-18 06:24:43 PDT
Anne suggested adding .withCredentials attribute to the EventSource interface.
If that was set right after ctor, the connection would use credentials.

I think I prefer that approach, since that is closer to what XHR has.
Comment 46 Olli Pettay [:smaug] 2011-07-18 06:25:58 PDT
But Jonas may have other opinion.
Comment 47 Jonas Sicking (:sicking) 2011-07-18 08:17:50 PDT
Define "right after". The only way to do that that I can think of means waiting to start opening the connection until we get back to the event loop. This can be a non-trivial amount of time, especially in workers.

I agree that aligning with XHR is good, but it uses a significantly different API in that it has an explicit .open call which means that it's pretty different in general.
Comment 48 Olli Pettay [:smaug] 2011-07-18 10:40:13 PDT
(In reply to comment #47)
> Define "right after". The only way to do that that I can think of means
> waiting to start opening the connection until we get back to the event loop.
> This can be a non-trivial amount of time, especially in workers.
"Right after" is after calling new EventSource() in the same task.
Per spec network connection is opened after returning EventSource object to the caller.

 
> I agree that aligning with XHR is good, but it uses a significantly
> different API in that it has an explicit .open call which means that it's
> pretty different in general.
I agree. But IMO we need at least readonly withCredentials, so non-readonly property could make sense.
Comment 49 Jonas Sicking (:sicking) 2011-07-18 10:54:39 PDT
(In reply to comment #48)
> (In reply to comment #47)
> > Define "right after". The only way to do that that I can think of means
> > waiting to start opening the connection until we get back to the event loop.
> > This can be a non-trivial amount of time, especially in workers.
> "Right after" is after calling new EventSource() in the same task.
> Per spec network connection is opened after returning EventSource object to
> the caller.

Right, "in the same task" means "before returning to the event loop", right? So we wouldn't be able to start the network connection until the page returns to the event loop since it could set .withCredentials at any point before then.

And returning to the event loop can take a long time in workers where the whole point is not having to return to the event loop that often.
Comment 50 Olli Pettay [:smaug] 2011-07-18 11:16:31 PDT
I don't understand how workers are related to this at all.
Per spec 'new EventSource' returns before the connection has been opened.
var es = new EventSource("foobar");
es.withCredentials = true;
should work just fine.
Comment 51 Jonas Sicking (:sicking) 2011-07-18 11:24:52 PDT
When are we opening the connection today?

When are you proposing that we open the connection under the .withCredentials proposal?

By "open connection" I mean call AsyncOpen on the channel.
Comment 52 Hixie (not reading bugmail) 2011-07-18 11:30:24 PDT
I agree with sicking that .withCredentials wouldn't work because workers don't return to the event loop promptly enough. I really don't like arguments that are objects, but I agree that they're a hair better than a boolean argument, and I have no other suggestion for how to convey a boolean to the constructor, so I guess if you insist on allowing authors to turn off credentials here then that's the way to go, even though it's not consistent with XHR's API.

If this ends up in the spec it'll be specified as a parameter whose value is a WebIDL dictionary type with one boolean attribute 'withCredentials' whose default value is false.
Comment 53 Wellington Fernando de Macedo 2011-07-18 11:47:47 PDT
> And returning to the event loop can take a long time in workers where the 
> whole point is not having to return to the event loop that often.
Yes, I agree with sicking. The .withCredentials proposal IMO would work only if EventSource had a open/send (or something named like that) method like XHR.
Comment 54 Wellington Fernando de Macedo 2011-07-18 11:51:35 PDT
BTW, a open or start() method in the EventSource API IMHO wouldn't be so ugly :)
Comment 55 Olli Pettay [:smaug] 2011-07-18 11:53:37 PDT
Ok. { withCredentials: true } to the ctor is ok to me.

(es.withCredentials was just something Anne suggested, and I thought 
it could (and indeed it would) work ok.
I still don't understand what the withCredentials attribute handling
has to do with workers)
Comment 56 Jonas Sicking (:sicking) 2011-07-18 14:41:58 PDT
Comment on attachment 546463 [details] [diff] [review]
patch v3

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

r=me with those changes. Though do make sure that Olli is happy with the patch too.

::: content/base/src/nsEventSource.cpp
@@ +290,5 @@
>    rv = os->AddObserver(this, DOM_WINDOW_THAWED_TOPIC, PR_TRUE);
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsCAutoString origin;
> +  rv = nsContentUtils::GetASCIIOrigin(srcURI, origin);

Why this change?

@@ +378,5 @@
>  
> +  PRBool withCredentialsParam = PR_FALSE;
> +  if (aArgc >= 2) {
> +    JSObject *obj;
> +    if (JS_TypeOfValue(aContext, aArgv[1]) != JSTYPE_OBJECT ||

Use !JSVAL_IS_PRIMITIVE(aArgv[1]) instead. That will also give the proper handling for null.

And make sure that you add a test for
new EventSource("...", null);

@@ +379,5 @@
> +  PRBool withCredentialsParam = PR_FALSE;
> +  if (aArgc >= 2) {
> +    JSObject *obj;
> +    if (JS_TypeOfValue(aContext, aArgv[1]) != JSTYPE_OBJECT ||
> +        !JS_ValueToObject(aContext, aArgv[1], &obj)) {

Use JSVAL_TO_OBJECT. The above does more work than you want in a few situations.

@@ +389,5 @@
> +    if (JS_HasProperty(aContext, obj, "withCredentials", &hasProperty) &&
> +        hasProperty &&
> +        JS_GetProperty(aContext, obj, "withCredentials", &withCredentialsVal)) {
> +      JSBool withCredentials = JS_FALSE;
> +      if (JS_TypeOfValue(aContext, withCredentialsVal) != JSTYPE_BOOLEAN ||

Don't do this test. Just call JS_ValueToBoolean and that'll convert anything to a boolean which is what we want here.
Comment 57 :Ms2ger 2011-07-19 02:32:33 PDT
(In reply to comment #56)
> @@ +379,5 @@
> > +  PRBool withCredentialsParam = PR_FALSE;
> > +  if (aArgc >= 2) {
> > +    JSObject *obj;
> > +    if (JS_TypeOfValue(aContext, aArgv[1]) != JSTYPE_OBJECT ||
> > +        !JS_ValueToObject(aContext, aArgv[1], &obj)) {
> 
> Use JSVAL_TO_OBJECT. The above does more work than you want in a few
> situations.

Note that the spec requires ToObject.
Comment 58 Jonas Sicking (:sicking) 2011-07-19 09:17:54 PDT
I don't know what you mean by that, or what cases you are worried that my suggestion would get wrong?
Comment 59 Wellington Fernando de Macedo 2011-07-19 10:34:00 PDT
>> +  rv = nsContentUtils::GetASCIIOrigin(srcURI, origin);
> Why this change?

See comment #14 and bug 670687.
Comment 60 Wellington Fernando de Macedo 2011-07-19 18:03:57 PDT
Created attachment 546956 [details] [diff] [review]
patch v4
Comment 61 Olli Pettay [:smaug] 2011-07-20 02:48:27 PDT
Comment on attachment 546956 [details] [diff] [review]
patch v4


>+  // if true then cross-site Access-Control requests are made using credentials
>+  // such as cookies and authorization headers. Never affects same-site
>+  // requests.
>+  readonly attribute boolean withCredentials;
I guess this should be mozWithCredentials
until it is spec'ed.


>@@ -281,27 +293,27 @@ nsEventSource::Init(nsIPrincipal* aPrinc
> 
>   rv = os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, PR_TRUE);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = os->AddObserver(this, DOM_WINDOW_FROZEN_TOPIC, PR_TRUE);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = os->AddObserver(this, DOM_WINDOW_THAWED_TOPIC, PR_TRUE);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  nsXPIDLCString origin;
>-  rv = mPrincipal->GetOrigin(getter_Copies(origin));
>+  nsCAutoString origin;
>+  rv = nsContentUtils::GetASCIIOrigin(srcURI, origin);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   nsCAutoString spec;
>   rv = srcURI->GetSpec(spec);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>   mOriginalURL = NS_ConvertUTF8toUTF16(spec);
>   mSrc = srcURI;
>-  mOrigin = origin;
>+  CopyUTF8toUTF16(origin, mUTF16Origin);
Could you please file a separate bug about this, since
we probably should get this change to Fx6.
Comment 62 Wellington Fernando de Macedo 2011-07-20 05:47:54 PDT
> I guess this should be mozWithCredentials until it is spec'ed.
Should the withCredentials ctor parameter be renamed, too?
Comment 63 Jonas Sicking (:sicking) 2011-07-20 08:57:21 PDT
Comment on attachment 546956 [details] [diff] [review]
patch v4

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

Yeah, we should probably change both to mozWithCredentials. Though might be worth checking with Hixie first in case we can get this into spec quickly and avoid the prefixing.

::: content/base/src/nsEventSource.cpp
@@ +388,5 @@
> +  if (aArgc >= 2) {
> +    NS_ENSURE_TRUE(!JSVAL_IS_PRIMITIVE(aArgv[1]), NS_ERROR_INVALID_ARG);
> +
> +    JSObject *obj = JSVAL_TO_OBJECT(aArgv[1]);
> +    NS_ENSURE_TRUE(obj, NS_ERROR_FAILURE);

Make this an NS_ASSERTION rather than an NS_ENSURE_TRUE. This should never happen since JSVAL_IS_PRIMITIVE test should check for all conditions when this returns null.
Comment 64 :Ms2ger 2011-07-20 09:24:04 PDT
(In reply to comment #63)
> ::: content/base/src/nsEventSource.cpp
> @@ +388,5 @@
> > +  if (aArgc >= 2) {
> > +    NS_ENSURE_TRUE(!JSVAL_IS_PRIMITIVE(aArgv[1]), NS_ERROR_INVALID_ARG);
> > +
> > +    JSObject *obj = JSVAL_TO_OBJECT(aArgv[1]);
> > +    NS_ENSURE_TRUE(obj, NS_ERROR_FAILURE);
> 
> Make this an NS_ASSERTION rather than an NS_ENSURE_TRUE. This should never
> happen since JSVAL_IS_PRIMITIVE test should check for all conditions when
> this returns null.

I still believe that check is wrong per spec; see step 2 at <http://dev.w3.org/2006/webapi/WebIDL/#es-dictionary>.
Comment 65 Jonas Sicking (:sicking) 2011-07-20 13:04:03 PDT
You still didn't answer which situations that you think we behave incorrectly, and how we should behave.

I could possibly see that it would mean that we should convert primitives like 4 or "foo" to an object, which would mean that we'd use default values rather than throw.

Though this seems like that is somewhat strange behavior. If someone does:

new EventSource(url, 4)

it seems buggy enough that it's better to throw than to silently use default values for all parameters.
Comment 66 Cameron McCormack (:heycam) 2011-07-20 14:40:44 PDT
I think that is what Ms2ger is referring to.  I agree it's unlikely that `new EventSource(url, 4)` would not be a result of a bug in the user's code.  Looking at how for example property descriptor object arguments are treated in Object.defineProperty, they do an "is it an object" check and throw if it isn't.  So perhaps it would be good to change Web IDL here.  Jonas, could you mail public-script-coord with the suggestion so I can track it as a LC comment?  Thanks!
Comment 67 Jonas Sicking (:sicking) 2011-07-21 15:42:19 PDT
Either way, I think we should land the current patch first, while debating what to do for the edge cases.
Comment 68 Wellington Fernando de Macedo 2011-07-21 17:15:51 PDT
Created attachment 547565 [details] [diff] [review]
patch v5
Comment 69 Jonas Sicking (:sicking) 2011-07-21 18:31:26 PDT
Comment on attachment 547565 [details] [diff] [review]
patch v5

What's Changes in the latest patch? Just prefix? r=me if so
Comment 70 Wellington Fernando de Macedo 2011-07-21 18:42:40 PDT
> What's Changes in the latest patch? Just prefix? r=me if so
I've prefixed the withCredentials prop, I've added that assertion you said, and I've removed that block of code changes that went to the bug 673296 as Olli asked for.
Comment 71 Jonas Sicking (:sicking) 2011-10-28 12:29:24 PDT
What's remaining here. It looks like the patch is ready to land?
Comment 72 Wellington Fernando de Macedo 2011-10-28 13:18:02 PDT
Yes, it is.
Comment 73 Olli Pettay [:smaug] 2011-10-28 14:55:45 PDT
The patch just need to be updated for trunk
Comment 74 Jonas Sicking (:sicking) 2011-10-28 16:35:23 PDT
Wellington, let me know if you want me to do that? Either way I'm happy to land this.
Comment 75 Wellington Fernando de Macedo 2011-10-28 17:01:58 PDT
Yes, I'm doing it right now.
Comment 76 Wellington Fernando de Macedo 2011-10-28 18:49:12 PDT
Created attachment 570445 [details] [diff] [review]
v6

This one is up to date. Two of the tests fail, because it expects the bug https://bugzilla.mozilla.org/show_bug.cgi?id=673296.
Comment 77 Jonas Sicking (:sicking) 2011-10-28 18:52:02 PDT
That one looks ready to land too, right? If we land both, should all tests pass?
Comment 78 Wellington Fernando de Macedo 2011-10-28 19:00:45 PDT
I've just imported the bug 673296 patch. It also needs to be updated. I'm going to test it right now.
Comment 79 Wellington Fernando de Macedo 2011-10-28 19:08:49 PDT
Ok, now this one together with the latest patch of the bug 673296 should pass all tests.
Comment 80 Jonas Sicking (:sicking) 2011-10-28 19:09:30 PDT
Comment on attachment 570445 [details] [diff] [review]
v6

Assuming this just updates to tip, rs=me. Let me know if there was anything in particular you want me to look at.
Comment 81 Wellington Fernando de Macedo 2011-10-28 19:16:06 PDT
Comment on attachment 570445 [details] [diff] [review]
v6

>  Let me know if there was anything in particular you want me to look at.

No, I don't. Only the mozWithCredentials needs to be documented in the Mozilla Developer Network to people know that it is supported.
Comment 82 Jonas Sicking (:sicking) 2011-10-28 20:01:55 PDT
Checked in to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad7f12bde01

Thanks for doing this work!! As I mentioned in the other bug, please do poke us if you see patches lingering like this. That should never happen.

For what it's worth, I filed [1] on the spec changing to use the same mechanism as is implemented here. Once that bug is fixed we should file a separate bug on unprefixing our implementation.

[1] http://www.w3.org/Bugs/Public/show_bug.cgi?id=14592

Yay!
Comment 83 Marco Bonardo [::mak] 2011-10-29 01:52:52 PDT
https://hg.mozilla.org/mozilla-central/rev/3ad7f12bde01
Comment 84 Wellington Fernando de Macedo 2011-10-29 05:15:14 PDT
> Thanks for doing this work!!
You're welcome.
Comment 86 Wellington Fernando de Macedo 2011-10-29 18:07:57 PDT
The problem is in content/base/test/test_bug338583.html:63. I've forgotten to apply the stress_factor on that timeout. I'll fix that.
Comment 87 Jonas Sicking (:sicking) 2011-10-31 09:21:34 PDT
By the sounds of things in the W3C bug, it seems like the approach used in this bug is what we'll end up using. So feel free to prepare a separate patch to drop the prefix, and we'll hopefully be able to take it on Aurora (or even sooner if Hixie modifies the spec).
Comment 88 Jonas Sicking (:sicking) 2011-10-31 16:25:58 PDT
Spec has been updated, so IMHO we can remove the prefix. Indications so far was that all other vendors agreed with the proposal, so I'm pretty sure it'll stick.
Comment 89 Wellington Fernando de Macedo 2011-11-05 10:09:41 PDT
Created attachment 572212 [details] [diff] [review]
fixes on v6
Comment 90 Wellington Fernando de Macedo 2011-11-05 10:42:28 PDT
https://hg.mozilla.org/try/rev/37276cb289e7
Comment 91 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-26 06:58:11 PST
Is this ready to land?
Comment 92 Jonas Sicking (:sicking) 2011-11-26 10:49:17 PST
Yes!

Wellington, feel free to set checkin-needed flag if you want something checked in. Or apply for a hg account.
Comment 93 Jonas Sicking (:sicking) 2011-11-26 17:12:35 PST
I can't check in until Sunday night, so if someone wants to beat me to it go ahead
Comment 94 Jonas Sicking (:sicking) 2011-12-02 18:34:28 PST
Check in to inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/127374ca4f92

Thank you so much for your work! You rock!
Comment 95 Marco Bonardo [::mak] 2011-12-03 03:25:53 PST
This has been backed out, Sicking please annotate backouts in the bugs and backout revisions in the checkin comment otherwise it's a nightmare to find what you backed out.

8395 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug338583.html | Wrong Origin in test 5.e
8399 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug338583.html | Wrong Origin in test 5.c
8415 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug338583.html | Wrong Origin in test 5.e
Comment 96 Wellington Fernando de Macedo 2011-12-04 14:54:28 PST
I tested on Try Server on all platforms and I didn't get those fails.
https://tbpl.mozilla.org/?tree=Try&rev=37276cb289e7

It failed because the bug 673296 wasn't applied correctly. Especially this  block was missing:
-  nsXPIDLCString origin;
-  rv = mPrincipal->GetOrigin(getter_Copies(origin));
+  nsAutoString origin;
+  rv = nsContentUtils::GetUTFOrigin(srcURI, origin);
   NS_ENSURE_SUCCESS(rv, rv);
Comment 97 Jonas Sicking (:sicking) 2011-12-04 16:47:16 PST
If you attach a followup patch to that bug I can check both of them in.
Comment 98 Jonas Sicking (:sicking) 2011-12-06 02:47:53 PST
Landed with followup fix on inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed783dfd8179
Comment 99 Ed Morley [:emorley] 2011-12-07 02:49:28 PST
https://hg.mozilla.org/mozilla-central/rev/ed783dfd8179
Comment 100 Henri Sivonen (:hsivonen) 2011-12-07 08:50:04 PST
This bug added tests that use sync XHR with withCredentials. Support for that combination is going away in bug 701787.
Comment 101 Eric Shepherd [:sheppy] 2012-03-24 05:38:30 PDT
Updated docs:

https://developer.mozilla.org/en/Server-sent_events/Using_server-sent_events
https://developer.mozilla.org/en/Server-sent_events/EventSource

And mentioned on Firefox 11 for developers.
Comment 102 André Fiedler 2014-10-22 11:27:09 PDT
Cross-Origin URLs in EventSource is broken in current Nightly 36.0a1 (2014-10-22)! Can we open this bug again?
Comment 103 Olli Pettay [:smaug] 2014-10-22 11:30:28 PDT
No. Please file a new bug, thanks.
Comment 104 Wellington Fernando de Macedo 2014-11-09 05:46:53 PST
Please file a new bug, thanks.

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