[Webkit-unassigned] [Bug 61862] EventSource should support CORS

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61862





--- Comment #36 from Per-Erik Brodin <per-erik.brodin at ericsson.com>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.