Bug 201079 – [RemoteInspector][Socket] Restructuring the components of Socket implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Basuke Suzuki]

Since the change for WeakPtr on r248386, our port start assersion failure on the usage of RemoteInspectorSocketEndpoint. We have to send a message to connection client, but if that has to be done in the same thread which weakPtr generated, it's a little bit stronger ristriction for us to handle. In this restructure, we are stopping to use weakPtr to resolve circular dependency, but using a reference with invalidation method because everything is under our control.

[Basuke Suzuki]

Created attachment 377147 [details]
PATCH

[Basuke Suzuki]

- Make SocketEndpoint a singleton. This class represents a central place to handle socket connections and there's no need to instantiate more than one in a process. Once every connection goes away, it just start sleeping until next connection is created. Very low resource usage.
- Move Socket::Connection structure from global definition to SocketEndpoint local structure. It is directly used in SocketEndpoint privately.
- Move responsibility to handle message encoding/decoding task from SocketEndpoint to ConnectionClient. Make SocketEndpoint as plain socket handling as possible to keep it simple to exist long span.
- Extract an interface from ConnectionClient as SocketEndpoint::Client which is required to work with SocketEndpoint. Now SocketEndpoint is very independent from others. SocketEndpoint::Client is the required parameter to create a connection.
- Remove ConnectionID member variable from MessageParser. It now exists in closure to receive decoded message.
- Many responsibilities are moved into ConnectionClient which was a thin interface for communication between RemoteInspector, RemoteInspectorServer and RemoteInspectorClient. It now handles followings:
  - life cycle of connection: create, listen and close or invalidation
  - sending and receiving data packed in a message.
- RemoteInspector, RemoteInspectorServer and RemoteInspectorClient are now free from creation of SocketEndpoint. All communication

[Basuke Suzuki]

Circular dependency among subclass of ConnectionClient (RemoteInspector, RemoteInspectorServer, RemoteInspectorClient) and SocketEndpoint is solved like this:
- SocketEndpoint has no knowledge about ConnectionClient.
- It keeps list of Connection object for each socket.
- When ConnectionClient try to create a connection, ask shared SocketEndpoint (now a singleton per process) with itself's reference. Connection object keep that reference in it. No weakPtr, no refCount.
- ConnectionClient must invalidate the connections which tied itself when the object is going away.

[Ross Kirsling]

Comment on attachment 377147 [details]
PATCH

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

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorConnectionClient.cpp:77
> +        auto result = m_parsers.ensure(clientID, [&] {

Should we make the captures explicit here?

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorConnectionClient.h:50
> +    void didReceive(ConnectionID, Vector<uint8_t>&&);

Isn't this an override too?

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:47
> +        virtual void didAccept(ConnectionID /* acceptedID */, ConnectionID /* listenerID */, Socket::Domain) = 0;

Do we get an error if these aren't commented out? (I can't remember...)

> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:56
> +    Optional<ConnectionID> connectInet(const char* serverAddr, uint16_t serverPort, RemoteInspectorSocketEndpoint::Client&);

Throughout this header and its implementation, it seems like RemoteInspectorSocketEndpoint::Client& could just be Client&, no?

> Source/WebKit/ChangeLog:8
> +        Since the change for WeakPtr on r248386, our port start assertion failure on the usage of

I'm not sure you need to write the full description in multiple places -- I think one place should be enough?

[Basuke Suzuki]

Comment on attachment 377147 [details]
PATCH

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

>> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorConnectionClient.cpp:77
>> +        auto result = m_parsers.ensure(clientID, [&] {
> 
> Should we make the captures explicit here?

Right. That's more secure.

>> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorConnectionClient.h:50
>> +    void didReceive(ConnectionID, Vector<uint8_t>&&);
> 
> Isn't this an override too?

My fault.

>> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:47
>> +        virtual void didAccept(ConnectionID /* acceptedID */, ConnectionID /* listenerID */, Socket::Domain) = 0;
> 
> Do we get an error if these aren't commented out? (I can't remember...)

Ah, I don't remember exactly, but it is possible clang won't generate warning for abstract function. I'll try.

>> Source/JavaScriptCore/inspector/remote/socket/RemoteInspectorSocketEndpoint.h:56
>> +    Optional<ConnectionID> connectInet(const char* serverAddr, uint16_t serverPort, RemoteInspectorSocketEndpoint::Client&);
> 
> Throughout this header and its implementation, it seems like RemoteInspectorSocketEndpoint::Client& could just be Client&, no?

Right. I'll replace as much as possible. Thanks!

>> Source/WebKit/ChangeLog:8
>> +        Since the change for WeakPtr on r248386, our port start assertion failure on the usage of
> 
> I'm not sure you need to write the full description in multiple places -- I think one place should be enough?

Okay. I make it simple.

[Basuke Suzuki]

Created attachment 377275 [details]
PATCH

Fixed reviewed points.

[WebKit Commit Bot]

Comment on attachment 377275 [details]
PATCH

Clearing flags on attachment: 377275

Committed r249158: <https://trac.webkit.org/changeset/249158>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54757673>