Bug 61862

Summary:

EventSource should support CORS

Product:

WebKit

Reporter:

goberman

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Enhancement

CC:

abarth, adam.bergkvist, allan.jardine, ap, christoph.kaser, darren, dglazkov, donggwan.kim, d-r, evan, ian, japhet, levin+threading, mifenton, mike, ojan.autocc, ojan, per-erik.brodin, pf, rakuco, rwlbuis, sonny.piers, syoichi, tonikitoo, webkit.review.bot, yong.li.webkit

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

65694

Bug Blocks:

70112

Attachments:

Description	Flags
proposed patch	abarth: review-
updated patch	none
patch 3	none
patch 4	ap: review-, webkit.review.bot: commit-queue-
patch 5	ap: review-
patch 6	webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03	none
patch 7	none
patch 8	none

goberman

Reported 2011-06-01 08:13:38 PDT

I set "Access-Control-Allow-Origin" header to "*" in server response to enable cross-domain requests. This works for XHR, but does not work for EventSource. Can EventSource be enhanced to support cross-domain calls. SERVER package test; import java.io.IOException; import java.io.PrintWriter; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; public class WebFrameworkServletEventSource extends HttpServlet { public void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { System.out.println("Begin for session: " + request.getSession(true).getId() + " " + response.getWriter()); response.setHeader("pragma", "no-cache,no-store"); response.setHeader("cache-control", "no-cache,no-store,max-age=0,max-stale=0"); response.setHeader("Connection", "close"); response.setHeader("Access-Control-Allow-Origin", "*"); response.setContentType("text/event-stream"); PrintWriter out = response.getWriter(); int messagesSent = 0; while (true) { out.print("data: "); out.print("{\"m\":8448,\"r\":0,\"e\":1,\"s\":[\"BAC.\",0],\"f\":[[0,6,3993,2],[55,6,1185,2],[54,6,3218,2],[5,6,6617,2],[52,4,15],[12,6,1700,2]]}"); out.print("\n\n"); out.flush(); messagesSent++; try { Thread.sleep(2000); } catch (InterruptedException e1) { } } } } CLIENT <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/1999/REC-html401-19991224/loose.dtd"> <html> <head> <title>test</title> </head> <body> <input type="button" value="TEST" onclick="test(); return false;" /> <script type="text/javascript"> var domElement; var messagesProcessed = 0; var eventSrc; function test() { domElement = document.getElementsByTagName('body')[0]; debug('connect'); try { eventSrc = new EventSource('http://cross-domain-address/Test/Controller'); } catch (e) { debug('exception'); } eventSrc.onopen = function (event) { debug('open ' + event.type); }; eventSrc.onerror = function (event) { debug('error ' + event.type); eventSrc.close(); }; eventSrc.onmessage = function (event) { //debug(event.type + ' ' + event.data); }; } function debug(message) { var oText = document.createTextNode(message); var oDiv = document.createElement('div'); oDiv.appendChild(oText); domElement.insertBefore(oDiv, domElement.hasChildNodes() ? domElement.childNodes[0] : null); } </script> </body> </html>

Attachments
proposed patch (9.47 KB, patch) 2011-06-19 14:37 PDT, Per-Erik Brodin	abarth: review-	Details Formatted Diff Diff
updated patch (10.46 KB, patch) 2011-08-17 11:39 PDT, Per-Erik Brodin	no flags	Details Formatted Diff Diff
patch 3 (10.43 KB, patch) 2011-08-24 13:45 PDT, Per-Erik Brodin	no flags	Details Formatted Diff Diff
patch 4 (13.11 KB, patch) 2011-10-17 12:20 PDT, Per-Erik Brodin	ap: review- webkit.review.bot: commit-queue-	Details Formatted Diff Diff
patch 5 (44.90 KB, patch) 2011-12-22 06:18 PST, Per-Erik Brodin	ap: review-	Details Formatted Diff Diff
patch 6 (36.89 KB, patch) 2012-05-17 10:13 PDT, Per-Erik Brodin	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ec2-cr-linux-03 (480.24 KB, application/zip) 2012-05-17 11:06 PDT, WebKit Review Bot	no flags	Details
patch 7 (38.76 KB, patch) 2012-12-13 11:17 PST, Per-Erik Brodin	no flags	Details Formatted Diff Diff
patch 8 (38.80 KB, patch) 2012-12-18 14:15 PST, Per-Erik Brodin	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

Alexey Proskuryakov

Comment 1 2011-06-01 08:40:22 PDT

EventSource specification doesn't allow this yet, and almost certainly won't until version 2. This seems to be a feature requested by many developers.

goberman

Comment 2 2011-06-01 08:49:29 PDT

I would appreciate if you keep this enhancement request open. I have not seen any other formal requests for this. Thanks

goberman

Comment 3 2011-06-01 10:49:23 PDT

Alexey, sorry to bug you with this, but do you know where I can find road map for the EventSource 2 release? Also, I looked at http://www.w3.org/TR/2011/WD-eventsource-20110310/ draft and I do not see cross domain calls be a part of it. Do you know if they have a list of formal requests? Thanks

Ian 'Hixie' Hickson

Comment 4 2011-06-01 10:54:04 PDT

I expect I'll update EventSource to support CORS sometime this year. If it's particularly urgent, please e-mail the WHATWG list with your use case. The main reason it's not done already is that I'm waiting for implementation feedback on the various other parts of the platform that use CORS — primarily XMLHttpRequest, but also now <img src> and <video src>/<audio src>/<source>/<track>. I don't want to add too many dependencies on CORS before we are confident it works.

goberman

Comment 5 2011-06-01 10:57:04 PDT

OK, it is fair. But how would I know when it is implemented? Is there some request I can subscribe to to receive notification of progress for this? My concern is that it will be implemented and I would not know anything about it. Or you will update this request with progress... Thanks

Ian 'Hixie' Hickson

Comment 6 2011-06-01 11:00:35 PDT

I expect this bug will be updated when it's implemented in WebKit. I don't know of a good way to keep track of it for all browsers. Maybe you should suggest to http://caniuse.com/ that there should be a way to subscribe for updates or something. :-) Let me know if you do find something.

goberman

Comment 7 2011-06-01 11:25:27 PDT

OK, sent request to whatwg@lists.whatwg.org

goberman

Comment 8 2011-06-02 07:32:10 PDT

Got a response from Jonas Sicking: I absolutely think we should do this. I was in fact quite surprised to find that this wasn't already the case. Getting event streams from other sites seems like a prime use case for EventSource.

goberman

Comment 9 2011-06-10 09:30:38 PDT

It looks like a large number of people want to have it happen, but people responsible for the EventSource draft document (whoever they are) have not updated it to reflect it. A similar request was sent to WHATWG half a year ago. I am not sure what would be the best way to keep moving it forward. I am out of ideas personally. Thanks

Ian 'Hixie' Hickson

Comment 10 2011-06-10 12:09:54 PDT

The only reason the spec doesn't have this yet is that I'm rolling out CORS support in the WHATWG spec slowly over time, so as to collect implementation experience and not make a bunch of mistakes we can't fix. I recently added CORS support for <img> and <video>; once we have implementation experience on that I'll move on to adding it to one of the other features that could use it, such as EventSource or Workers.

goberman

Comment 11 2011-06-13 13:18:33 PDT

I appreciate it. Thanks

Per-Erik Brodin

Comment 12 2011-06-19 14:37:46 PDT

Created attachment 97728 [details] proposed patch Had to make a few assumptions. Feedback sent to WHATWG: http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2011-June/032158.html

Adam Barth

Comment 13 2011-06-20 01:52:30 PDT

Comment on attachment 97728 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=97728&action=review > Source/WebCore/loader/CrossOriginAccessControl.cpp:52 > if (equalIgnoringCase(name, "accept") > || equalIgnoringCase(name, "accept-language") > + || equalIgnoringCase(name, "cache-control") > || equalIgnoringCase(name, "content-language") > + || equalIgnoringCase(name, "last-event-id") > || equalIgnoringCase(name, "origin")) > return true; This list comes from the CORS spec. It's not clear we should change it unilaterally. > Source/WebCore/page/EventSource.cpp:71 > - , m_origin(context->securityOrigin()->toString()) > + , m_origin(SecurityOrigin::createFromString(url)->toString()) I don't understand this part of the change. Why don't the origin come from the context?

Adam Barth

Comment 14 2011-06-20 01:52:48 PDT

*** Bug 62949 has been marked as a duplicate of this bug. ***

Adam Barth

Comment 15 2011-06-20 01:53:52 PDT

Comment on attachment 97728 [details] proposed patch Marking R- because I'd like those questions answered. If you think the current patch is correct, please feel free to re-nominate.

Per-Erik Brodin

Comment 16 2011-06-20 03:13:11 PDT

(In reply to comment #13) > (From update of attachment 97728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97728&action=review > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:52 > > if (equalIgnoringCase(name, "accept") > > || equalIgnoringCase(name, "accept-language") > > + || equalIgnoringCase(name, "cache-control") > > || equalIgnoringCase(name, "content-language") > > + || equalIgnoringCase(name, "last-event-id") > > || equalIgnoringCase(name, "origin")) > > return true; > > This list comes from the CORS spec. It's not clear we should change it unilaterally. Note that Last-Event-ID is in the simple request header list in the CORS spec, but it doesn't make sense to add only that since then a preflight request will be made anyway. > > > Source/WebCore/page/EventSource.cpp:71 > > - , m_origin(context->securityOrigin()->toString()) > > + , m_origin(SecurityOrigin::createFromString(url)->toString()) > > I don't understand this part of the change. Why don't the origin come from the context? This is the origin set on the dispatched message events, "the origin of the event stream's URL" in the spec. I should probably rename this to m_eventStreamOrigin or similar. This was the same as the script's origin when only same origin requests were supported. I should drop 'fromString' as well since url is passed as a KURL to the constructor. I agree that we shouldn't land anything until the questions on the spec(s) get answered.

Adam Barth

Comment 17 2011-06-20 10:06:09 PDT

> This is the origin set on the dispatched message events, "the origin of the event stream's URL" in the spec. I should probably rename this to m_eventStreamOrigin or similar. This was the same as the script's origin when only same origin requests were supported. Makes sense, but don't we need to get that value from the final response in the redirect chain rather than the first? In either case, having a clearer name would be very helpful.

Per-Erik Brodin

Comment 18 2011-08-17 11:39:46 PDT

Created attachment 104203 [details] updated patch (In reply to comment #13) > (From update of attachment 97728 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97728&action=review > > > Source/WebCore/loader/CrossOriginAccessControl.cpp:52 > > if (equalIgnoringCase(name, "accept") > > || equalIgnoringCase(name, "accept-language") > > + || equalIgnoringCase(name, "cache-control") > > || equalIgnoringCase(name, "content-language") > > + || equalIgnoringCase(name, "last-event-id") > > || equalIgnoringCase(name, "origin")) > > return true; > > This list comes from the CORS spec. It's not clear we should change it unilaterally. This change is no longer needed since the check is skipped altogether. (In reply to comment #17) > > This is the origin set on the dispatched message events, "the origin of the event stream's URL" in the spec. I should probably rename this to m_eventStreamOrigin or similar. This was the same as the script's origin when only same origin requests were supported. > > Makes sense, but don't we need to get that value from the final response in the redirect chain rather than the first? In either case, having a clearer name would be very helpful. m_origin renamed to m_eventStreamOrigin and now set from the response URL. (makes no difference currently since it seems that WebKit doesn't support CORS redirects)

Per-Erik Brodin

Comment 19 2011-08-24 13:45:44 PDT

Created attachment 105060 [details] patch 3 Fixed an issue with the test (the idea was to make EventSource reconnect to test that the Last-Event-ID header doesn't trigger a preflight but a new EventSource was created instead and the test passed anyway due to incorrect logic).

goberman

Comment 20 2011-10-13 13:23:15 PDT

It has been a while since this request was entered. Is there a chance to promote the fix anytime soon? Thanks

Adam Barth

Comment 21 2011-10-13 14:15:32 PDT

Comment on attachment 105060 [details] patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=105060&action=review > Source/WebCore/page/EventSource.cpp:246 > + String message = "EventSource cannot load " + error.failingURL() + ". " + error.localizedDescription(); Please use makeString rather than operator+

Adam Barth

Comment 22 2011-10-13 14:16:33 PDT

This implementation looks reasonable. There's a question of whether we should implement before the spec is written. @Hixie: Any thoughts on when you're going to add this feature to the spec?

Ian 'Hixie' Hickson

Comment 23 2011-10-13 15:04:00 PDT

It's in. Sorry I forgot to update this bug about it, my bad.

Adam Barth

Comment 24 2011-10-13 15:05:31 PDT

Awesome. Thanks.

Per-Erik Brodin

Comment 25 2011-10-17 12:20:18 PDT

Created attachment 111298 [details] patch 4

WebKit Review Bot

Comment 26 2011-10-17 12:22:59 PDT

Attachment 111298 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2 Updating OpenSource M Source/WebCore/GNUmakefile.list.am M Source/WebCore/ChangeLog M Source/WebCore/platform/network/chromium/ResourceRequest.h M Source/WebCore/WebCore.gypi M Source/WebCore/CMakeLists.txt M Source/WebCore/WebCore.xcodeproj/project.pbxproj M Source/WebCore/loader/cache/CachedResourceLoader.cpp M Source/WebCore/loader/cache/CachedResourceLoader.h M Source/WebCore/loader/cache/CachedResource.cpp M Source/WebCore/loader/cache/CachedResourceClient.h M Source/WebCore/loader/cache/CachedResource.h A Source/WebCore/loader/cache/CachedCues.cpp M Source/WebCore/loader/cache/CachedResourceRequest.cpp A Source/WebCore/loader/cache/CachedCues.h r97637 = a24babbf6336686dc5c919f00b81e650219e531e (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Updating chromium port dependencies using gclient... Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Re-trying 'depot_tools/gclient sync' Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again. Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107. Re-trying 'depot_tools/gclient sync' No such file or directory at Tools/Scripts/update-webkit line 104. If any of these errors are false positives, please file a bug against check-webkit-style.

WebKit Review Bot

Comment 27 2011-10-17 18:03:37 PDT

Comment on attachment 111298 [details] patch 4 Attachment 111298 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10120056 New failing tests: http/tests/eventsource/eventsource-cors-no-server.html

goberman

Comment 28 2011-12-02 10:06:50 PST

Can someone resolve whatever is holding the progress on this item? It has been half a year since the request was entered. Thanks

Adam Barth

Comment 29 2011-12-02 10:46:38 PST

ap should probably review this patch.

Alexey Proskuryakov

Comment 30 2011-12-02 11:40:45 PST

Comment on attachment 111298 [details] patch 4 View in context: https://bugs.webkit.org/attachment.cgi?id=111298&action=review Looks like the spec changed substantially since the patch was posted. > Source/WebCore/page/EventSource.cpp:119 > options.allowCredentials = AllowStoredCredentials; The spec says that credentials should not be sent cross-origin unless constructor is called with a second argument telling it to allow credentials: 4. Let CORS mode be Anonymous. 5. If the second argument is present, and the withCredentials dictionary member has the value true, then set CORS mode to Use Credentials and initialize the new EventSource object's withCredentials attribute to true. There should also be a readonly withCredentials attribute on EventSource object. The spec says to always use credentials when reconnecting, but that looks like a mistake. > Source/WebCore/page/EventSource.cpp:256 > + bool failedResourceSharingCheck = error.domain() == errorDomainWebKitInternal; That's terribly fragile. We need a better way to communicate this to EventSource code.

Ian 'Hixie' Hickson

Comment 31 2011-12-02 14:58:02 PST

> The spec says to always use credentials when reconnecting, but that looks like a mistake. Fixed.

Per-Erik Brodin

Comment 32 2011-12-22 06:18:33 PST

Created attachment 120316 [details] patch 5 > (From update of attachment 111298 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=111298&action=review > > Looks like the spec changed substantially since the patch was posted. > > > Source/WebCore/page/EventSource.cpp:119 > > options.allowCredentials = AllowStoredCredentials; > > The spec says that credentials should not be sent cross-origin unless constructor is called with a second argument telling it to allow credentials: Fixed. > There should also be a readonly withCredentials attribute on EventSource object. Fixed. > > Source/WebCore/page/EventSource.cpp:256 > > + bool failedResourceSharingCheck = error.domain() == errorDomainWebKitInternal; > > That's terribly fragile. We need a better way to communicate this to EventSource code. Yes, that doesn't even work anymore since now errorDomainWebKitInternal is used for other error conditions as well. Fixed.

Alexey Proskuryakov

Comment 33 2011-12-22 11:30:08 PST

Comment on attachment 120316 [details] patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review > Source/WebCore/bindings/js/JSEventSourceCustom.cpp:53 > + JSValue initializerValue = exec->argument(1); > + if (!initializerValue.isUndefinedOrNull()) { This logic is somewhat surprising, and entirely untested. The spec just says "If the second argument is present". Not sure what the implied behavior for non-dictionary second argument is, but special casing null and undefined appears clearly wrong. Please make sure that the behavior is correct (Adam can advise on current IDL semantics), and add tests. > Source/WebCore/bindings/js/JSEventSourceCustom.cpp:54 > + JSDictionary dictionary(exec, initializerValue.toObject(exec)); I think that this can raise an exception. > Source/WebCore/bindings/v8/custom/V8EventSourceCustom.cpp:57 > + if (args.Length() >= 2) { This is different from JSC counterpart. > Source/WebCore/loader/ThreadableLoaderClient.h:50 > + virtual void didFailAccessControlCheck(const ResourceError&) { } It seems that the rule here is that anyone who cares about didFail should also care about didFailAccessControlCheck. Nothing enforces that (and it's not easy to even check that XMLHttpRequest is the only other client that needed this). Perhaps we should have a base class behavior of calling didFail() here. > Source/WebCore/loader/ThreadableLoaderClient.h:51 > virtual void didFailRedirectCheck() { } This existing code also looks suspicious, for the same reason as above. > Source/WebCore/page/EventSource.cpp:286 > + scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), 0); Aren't there default arguments for missing line number etc? Per discussion in bug 74075, the line should likely be 0, not 1. > Source/WebCore/xml/XMLHttpRequest.h:163 > virtual void didFail(const ResourceError&); > + virtual void didFailAccessControlCheck(const ResourceError&); I've been thinking of adding a more direct reason indication to ResourceError (similar to isCancellation()), but this also looks OK (similar to didFailRedirectCheck). We'll probably need to clean this up one day.

Adam Barth

Comment 34 2011-12-22 11:50:07 PST

Comment on attachment 120316 [details] patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review >> Source/WebCore/bindings/js/JSEventSourceCustom.cpp:54 >> + JSDictionary dictionary(exec, initializerValue.toObject(exec)); > > I think that this can raise an exception. Why is this using a dictionary at all? /me is confused. > Source/WebCore/page/EventSource.idl:36 > + CustomConstructor, Why does this need a custom constructor? That seems unlikely.

Adam Barth

Comment 35 2011-12-22 11:51:40 PST

Comment on attachment 120316 [details] patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review > Source/WebCore/page/EventSource.idl:38 > + Constructor(in DOMString url, in [Optional] EventSourceInit eventSourceInit), Ah, I see. The spec uses EventSourceInit here. Yeah, the code generator should be able to handle all of this for you. No custom bindings needed.

Per-Erik Brodin

Comment 36 2011-12-22 14:31:01 PST

(In reply to comment #33) > (From update of attachment 120316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review > > > Source/WebCore/bindings/js/JSEventSourceCustom.cpp:53 > > + JSValue initializerValue = exec->argument(1); > > + if (!initializerValue.isUndefinedOrNull()) { > > This logic is somewhat surprising, and entirely untested. The spec just says "If the second argument is present". Not sure what the implied behavior for non-dictionary second argument is, but special casing null and undefined appears clearly wrong. This is inspired by the code that is generated for the event constructor bindings. I think the idea is that everything that passes this check can be turned into a dictionary. > > Source/WebCore/bindings/js/JSEventSourceCustom.cpp:54 > > + JSDictionary dictionary(exec, initializerValue.toObject(exec)); > > I think that this can raise an exception. I think the check above will ensure that this cannot raise an exception. > > Source/WebCore/bindings/v8/custom/V8EventSourceCustom.cpp:57 > > + if (args.Length() >= 2) { > > This is different from JSC counterpart. Yes, but the behavior should be the same. If there is a second argument that is an object that has a property 'withCredentials' that is true, then the EventSourceInit dictionary member 'withCredentials' will be true, otherwise it will be false. As I said, this is similar to the code that is generated for the event constructor bindings. > > Source/WebCore/loader/ThreadableLoaderClient.h:50 > > + virtual void didFailAccessControlCheck(const ResourceError&) { } > > It seems that the rule here is that anyone who cares about didFail should also care about didFailAccessControlCheck. Nothing enforces that (and it's not easy to even check that XMLHttpRequest is the only other client that needed this). didFailAccessControlCheck will only be called when CORS is used so it's good that there's a default/empty implementation for it. I used an ASSERT_NOT_REACHED() here to test that didFailAccessControlCheck is implemented wherever it's needed, but then I removed it since everything else here has completely empty implementations. Perhaps I should add it back? > > Source/WebCore/page/EventSource.cpp:286 > > + scriptExecutionContext()->addMessage(JSMessageSource, LogMessageType, ErrorMessageLevel, message, 1, String(), 0); > > Aren't there default arguments for missing line number etc? Per discussion in bug 74075, the line should likely be 0, not 1. I guess I should use addConsoleMessage instead, since it's used elsewhere in EventSource.cpp and also in XMLHttpRequest.cpp. Then I don't have to supply the line number. It seems that addConsoleMessage is using line number 1 though. > > Source/WebCore/xml/XMLHttpRequest.h:163 > > virtual void didFail(const ResourceError&); > > + virtual void didFailAccessControlCheck(const ResourceError&); > > I've been thinking of adding a more direct reason indication to ResourceError (similar to isCancellation()), but this also looks OK (similar to didFailRedirectCheck). We'll probably need to clean this up one day. I wonder how this can be done without making ResourceError aware of web platform concepts like CORS.

Per-Erik Brodin

Comment 37 2011-12-22 14:34:05 PST

(In reply to comment #35) > (From update of attachment 120316 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=120316&action=review > > > Source/WebCore/page/EventSource.idl:38 > > + Constructor(in DOMString url, in [Optional] EventSourceInit eventSourceInit), > > Ah, I see. The spec uses EventSourceInit here. Yeah, the code generator should be able to handle all of this for you. No custom bindings needed. EventSourceInit is a WebIDL dictionary. I don't think that this is supported by the code generators other than specifically for the event constructors, using a "constructor template". In case I'm mistaken, could you advise me on the proper WebKit WebIDL syntax to have the bindings automatically generated? It would be great if we could avoid the custom bindings.

Alexey Proskuryakov

Comment 38 2011-12-22 14:43:18 PST

> didFailAccessControlCheck will only be called when CORS is used so it's good that there's a default/empty implementation for it. Understood why it cannot be "= 0". I don't understand why it's good for it to be empty. > I used an ASSERT_NOT_REACHED() here to test that didFailAccessControlCheck is implemented wherever it's needed, but then I removed it I would still prefer a compile time guarantee. Would it be a mistake to fall back to didFail()? > I wonder how this can be done without making ResourceError aware of web platform concepts like CORS. Yes, you're right. This wasn't a good idea. This still feels quite messy (e.g. CORS will be doing redirect checks too, so why is there a separate callback with a different signature for didFailRedirectCheck?)

Per-Erik Brodin

Comment 39 2012-05-17 10:13:16 PDT

Created attachment 142495 [details] patch 6 Updated patch: - The constructor bindings are now generated (this wasn't possible until recently) - Added testing of the new second argument to the constructor - addConsoleMessage now used instead of addMessage (it doesn't output a line number anymore) - didFail is now called from ThreadableLoaderClient in case didFailAccessControlCheck is not overridden - Fixed EventSource.h indentation

Per-Erik Brodin

Comment 40 2012-05-17 10:15:08 PDT

(In reply to comment #38) > This still feels quite messy (e.g. CORS will be doing redirect checks too, so why is there a separate callback with a different signature for didFailRedirectCheck?) I agree that this could probably be improved, but I'd rather not touch didFailRedirectCheck in this patch.

WebKit Review Bot

Comment 41 2012-05-17 11:05:59 PDT

Comment on attachment 142495 [details] patch 6 Attachment 142495 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12717588 New failing tests: fast/eventsource/eventsource-constructor.html

WebKit Review Bot

Comment 42 2012-05-17 11:06:05 PDT

Created attachment 142505 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Alexey Proskuryakov

Comment 43 2012-09-11 11:01:49 PDT

*** Bug 96368 has been marked as a duplicate of this bug. ***

Dominik Röttsches (drott)

Comment 44 2012-12-12 08:38:49 PST

(In reply to comment #39) > Created an attachment (id=142495) [details] > patch 6 Any plans to update this patch? Or would you like to have someone take over?

Per-Erik Brodin

Comment 45 2012-12-13 11:17:05 PST

Created attachment 179305 [details] patch 7 Updated patch: - Changed layoutTestController to testRunner in new tests - Removed DefaultIsUndefined from the 'Optional' extended IDL attribute since that is now the default for dictionaries - Removed LogMessageType argument that has been removed from addConsoleMessage() - Simplified constructor argument test - Updated more platform specific expected results for the constructor length test - Replaced enum with static const members to avoid style checker errors - Removed 'int' from 'unsigned int' in method signature to avoid style checker error

Alexey Proskuryakov

Comment 46 2012-12-17 13:45:05 PST

Comment on attachment 179305 [details] patch 7 View in context: https://bugs.webkit.org/attachment.cgi?id=179305&action=review Looks good to me, except for failing test. Sorry for the delay. > LayoutTests/http/tests/eventsource/eventsource-cors-no-server-expected.txt:3 > +FAIL: got error event but readyState is CONNECTING Is this OK? > Source/WebCore/page/EventSource.cpp:124 > + options.allowCredentials = origin->canRequest(m_url) || m_withCredentials ? AllowStoredCredentials : DoNotAllowStoredCredentials; I would put braces around the condition for clarity (and because this makes it look more like an if () condition).

Per-Erik Brodin

Comment 47 2012-12-18 14:15:22 PST

Created attachment 180024 [details] patch 8 (In reply to comment #46) > > LayoutTests/http/tests/eventsource/eventsource-cors-no-server-expected.txt:3 > > +FAIL: got error event but readyState is CONNECTING > > Is this OK? The failing test was originally for bug 70112 and my plan was to update the expected result when fixing that one. However, the spec changed recently so the current behavior is correct. I have updated the test accordingly. > > > Source/WebCore/page/EventSource.cpp:124 > > + options.allowCredentials = origin->canRequest(m_url) || m_withCredentials ? AllowStoredCredentials : DoNotAllowStoredCredentials; > > I would put braces around the condition for clarity (and because this makes it look more like an if () condition). Fixed.

WebKit Review Bot

Comment 48 2012-12-18 16:20:48 PST

Comment on attachment 180024 [details] patch 8 Clearing flags on attachment: 180024 Committed r138083: <http://trac.webkit.org/changeset/138083>

WebKit Review Bot

Comment 49 2012-12-18 16:20:57 PST

All reviewed patches have been landed. Closing bug.

Darren Cook

Comment 50 2013-05-30 20:32:26 PDT

Does "RESOLVED FIXED" mean it became live and released in Dec 2012? EventSource using CORS does not appear to be working. (I get "SecurityError: DOM Exception 18" in Chromium 25, on Ubuntu 10.04; same code is working with Firefox 20, and Opera 12.15) (Chromium 25 was released Feb 21st 2013.) (I'm using "Access-Control-Allow-Origin: *" on the server, not using credentials.)

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