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: | P2 | ||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | 65694 | ||||||||||||||||||||||
Bug Blocks: | 70112 | ||||||||||||||||||||||
Attachments: |
|
Description
goberman
2011-06-01 08:13:38 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. I would appreciate if you keep this enhancement request open. I have not seen any other formal requests for this. Thanks 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 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. 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 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. OK, sent request to whatwg@lists.whatwg.org 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. 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 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. I appreciate it. Thanks 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 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? *** Bug 62949 has been marked as a duplicate of this bug. *** 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.
(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. > 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.
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) 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).
It has been a while since this request was entered. Is there a chance to promote the fix anytime soon? Thanks 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+ 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? It's in. Sorry I forgot to update this bug about it, my bad. Awesome. Thanks. Created attachment 111298 [details]
patch 4
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. 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 Can someone resolve whatever is holding the progress on this item? It has been half a year since the request was entered. Thanks ap should probably review this patch. 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. > The spec says to always use credentials when reconnecting, but that looks like a mistake.
Fixed.
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. 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. 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. 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. (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. (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. > 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?) 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
(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. 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 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
*** Bug 96368 has been marked as a duplicate of this bug. *** (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? 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
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). 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. Comment on attachment 180024 [details] patch 8 Clearing flags on attachment: 180024 Committed r138083: <http://trac.webkit.org/changeset/138083> All reviewed patches have been landed. Closing bug. 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.) |