Closed Bug 664179 Opened 13 years ago Closed 13 years ago

Allow Cross-Origin URLs in EventSource (Server-Sent Events)

Categories

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

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

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla11

People

(Reporter: public, Assigned: wfernandom2004)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 7 obsolete files)

v6
34.25 KB, patch
sicking
: review+
sicking
: checkin+
Details | Diff | Splinter Review
fixes on v6
11.79 KB, patch
sicking
: review+
sicking
: checkin+
Details | Diff | Splinter Review 
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.
Summary: Allow Cross-Origin URLs as EventSource (Server-Sent Events) → Allow Cross-Origin URLs in EventSource (Server-Sent Events)
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?
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
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
OS: Linux → All
Hardware: x86 → All
> Wellington, would you be interested to implement this?
Yes. I will try to implement it this weekend.
Status: NEW → ASSIGNED
Assignee: nobody → wfernandom2004
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.
I've updated the spec to define how CORS works in EventSource.
(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?
> reading the construction steps of the specification, the steps 3 and 4 (...)
Sorry, steps 3 and 4 -> steps 4 and 5
I forgot to remove step 4, my apologies. It's gone now.
Attached patch patch v1 (obsolete) — — Splinter Review 

        Attachment #542022 -
        Flags: review?(Olli.Pettay)

    
    

    
  
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();

        Attachment #542022 -
        Flags: review?(Olli.Pettay) → review+

    
    

    
  
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.

        Attachment #542022 -
        Flags: review?(jonas)

    
  
Depends on: 338583

    
    

    
  
We must check that how mixed mode UI works with this.
No longer depends on: 338583

    
    

    
  
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?

    
    

    
  
I mean the case when some part of the page is using https and some
part is using http.

    
    

    
  
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.

        Attachment #542022 -
        Flags: review?(jonas) → review-

    
    

    
  
> 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?

    
    

    
  
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.

    
    

    
  
And I agree with you. I think EventSource should default to the same what
XMLHttpRequest has, by default.

    
    

    
  
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.

    
    

    
  
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?

    
    

    
  
I'd be ok with that. The parameter should be optional, and false by default.

    
    

    
  
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?

    
    

    
  
(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?

    
    

    
  
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.

    
    

    
  
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)

    
    

    
  
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?

    
    

    
  
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.

    
    

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

    
    

    
  
(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.

    
    

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

    
    

    
  
(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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #542022 -
        Attachment is obsolete: true

        Attachment #546453 -
        Flags: review?(jonas)

        Attachment #546453 -
        Flags: review?(Olli.Pettay)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #546453 -
        Attachment is obsolete: true

        Attachment #546453 -
        Flags: review?(jonas)

        Attachment #546453 -
        Flags: review?(Olli.Pettay)

        Attachment #546454 -
        Flags: review?(jonas)

        Attachment #546454 -
        Flags: review?(Olli.Pettay)

    
    

    
  
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)

    
    

    
  
if second argument is invalid, should the constructor fail and raise an exception, or should continue with withCredentials=false?

    
    

    
  
if the second argument is invalid, should the constructor fail and raise an exception, or should continue with withCredentials=false?

    
    

    
  
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.

    
    

    
  
> If it's something other than an object I suspect we should throw yes.
Yes, that I meant. Thanks.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #546454 -
        Attachment is obsolete: true

        Attachment #546454 -
        Flags: review?(jonas)

        Attachment #546454 -
        Flags: review?(Olli.Pettay)

        Attachment #546458 -
        Flags: review?(jonas)

        Attachment #546458 -
        Flags: review?(Olli.Pettay)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
A fix. This is the last one :)

        Attachment #546458 -
        Attachment is obsolete: true

        Attachment #546458 -
        Flags: review?(jonas)

        Attachment #546458 -
        Flags: review?(Olli.Pettay)

        Attachment #546463 -
        Flags: review?(jonas)

        Attachment #546463 -
        Flags: review?(Olli.Pettay)

    
    

    
  
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.

        Attachment #546463 -
        Flags: review?(Olli.Pettay) → review-

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
But Jonas may have other opinion.

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
> 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.

    
    

    
  
BTW, a open or start() method in the EventSource API IMHO wouldn't be so ugly :)

    
    

    
  
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 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.

        Attachment #546463 -
        Flags: review?(jonas) → review+

    
    

    
  
(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.

    
    

    
  
I don't know what you mean by that, or what cases you are worried that my suggestion would get wrong?

    
    

    
  
>> +  rv = nsContentUtils::GetASCIIOrigin(srcURI, origin);
> Why this change?

See comment #14 and bug 670687.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v4 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #546463 -
        Attachment is obsolete: true

        Attachment #546956 -
        Flags: review?(jonas)

        Attachment #546956 -
        Flags: review?(Olli.Pettay)

    
    

    
  
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.

        Attachment #546956 -
        Flags: review?(Olli.Pettay) → review+

    
    

    
  
> I guess this should be mozWithCredentials until it is spec'ed.
Should the withCredentials ctor parameter be renamed, too?

    
    

    
  
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.

        Attachment #546956 -
        Flags: review?(jonas) → review+

    
    

    
  
(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>.

    
    

    
  
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.

    
    

    
  
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!

    
    

    
  
Either way, I think we should land the current patch first, while debating what to do for the edge cases.

    
  
Depends on: 673296

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v5 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #546956 -
        Attachment is obsolete: true

        Attachment #547565 -
        Flags: review?(jonas)

        Attachment #547565 -
        Flags: review?(Olli.Pettay)

    
    

    
  
Comment on attachment 547565 [details] [diff] [review]
patch v5

What's Changes in the latest patch? Just prefix? r=me if so

        Attachment #547565 -
        Flags: review?(jonas) → review+

    
    

    
  
> 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.

        Attachment #547565 -
        Flags: review?(Olli.Pettay) → review+

    
    

    
  
What's remaining here. It looks like the patch is ready to land?

    
    

    
  
Yes, it is.

    
    

    
  
The patch just need to be updated for trunk

    
    

    
  
Wellington, let me know if you want me to do that? Either way I'm happy to land this.

    
    

    
  
Yes, I'm doing it right now.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v6
        — 
          — Splinter Review
      

    


  
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.

        Attachment #547565 -
        Attachment is obsolete: true

        Attachment #570445 -
        Flags: review?(jonas)

    
    

    
  
That one looks ready to land too, right? If we land both, should all tests pass?

    
    

    
  
I've just imported the bug 673296 patch. It also needs to be updated. I'm going to test it right now.

    
    

    
  
Ok, now this one together with the latest patch of the bug 673296 should pass all tests.

    
    

    
  
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.

        Attachment #570445 -
        Flags: review?(jonas) → review+

    
    

    
  
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.

        Attachment #570445 -
        Flags: superreview?(jonas)

    
    

    
  
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!

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/3ad7f12bde01
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

    
    

    
  
> Thanks for doing this work!!
You're welcome.

    
    

    
  
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.

    
    

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

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        fixes on v6
        — 
          — Splinter Review
      

    


  

        Attachment #572212 -
        Flags: review?(jonas)

    
    

    
  
Yes!

Wellington, feel free to set checkin-needed flag if you want something checked in. Or apply for a hg account.

    
  

        Attachment #570445 -
        Flags: checkin?(jonas)

    
  

        Attachment #572212 -
        Flags: checkin?(jonas)

    
    

    
  
I can't check in until Sunday night, so if someone wants to beat me to it go ahead

    
    

    
  
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

    
    

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

    
    

    
  
If you attach a followup patch to that bug I can check both of them in.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ed783dfd8179
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Target Milestone: mozilla10 → mozilla11

    
    

    
  
This bug added tests that use sync XHR with withCredentials. Support for that combination is going away in bug 701787.

    
  
Status: RESOLVED → VERIFIED
Target Milestone: mozilla11 → mozilla10

    
  
Target Milestone: mozilla10 → mozilla11

    
  
Blocks: 716841
No longer blocks: 716841
Depends on: 716841
Flags: sec-review+

    
    

    
  
Cross-Origin URLs in EventSource is broken in current Nightly 36.0a1 (2014-10-22)! Can we open this bug again?
Flags: needinfo?(wfernandom2004)

    
    

    
  
No. Please file a new bug, thanks.

    
    

    
  
Please file a new bug, thanks.
Flags: needinfo?(wfernandom2004)

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: