Bug 198568 – Use NSURLSession for WebSocket

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Use NSURLSession for WebSocket

[youenn fablet]

Created attachment 371620 [details]
Patch

[youenn fablet]

Created attachment 371622 [details]
Patch

[youenn fablet]

Created attachment 371623 [details]
Patch

[youenn fablet]

Created attachment 371630 [details]
Patch

[youenn fablet]

Created attachment 371747 [details]
Patch

[youenn fablet]

Created attachment 371753 [details]
Patch

[youenn fablet]

Created attachment 371759 [details]
Patch

[youenn fablet]

Created attachment 371760 [details]
Patch

[youenn fablet]

Created attachment 371761 [details]
Patch

[youenn fablet]

Created attachment 371762 [details]
Patch

[youenn fablet]

Created attachment 371763 [details]
Patch

[youenn fablet]

Created attachment 371770 [details]
Patch

[youenn fablet]

Created attachment 371771 [details]
Patch

[EWS Watchlist]

Comment on attachment 371771 [details]
Patch

Attachment 371771 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12436931

New failing tests:
http/tests/websocket/tests/hybi/contentextensions/block.html
http/tests/websocket/tests/hybi/contentextensions/block-worker.html

[EWS Watchlist]

Created attachment 371774 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 371777 [details]
Patch

[youenn fablet]

Created attachment 371789 [details]
Patch

[youenn fablet]

Created attachment 371896 [details]
Patch

[Geoffrey Garen]

Comment on attachment 371896 [details]
Patch

r=me

[Sam Weinig]

Comment on attachment 371896 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=371896&action=review

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:70
>      if (m_bridge)
>          m_bridge->connect(url, protocol);
> +    return true;

It seems a bit weird that we would return true here even if m_bridge is null. Probably deserves a comment.

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:58
> +    bool connect(const URL&, const String& protocol) final;

What exactly does this return value indicate?

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:79
> +        bool connect(const URL&, const String& protocol);

If looks like the call to this function in WorkerThreadableWebSocketChannel::Bridge::connect does not handle the bool result. Should it?

> Source/WebCore/page/RuntimeEnabledFeatures.h:369
> +    bool isNSURLSessionWebSocketEnabled() const { return m_isNSURLSessionWebSocketEnabled; }
> +    void setIsNSURLSessionWebSocketEnabled(bool isEnabled) { m_isNSURLSessionWebSocketEnabled = isEnabled; }

Should this be in HAVE(NSURLSESSION_WEBSOCKET)? It looks like it is only ever called within it.

> Source/WebCore/page/RuntimeEnabledFeatures.h:556
> +    bool m_isNSURLSessionWebSocketEnabled { false };

#if HAVE(NSURLSESSION_WEBSOCKET)?

> Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.h:29
> +#include <WebCore/PageIdentifier.h>

Is this change intentional?

[youenn fablet]

(In reply to Sam Weinig from comment #20)
> Comment on attachment 371896 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371896&action=review
> 
> > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:70
> >      if (m_bridge)
> >          m_bridge->connect(url, protocol);
> > +    return true;
> 
> It seems a bit weird that we would return true here even if m_bridge is
> null. Probably deserves a comment.

OK, returning true keeps the current behavior where connect can never fail synchronously for workers.
I guess if bridge is null, it might be good to have the web socket fail.
Not sure if there is a possibility for this case to happen in practice or to be testable though.

> 
> > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:58
> > +    bool connect(const URL&, const String& protocol) final;
> 
> What exactly does this return value indicate?

It indicates that attempting the connection failed synchronously.
This allows to have the handling of whether firing the error event synchronously or asynchronously in WebSocket.
This also allows to share more code between WebKit::WebSocketChannel and WebCore::WebSocketChannel.
If we need to share more code, we could introduce MainThreadWebSocketChannel.

> 
> > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:79
> > +        bool connect(const URL&, const String& protocol);
> 
> If looks like the call to this function in
> WorkerThreadableWebSocketChannel::Bridge::connect does not handle the bool
> result. Should it?

I am not sure to understand.
WorkerThreadableWebSocketChannel::Bridge::connect calls WebSocketChannel::connect and make sure the worker is notified of the error if connect fails.

> 
> > Source/WebCore/page/RuntimeEnabledFeatures.h:369
> > +    bool isNSURLSessionWebSocketEnabled() const { return m_isNSURLSessionWebSocketEnabled; }
> > +    void setIsNSURLSessionWebSocketEnabled(bool isEnabled) { m_isNSURLSessionWebSocketEnabled = isEnabled; }
> 
> Should this be in HAVE(NSURLSESSION_WEBSOCKET)? It looks like it is only
> ever called within it.

We probably could do that by updating WebPreferences,yaml to make the preference condition as well.

> > Source/WebCore/page/RuntimeEnabledFeatures.h:556
> > +    bool m_isNSURLSessionWebSocketEnabled { false };
> 
> #if HAVE(NSURLSESSION_WEBSOCKET)?
> 
> > Source/WebKit/WebProcess/WebCoreSupport/ios/WebPreviewLoaderClient.h:29
> > +#include <WebCore/PageIdentifier.h>
> 
> Is this change intentional?

Unity build issue.

[Michael Catanzaro]

The problem with the GTK EWS is clash with Xlib.h's None. Anything including Xlib.h needs to never be #included with anything using the name None. With unified builds that becomes quite frustrating to ensure when adding new files.

E.g.:

In file included from /usr/include/X11/Xlib.h:44,
                 from /usr/include/EGL/eglplatform.h:119,
                 from ../DependenciesGTK/Root/include/wpe-1.0/wpe/renderer-backend-egl.h:38,
                 from ../DependenciesGTK/Root/include/wpe-1.0/wpe/wpe-egl.h:38,
                 from ../../Source/WebKit/WebProcess/WebPage/libwpe/AcceleratedSurfaceLibWPE.cpp:33,
                 from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-31.cpp:1:
DerivedSources/ForwardingHeaders/WebCore/SettingsBase.h:106:41: error: expected identifier before numeric constant
     enum class FontLoadTimingOverride { None, Block, Swap, Failure };
                                         ^~~~

Easiest solution would be to build AcceleratedSurfaceLibWPE.cpp with @no-unify. Ideally everything that #includes eglplatform.h would use @no-unify to ensure this can never be an issue, but we've been taking it one build failure at a time thus far.

[Sam Weinig]

(In reply to youenn fablet from comment #21)
> (In reply to Sam Weinig from comment #20)
> > Comment on attachment 371896 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=371896&action=review
> > 
> > > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:70
> > >      if (m_bridge)
> > >          m_bridge->connect(url, protocol);
> > > +    return true;
> > 
> > It seems a bit weird that we would return true here even if m_bridge is
> > null. Probably deserves a comment.
> 
> OK, returning true keeps the current behavior where connect can never fail
> synchronously for workers.

Is that a spec'd behavior?

> > 
> > > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:58
> > > +    bool connect(const URL&, const String& protocol) final;
> > 
> > What exactly does this return value indicate?
> 
> It indicates that attempting the connection failed synchronously.
> This allows to have the handling of whether firing the error event
> synchronously or asynchronously in WebSocket.
> This also allows to share more code between WebKit::WebSocketChannel and
> WebCore::WebSocketChannel.
> If we need to share more code, we could introduce MainThreadWebSocketChannel.

I think returning an enum would be preferable here. We generally try to only return bool when the function has a clear true/false meaning.

> 
> > 
> > > Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.h:79
> > > +        bool connect(const URL&, const String& protocol);
> > 
> > If looks like the call to this function in
> > WorkerThreadableWebSocketChannel::Bridge::connect does not handle the bool
> > result. Should it?
> 
> I am not sure to understand.
> WorkerThreadableWebSocketChannel::Bridge::connect calls
> WebSocketChannel::connect and make sure the worker is notified of the error
> if connect fails.

My mistake. I misread the diff.

[youenn fablet]

> > OK, returning true keeps the current behavior where connect can never fail
> > synchronously for workers.
> 
> Is that a spec'd behavior?

According the spec, we should call connect in parallel to the WebSocket constructor.
Should connect fail, the web application can know about it using the websocket onerror event handler.
WebKit follows the behavior but implements it differently in worker (connect is called asynchronously) than in documents (connect is called synchronously but the error event is dispatched asynchronously).

[youenn fablet]

> Easiest solution would be to build AcceleratedSurfaceLibWPE.cpp with
> @no-unify. Ideally everything that #includes eglplatform.h would use
> @no-unify to ensure this can never be an issue, but we've been taking it one
> build failure at a time thus far.

Thanks, Will do!

[youenn fablet]

Created attachment 371997 [details]
Fixing review comments

[EWS Watchlist]

Attachment 371997 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/websockets/ThreadableWebSocketChannel.h:63:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 371997 [details]
Fixing review comments

Clearing flags on attachment: 371997

Committed r246388: <https://trac.webkit.org/changeset/246388>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51691106>

[Daniel Bates]

Comment on attachment 371997 [details]
Fixing review comments

View in context: https://bugs.webkit.org/attachment.cgi?id=371997&action=review

> Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:87
> +    m_channel.didConnect();

This line broke my build! My local build fails to compile here saids missing an argument.... See <rdar://problem/52270069>. Haven't spent even a second thinking why EWS is happy....

[youenn fablet]

(In reply to Daniel Bates from comment #31)
> Comment on attachment 371997 [details]
> Fixing review comments
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371997&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/WebSocketTaskCocoa.mm:87
> > +    m_channel.didConnect();
> 
> This line broke my build! My local build fails to compile here saids missing
> an argument.... See <rdar://problem/52270069>. Haven't spent even a second
> thinking why EWS is happy....

This is a regression of https://trac.webkit.org/changeset/246877
See https://bugs.webkit.org/show_bug.cgi?id=199276 for a fix