Bug 198568

Summary: Use NSURLSession for WebSocket
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dbates, ews-watchlist, ggaren, mcatanzaro, rniwa, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=199151
https://bugs.webkit.org/show_bug.cgi?id=199189
https://bugs.webkit.org/show_bug.cgi?id=202095
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch
none
Fixing review comments none

Description youenn fablet 2019-06-05 09:10:58 PDT
Use NSURLSession for WebSocket
Comment 1 youenn fablet 2019-06-07 15:58:57 PDT
Created attachment 371620 [details]
Patch
Comment 2 youenn fablet 2019-06-07 16:11:18 PDT
Created attachment 371622 [details]
Patch
Comment 3 youenn fablet 2019-06-07 16:13:30 PDT
Created attachment 371623 [details]
Patch
Comment 4 youenn fablet 2019-06-07 16:35:52 PDT
Created attachment 371630 [details]
Patch
Comment 5 youenn fablet 2019-06-10 08:54:52 PDT
Created attachment 371747 [details]
Patch
Comment 6 youenn fablet 2019-06-10 09:17:03 PDT
Created attachment 371753 [details]
Patch
Comment 7 youenn fablet 2019-06-10 10:24:54 PDT
Created attachment 371759 [details]
Patch
Comment 8 youenn fablet 2019-06-10 10:40:35 PDT
Created attachment 371760 [details]
Patch
Comment 9 youenn fablet 2019-06-10 10:48:54 PDT
Created attachment 371761 [details]
Patch
Comment 10 youenn fablet 2019-06-10 10:56:10 PDT
Created attachment 371762 [details]
Patch
Comment 11 youenn fablet 2019-06-10 11:12:37 PDT
Created attachment 371763 [details]
Patch
Comment 12 youenn fablet 2019-06-10 12:13:10 PDT
Created attachment 371770 [details]
Patch
Comment 13 youenn fablet 2019-06-10 12:43:47 PDT
Created attachment 371771 [details]
Patch
Comment 14 EWS Watchlist 2019-06-10 14:21:28 PDT
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
Comment 15 EWS Watchlist 2019-06-10 14:21:30 PDT
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
Comment 16 youenn fablet 2019-06-10 14:37:00 PDT
Created attachment 371777 [details]
Patch
Comment 17 youenn fablet 2019-06-10 16:11:36 PDT
Created attachment 371789 [details]
Patch
Comment 18 youenn fablet 2019-06-11 16:41:48 PDT
Created attachment 371896 [details]
Patch
Comment 19 Geoffrey Garen 2019-06-12 11:12:56 PDT
Comment on attachment 371896 [details]
Patch

r=me
Comment 20 Sam Weinig 2019-06-12 12:11:05 PDT
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?
Comment 21 youenn fablet 2019-06-12 12:24:28 PDT
(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.
Comment 22 Michael Catanzaro 2019-06-12 13:11:02 PDT
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.
Comment 23 Sam Weinig 2019-06-12 13:38:47 PDT
(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.
Comment 24 youenn fablet 2019-06-12 14:57:18 PDT
> > 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).
Comment 25 youenn fablet 2019-06-12 15:05:52 PDT
> 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!
Comment 26 youenn fablet 2019-06-12 16:13:48 PDT
Created attachment 371997 [details]
Fixing review comments
Comment 27 EWS Watchlist 2019-06-12 16:15:05 PDT
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.
Comment 28 WebKit Commit Bot 2019-06-12 18:04:22 PDT
Comment on attachment 371997 [details]
Fixing review comments

Clearing flags on attachment: 371997

Committed r246388: <https://trac.webkit.org/changeset/246388>
Comment 29 WebKit Commit Bot 2019-06-12 18:04:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Radar WebKit Bug Importer 2019-06-12 18:05:51 PDT
<rdar://problem/51691106>
Comment 31 Daniel Bates 2019-06-27 10:57:30 PDT
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....
Comment 32 youenn fablet 2019-06-27 10:59:36 PDT
(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