Bug 193806 – [PlayStation] Upstream playstation's remote inspector server

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Christopher Reid]

This is initial work to upstream our remote inspector server implementation.
We are also looking at upstreaming a wincairo remote inspector client and server using this protocol.
We are using wincairo clients for inspecting playstation targets.

[Christopher Reid]

Created attachment 360074 [details]
patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 11 files


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

[Don Olmstead]

Comment on attachment 360074 [details]
patch

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

Fix WPE and let's run this by someone with more remote inspector experience. Overall looks good.

> Source/JavaScriptCore/SourcesGTK.txt:31
> -inspector/remote/glib/RemoteConnectionToTargetGlib.cpp
> +inspector/remote/generic/RemoteConnectionToTargetGeneric.cpp

You forgot to do this with WPE hence the build failure.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:70
> +typedef RefPtr<TargetInfo> TargetListing;

Should prefer using.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:40
> +ConnectionIdentifier RemoteInspector::connectionIdentifier = -1;

I saw -1 used in a few places. Probably want that to be a constant if you're using that.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:132
> +    std::lock_guard<Lock> lock(m_mutex);

I'm not sure about these uses of std::lock_guard. It is used quite a bit in bmalloc but other than that its used sporadically.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:240
> +    return;

Remove

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:245
> +    return;

Remove

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:40
> +#if OS(UNIX)
> +#include <poll.h>
> +#endif

Remove the #if we know its UNIX.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:40
> +#if OS(UNIX)
> +#include <arpa/inet.h>
> +#include <netinet/in.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>
> +#endif

Remove the #if

[Stephan Szabo]

Comment on attachment 360074 [details]
patch

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

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:77
> +    Otional<String> message { std::nullopt };

Typo for Optional?

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:87
> +    m_socketConnection->send(event.utf8().data(), event.utf8().length());

A local might be nicer than hoping event.utf8() ends up only being evaluated once.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:95
> +        RefPtr<JSON::Value> messageValue;

Is there a reason we're doing the parsing on the main thread rather than doing that work whichever thread the receive was happening on?

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:132
>> +    std::lock_guard<Lock> lock(m_mutex);
> 
> I'm not sure about these uses of std::lock_guard. It is used quite a bit in bmalloc but other than that its used sporadically.

It appears to be done this way in RemoteInspectorGlib and RemoteInspectorCocoa
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/inspector/remote/glib/RemoteInspectorGlib.cpp#L60
https://github.com/WebKit/webkit/blob/master/Source/JavaScriptCore/inspector/remote/cocoa/RemoteInspectorCocoa.mm#L222

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServer.h:74
> +    Vector<int> m_inspectorConnections; // webprocess connections

Should this and m_clientConnection be using ClientIDs? It seems like some of the places like didAccept, etc, are taking ClientIDs and then working on this with them. Or should other things

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:140
> +            WTFLogAlways("[Server] Got event: %s, clientID: %d connectionID: %d targetID: %d message: %s", 

Remove or switch to something other than WTFLogAlways.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:45
> +#if PLATFORM(PLAYSTATION)

Like Don's comment above, we should know this is a playstation here.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:46
> +using ConnectionIdentifier = int;

Do we need both ClientID and ConnectionIdentifier? And like with ClientID, are there things here that are defined as int that would be better using this (or if they're folded together, whichever survived)?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:88
> +    int m_wakeupSocket;

Should we set this to -1 in case the initialization for it fails?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:131
> +    int addServerConnectionIdentifier(ConnectionIdentifier);

Should this be a ClientID return to match how it's used in RemoteInspectorServer?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:48
> +constexpr size_t SizeOfRecvBuffer = 65536;

Nitpick, this appears to be used for both SO_RCVBUF and SO_SNDBUF, so SizeOfRecvBuffer seems like a poor name choice.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:97
> +    for (size_t i = 0; i < numberOfConnections; i++) {

Probably <= or using m_connections.size()? Otherwise it seems like if the socketpair below fails we're left with the last one unset.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:256
> +    ::write(m_wakeupSocket, "1", 1);

It seems like we should check we have one that was made correctly before writing to it.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:502
> +        memcpy(&dataBuffer[0], &m_buffer[sizeof(int)], dataSize);

I don't have a better idea off-hand that isn't much more complicated, but it seems like we need to fix this to not end up with having the data in both m_buffer and dataBuffer at the same time because of memory usage.

[Christopher Reid]

Created attachment 360938 [details]
[WIP] Inspector Server

There's still quite a few items that need sorting out. I'm putting this WIP patch looking for feedback/suggestions and to see if these changes are going in the right direction.

These changes implement a Remote Inspector that runs over TCP. It uses a JSON RPC protocol, the first 4 bytes of the tcp message will contain the JSON message size and the following bytes are the JSON message.

I was able to use this server on WebKitGTK using a rough proof of concept patch. I can inspect a simple page but quickly run into bugs that still need sorting out. I'll attach that patch below.

Note: A item we plan on changing is the connectionID key in the JSON protocol. It currently corresponds to an index in the m_connections Vector. I would like to change that m_connections Vector to a HashMap and use some sort of random uid as the key. Doing that also allows us to remove the m_connection client limit.

[Christopher Reid]

Created attachment 360940 [details]
Proof of Concept

Rough patch used for testing on GTK

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:128:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:205:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:40:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:175:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:176:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:387:  One line control clauses should not use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:461:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:462:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 11 in 10 files


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

[Joseph Pecoraro]

Comment on attachment 360938 [details]
[WIP] Inspector Server

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

I did a quick once over and this looks good. I didn't dig into any of the threading complexity, but everything seems good! Let me know when this needs a more complete review.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:188
> +    info->type = (target.type() == RemoteInspectionTarget::Type::Web)? "Web" : "JavaScript";

Nit: "Web"_s and "JavaScript"_s to avoid unnecessary string copies.

Technically there are 3 types you may want to support:

    RemoteInspectionTarget::Type::JavaScript
    RemoteInspectionTarget::Type::ServiceWorker
    RemoteInspectionTarget::Type::Web

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:156
> +bool RemoteInspectorServer::start(const char* /* address */, unsigned port)

Address is never used, and probably not something you want anyways. Seems like it can be removed.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:51
> +    +--------------+----------------------------------+--------------
> +    |    size      |               data               | (next message)
> +    |    4byte     |          variable length         |
> +    +--------------+----------------------------------+--------------

Given you are expecting 4 bytes, you may want to use int32_t or uint32_t below. The Cocoa implementations which communicate over TCP (which has a similar ASCII art!) uses a uint32_t. Of course in practice messages don't really come close to that full size.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:419
> +    saServer.sin_family = AF_INET;

So you want to also support IPv6 (AF_INET6)?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:454
> +Vector<uint8_t> MessageParser::createMessage(const void* data, size_t size)

Given you are interpreting the data to `const uint8_t*`, why not specify that in the signatures (MessageParser::createMessage, RemoteInspectorSocket::send). I think that is a more common type for arbitrary data.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:464
> +    auto messageBuffer = Vector<uint8_t>(size + sizeof(int));
> +    int intSize = static_cast<int>(size);
> +    //messageBuffer.append(&intSize, sizeof(int));
> +    //messageBuffer.append(reinterpret_cast<const uint8_t*>(data), intSize);
> +    memcpy(&messageBuffer[0], &intSize, sizeof(int));
> +    memcpy(&messageBuffer[sizeof(int)], data, intSize);

These `int` could be `int32_t`

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:488
> +        if (m_buffer.size() < sizeof(int)) {

Likewise everywhere in here.

[Joseph Pecoraro]

> Note: A item we plan on changing is the connectionID key in the JSON
> protocol. It currently corresponds to an index in the m_connections Vector.
> I would like to change that m_connections Vector to a HashMap and use some
> sort of random uid as the key. Doing that also allows us to remove the
> m_connection client limit.

Yeah, this is a good idea =)

[Christopher Reid]

(In reply to Joseph Pecoraro from comment #8)
> Comment on attachment 360938 [details]
> [WIP] Inspector Server
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360938&action=review

Thanks for the feedback!

> > Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:188
> > +    info->type = (target.type() == RemoteInspectionTarget::Type::Web)? "Web" : "JavaScript";
> 
> Nit: "Web"_s and "JavaScript"_s to avoid unnecessary string copies.
> 
> Technically there are 3 types you may want to support:
> 
>     RemoteInspectionTarget::Type::JavaScript
>     RemoteInspectionTarget::Type::ServiceWorker
>     RemoteInspectionTarget::Type::Web
> 
I believe we eventually need to support inspecting all three types, I'll update it.

> > Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:156
> > +bool RemoteInspectorServer::start(const char* /* address */, unsigned port)
> 
> Address is never used, and probably not something you want anyways. Seems
> like it can be removed.
> 
Will remove.

> > Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:51
> > +    +--------------+----------------------------------+--------------
> > +    |    size      |               data               | (next message)
> > +    |    4byte     |          variable length         |
> > +    +--------------+----------------------------------+--------------
> 
> Given you are expecting 4 bytes, you may want to use int32_t or uint32_t
> below. The Cocoa implementations which communicate over TCP (which has a
> similar ASCII art!) uses a uint32_t. Of course in practice messages don't
> really come close to that full size.
> 
That makes sense, I'll update all those suggested types.

> > Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:419
> > +    saServer.sin_family = AF_INET;
> 
> So you want to also support IPv6 (AF_INET6)?
Yeah, we should add support for IPv6. It could potentially be a followup patch.

[Christopher Reid]

Created attachment 361665 [details]
Inspector Server

Fixing up review items and storing connections as a HashMap with random IDs as keys.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:38:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 10 files


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

[EWS Watchlist]

Comment on attachment 361665 [details]
Inspector Server

Attachment 361665 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11106873

New failing tests:
fast/viewport/ios/device-width-viewport-after-changing-view-scale.html

[EWS Watchlist]

Created attachment 361668 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Christopher Reid]

Created attachment 361701 [details]
Inspector Server

Fixing some smaller items I noticed since the last patch.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 10 files


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

[Christopher Reid]

(In reply to Build Bot from comment #16)
> Attachment 361701 [details] did not pass style-queue:
> 
> 
> ERROR:
> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:
> 37:  Alphabetical sorting problem.  [build/include_order] [4]
> ERROR:
> Source/JavaScriptCore/inspector/remote/playstation/
> RemoteInspectorSocketPlayStation.cpp:34:  Alphabetical sorting problem. 
> [build/include_order] [4]
> Total errors found: 2 in 10 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

I believe the style bot is wrong here https://webkit.org/code-style-guidelines/#include-system

This patch is also ready for a more in depth review.

[Christopher Reid]

Created attachment 361707 [details]
Inspector Server

Fixing a build error

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:34:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2 in 10 files


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

[Joseph Pecoraro]

Comment on attachment 361707 [details]
Inspector Server

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

Overall this looks great! I identified what looks like a few possible threading issues, concerns around assertions in case of handling messages from a malicious/bad client, and a few generic questions. The rest is mostly just style comments. I'd like to see another patch just to see how changes were addressed. I'm not entirely familiar with lower level socket APIs, but everything seems straightforward and you've gotten a few other reviewers to look it over. Address this round of feedback and I'll happily do another review.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:64
> +    uint64_t targetIdentifier;

Initialize this to 0?

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:68
> +    bool hasLocalDebugger;

Initialize this to 0?

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:77
> +    Optional<uint64_t> connectionID { WTF::nullopt };
> +    Optional<uint64_t> targetID { WTF::nullopt };
> +    Optional<String> message { WTF::nullopt };

These initializations are unnecessary, an Optional<T> should default initialize as nullopt.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:70
> +    // FIXME:
> +    // glib port doesn't implement event handler for the "closed" signal.
> +    // we should consider whether this function is unnecessary,
> +    // or should call receivedCloseMessage() instead.

Currently this `didClose` does nothing, but it probably should. If the Server connection fails this could do some cleanup such as remove connection to targets that don't exist. The Cocoa implementation is very similar to stopInternal.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:88
> +    if (!m_clientID)
> +        return;

Given this, then m_clientID needs to be cleared when m_socketConnection is cleared. Otherwise we could have a clientID but not a socketConnect below and crash.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:174
> +    m_socketConnection = nullptr;

This probably wants:

    m_clientID = WTF::nullopt;

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:179
> +

Style: Remove accidental empty line.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:204
> +    // FIXME: not implemented

The FIXME is not useful as written. Perhaps it could point to a bugzilla bug about supporting WebDriver / Automation; or just remove the comment.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:242
> +    WTF::RunLoop::current().dispatch([=] {

Style: You can drop the "WTF::" as you use RunLoop unprefixed elsewhere in this file.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:266
> +    sendMessageEvent->setString("message"_s, message.utf8().data());

You should be able to just set `message` and avoid `message.utf8().data()`. As written it looks like it is just copying the string.

Ultimately the `toJSONString` should transform the data to UTF8 for you, and if it doesn't then that would be a bug worth investigating.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServer.h:75
> +    Vector<ClientID> m_inspectorConnections; // webprocess connections
> +    Optional<ClientID> m_clientConnection { WTF::nullopt }; // connection from RemoteInspectorClient

Style: Comments should start with a capital and end with a period.
Nit: The Optional doesn't require explicit initialization, it will default to nullopt.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:155
> +bool RemoteInspectorServer::start(const char* /* address */, unsigned port)

Drop `address` from the signature?

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:186
> +    // FIXME: This shouldn't recreate the JSON event when the message doesn't change

You could drop this comment. This optimization should only be done if it is shown to be a win in real world use. In practice listings are small, rare, and already asynchronous, so this probably won't be a problem.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:202
> +    for (auto connection : m_inspectorConnections)
> +        sendWebInspectorEvent(connection, setupEvent->toJSONString());

`m_inspectorConnections` looks like it can be used here on the MainThread and modified on the RemoteInspectorSocket's worker thread via `RemoteInspectorServer::didAccept`. As written it would need some kind of lock across access and mutation to prevent threading related crashes for the unlikely case that a new connection comes in while this happens. Or perhaps didAccept could bounce over to the main thread.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:234
> +    ASSERT(m_inspectionTargets.contains(std::make_pair(*event.connectionID, *event.targetID)));

Asserts like this are potentially bad. A bad client could make a connection to the RemoteInspectorServer and send it random events, such as a `FrontendDidClose` for a connection/target that does not exist. So I'd suggest actual checks. In this case it appears to be harmless other then triggering some ASSERTs.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:285
> +    ASSERT(event.clientID != *m_clientConnection);

This is another assertion that doesn't seem safe given a bad actor.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:36
> +#include <wtf/WorkerPool.h>

Not needed?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:74
> +    void setDidReceiveDataListener(Function<void(ClientID, Vector<uint8_t>&&)>&& listener)
> +    {
> +        m_didReceiveDataListener = WTFMove(listener);
> +    }
> +
> +    void setDidCloseListener(Function<void(ClientID)>&& listener)
> +    {
> +        m_didCloseListener = WTFMove(listener);
> +    }
> +
> +    void setDidAcceptListener(Function<void(ClientID, DomainType)>&& listener)
> +    {
> +        m_didAcceptListener = WTFMove(listener);
> +    }

Style: You can place these all on one line, they are all simple setters.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:94
> +    HashMap<ClientID, std::unique_ptr<struct connection>> m_connections;

It looks like `m_connections` is accessed on both the Worker Thread (in `::workerThread`) and mutated on a client thread (in RemoteInspectorSocket::send). Sometimes accesses are guarded by `m_lock` but sometimes it is not. Seems like there will be cases of simultaneous access unless there is a mutex/lock or serializing access on a single thread/queue.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:100
> +    Lock m_lock;

What is m_lock guarding specifically?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:109
> +

Style: Remove accidental empty line.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:40
> +#include <wtf/MainThread.h>
> +#include <wtf/RandomNumber.h>
> +#include <wtf/text/WTFString.h>
> +
> +#include <arpa/inet.h>
> +#include <fcntl.h>
> +#include <netinet/in.h>
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +#include <unistd.h>

Style: In this file and others, just group these all together and sorted as `sort` would do.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:44
> +constexpr size_t SocketBufferSize = 65536;

How did you arrive at this value (64kb)? The vast majority of inspector messages will fit into that size. However there are some inspector messages that will certainly end up larger (for example image resources >64kb that have been base64 encoded). It may be useful to increase this if resources allow. Just something worth considering.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:63
> +    void setDidParseMessageListener(Function<void(ClientID, Vector<uint8_t>)>&& listener)
> +    {
> +        m_didParseMessageListener = WTFMove(listener);
> +    }

Style: You can place this all on one line, it is a simple setter.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:85
> +// -------------------------------------------------------------------------
> +// RemoteInspectorSocket
> +// -------------------------------------------------------------------------

This comments style is unusual for WebKit source. It is also unusual to have multiple classes in one header/implementation, even when they are as related as these. You could consider breaking them out into their own files:

    RemoteInspectorSocket.h/cpp
    RemoteInspectorSocketClient.h/cpp
    RemoteInspectorSocketServer.h/cpp

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:95
> +    m_workerThread = Thread::create("Remote Inspector Worker"_s, [this] {

You should not use the `_s` suffix here. Just use a character literal.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:128
> +            return out;

`out` is an `int` and the return value is a `bool`. Can this be a more specific int comparison resulting in a bool? Perhaps `out != 0` or `out == 1`?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:185
> +    ClientID id;
> +    do {
> +        id = std::numeric_limits<ClientID>::max() * randomNumber();
> +    } while (m_connections.contains(id));

You probably also want to ignore `0`.

    while (!id && m_connections.contains(id))

And why not just make this a random number, why is there the multiplication at all:

   id = cryptographicallyRandomNumber();

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:213
> +            LOG_ERROR("read error (errno = %d)", errno);

Should you also close in the negative read case?

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:237
> +        // some data remains or error

Style: Comments in this file should be full sentences that start with a capital and end with a period.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:262
> +                return writeSize;

This looks like it could be `return true`.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:275
> +        return writeSize;

And this `return false`.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:286
> +

Style: Remove accidental empty line.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:375
> +

Style: Remove accidental empty line.

[Joseph Pecoraro]

Oh, and of course there should be ChangeLogs in the next patch! See `prepare-ChangeLog`  or webkit-patch:
https://webkit.org/contributing-code/

[Carlos Garcia Campos]

Comment on attachment 361707 [details]
Inspector Server

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

I haven't had time to review the whole patch, so I'm adding only a few comments for now

> Source/JavaScriptCore/PlatformPlayStation.cmake:13
> +    inspector/remote/generic/RemoteInspectorServer.h

This depends on RemoteInspectorSocket which is under playstation directory, so I would move this there too for now, because it's not really generic.

> Source/JavaScriptCore/PlatformPlayStation.cmake:28
> +    inspector/remote/generic/RemoteInspectorGeneric.cpp
> +    inspector/remote/generic/RemoteInspectorServerGeneric.cpp

Ditto.

> Source/JavaScriptCore/SourcesGTK.txt:31
> -inspector/remote/glib/RemoteConnectionToTargetGlib.cpp
> +inspector/remote/generic/RemoteConnectionToTargetGeneric.cpp

I'm not a fan of the name 'generic', why not simply add RemoteConnectionToTarget.cpp if it's really generic? Apple ports can still use its own platform specific implementation in their platform specific file.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:72
> +class InspectorEvent {

This could probably be just a struct.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:165
> +    void setup(unsigned targetIdentifier);
> +    void sendMessageToTarget(unsigned targetIdentifier, const char* message);

You can split the GLIB ifdef to share the common signatures.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:222
> +    using RemoteInspectorCall = void (RemoteInspector::*)(const InspectorEvent&);

I would use MessageHandler or MethodHandler, or even CallHandler here instead of just Call

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:258
> +    static PlatformSocketType connectionIdentifier;

s_connectionIdentifier

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:90
> +    const CString& message = event.utf8();

String::utf8() doesn't return a const reference, but a new CString.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:98
> +    auto jsonData = String::adopt(WTFMove(data));

You are converting to utf8 before sending, I think you should convert back using String::fromUTF8() when receiving, no?

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:114
> +    InspectorEvent event;
> +    uint64_t connectionID;
> +    uint64_t targetID;
> +    String message;

Move these where they are first needed.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:117
> +        event.connectionID = Optional<uint64_t>(connectionID);

I don't think you need the explicit Optional<uint64_t>()

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:120
> +        event.targetID = Optional<uint64_t>(targetID);

Ditto.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:123
> +        event.message = Optional<String>(WTFMove(message));

Ditto.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:151
> +    m_socketConnection->setDidReceiveDataListener([this](ClientID, Vector<uint8_t>&& data) {
> +        didReceiveData(WTFMove(data));
> +    });
> +    m_socketConnection->setDidCloseListener([this](ClientID) {
> +        didClose();
> +    });

I would add a Client class that RemoteInspector class can derive from, received as argument by RemoteInspectorSocketClient constructor instead of using these listeners approach.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:187
> +    TargetInfo* info = new TargetInfo();

Use RefPtr directly here, it could be Ref<> instead of RefPtr

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:224
> +    auto targetListJSON = JSON::Array::create();
> +    for (auto listing : m_targetListingMap.values()) {
> +        auto target = JSON::Object::create();
> +        target->setInteger("targetID"_s, listing->targetIdentifier);
> +        target->setString("type"_s, listing->type);
> +        target->setString("name"_s, listing->name);
> +        target->setString("url"_s, listing->url);
> +        target->setBoolean("hasLocalDebugger"_s, listing->hasLocalDebugger);
> +        targetListJSON->pushObject(WTFMove(target));
> +    }

Why don't you use JSON::Array directly as TargetListing? I don't think we need the TargetInfo class is really needed.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:38
> +    if (m_server) {

Use an early return here.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:102
> +    const CString& message = event.utf8();

CString message

[Devin Rousso]

Comment on attachment 361707 [details]
Inspector Server

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

>> Source/JavaScriptCore/SourcesGTK.txt:31
>> +inspector/remote/generic/RemoteConnectionToTargetGeneric.cpp
> 
> I'm not a fan of the name 'generic', why not simply add RemoteConnectionToTarget.cpp if it's really generic? Apple ports can still use its own platform specific implementation in their platform specific file.

I agree.  Our usual style is to have the "generic"/"common"/"base" class in the "top-level" folder (which is inspector/remote/ in this case), and have any platform specific code in subfolders named after that platform (e.g. inspector/remote/PlayStation/).

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:70
> +using TargetListing = RefPtr<TargetInfo>;

I personally don't like these types of "simple" aliases, as it can make code harder to follow (unless you have a smarter IDE).

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:222
>> +    using RemoteInspectorCall = void (RemoteInspector::*)(const InspectorEvent&);
> 
> I would use MessageHandler or MethodHandler, or even CallHandler here instead of just Call

Inspector's generated backend dispatcher code uses `CallHandler`.

> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:223
> +    HashMap<String, RemoteInspectorCall>& methodTable();

Inspector's generated backend dispatcher code calls this the `dispatchMap`.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:91
> +    m_socketConnection->send(*m_clientID, reinterpret_cast<const uint8_t*>(message.data()), message.length());

When using `Optional`s, I prefer calling `.value()` over `*` to retrieve the value.  This makes it clearer that the value is an optional, not a pointer.

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:137
> +    std::lock_guard<Lock> lock(m_mutex);

Any reason not to use `LockHolder`?

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:187
>> +    TargetInfo* info = new TargetInfo();
> 
> Use RefPtr directly here, it could be Ref<> instead of RefPtr

Many classes have a `::create` that follows this pattern.  I'd recommend copying that (specifically with a `Ref`).

    adoptRef(*new TargetInfo())

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:191
> +    info->hasLocalDebugger = target.hasLocalDebugger();

Just to make sure, this means that if `hasLocalDebugger` is false, but we have `TargetInfo`, then there must be a remote debugger?

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:243
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:259
> +    std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:274
> +    std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:283
> +        setup(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:295
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:296
> +        connectionToTarget = m_targetConnectionMap.get(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:301
> +    connectionToTarget->sendMessageToTarget(*event.message);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:313
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:314
> +        RemoteControllableTarget* target = m_targetMap.get(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:318
> +        connectionToTarget = m_targetConnectionMap.take(*event.targetID);

Ditto (>91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:330
> +        std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:343
> +    std::lock_guard<Lock> lock(m_mutex);

Ditto (>137).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:190
> +    targetListEvent->setString("message"_s, *event.message);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:212
> +    m_inspectionTargets.add(std::make_pair(*event.connectionID, *event.targetID));

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:216
> +    setupEvent->setInteger("targetID"_s, *event.targetID);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:236
> +    sendCloseEvent(*event.connectionID, *event.targetID);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:237
> +    m_inspectionTargets.remove(std::make_pair(*event.connectionID, *event.targetID));

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:261
> +        sendWebInspectorEvent(*m_clientConnection, closedEvent->toJSONString());

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:274
> +    sendEvent->setInteger("targetID"_s, *event.targetID);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:275
> +    sendEvent->setString("message"_s, *event.message);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:276
> +    sendWebInspectorEvent(*event.connectionID, sendEvent->toJSONString());

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:292
> +    sendEvent->setInteger("targetID"_s, *event.targetID);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:294
> +    sendEvent->setString("message"_s, *event.message);

Ditto (>RemoteInspectorGeneric.cpp:91).

> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:295
> +    sendWebInspectorEvent(*m_clientConnection, sendEvent->toJSONString());

Ditto (>RemoteInspectorGeneric.cpp:91).

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:275
>> +        return writeSize;
> 
> And this `return false`.

It's possible that `writeSize` is not equal to `0` at this point, so this one might be valid.

[Joseph Pecoraro]

Comment on attachment 361707 [details]
Inspector Server

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

>>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:275
>>> +        return writeSize;
>> 
>> And this `return false`.
> 
> It's possible that `writeSize` is not equal to `0` at this point, so this one might be valid.

Good point. What does the boolean return value from this even mean? It looks like it just means "sent some data" or "sent no data" which doesn't appear to be useful given it buffers what was not sent. Either way, the return value is unused, so maybe it is just easier to have a void return then.

[Christopher Reid]

Comment on attachment 361707 [details]
Inspector Server

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

I'm still working on an updated patch addressing all the feedback, just adding some input.

>> Source/JavaScriptCore/PlatformPlayStation.cmake:13
>> +    inspector/remote/generic/RemoteInspectorServer.h
> 
> This depends on RemoteInspectorSocket which is under playstation directory, so I would move this there too for now, because it's not really generic.

I agree they aren't completely generic, I was using that term since we do eventually want to also share these files with WinCario. I'll move them over to playstation for now.

>> Source/JavaScriptCore/inspector/remote/RemoteInspector.h:70
>> +using TargetListing = RefPtr<TargetInfo>;
> 
> I personally don't like these types of "simple" aliases, as it can make code harder to follow (unless you have a smarter IDE).

I think it's currently needed for this case since the TargetListing type is used in shared code and glib/cocoa both have their own aliases to their own types.

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:137
>> +    std::lock_guard<Lock> lock(m_mutex);
> 
> Any reason not to use `LockHolder`?

Steph mentioned this above for an earlier patch, std::lock_guard is consistent with how that m_mutex is acquired in the shared RemoteInspector.cpp and cocoa/glib code. I agree LockHolder makes sense here but updating cocoa/glib seems a bit out of scope for this patch.

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorGeneric.cpp:191
>> +    info->hasLocalDebugger = target.hasLocalDebugger();
> 
> Just to make sure, this means that if `hasLocalDebugger` is false, but we have `TargetInfo`, then there must be a remote debugger?

FWIU the `TargetInfo`/`TargetListing` objects are created and maintained without necessarily having a remote debugger connected yet. I believe this is the case so when a remote inspector does connect, only m_targetListingMap needs sending instead of having to query WebProcesses/UIProcess for inspectable targets.

>> Source/JavaScriptCore/inspector/remote/generic/RemoteInspectorServerGeneric.cpp:155
>> +bool RemoteInspectorServer::start(const char* /* address */, unsigned port)
> 
> Drop `address` from the signature?

Oops, I forgot to do that for this patch.

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:100
>> +    Lock m_lock;
> 
> What is m_lock guarding specifically?

Yeah, it's meant to guard that m_connections object. I'll make the name clearer and ensure it's guarding all accesses.

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:44
>> +constexpr size_t SocketBufferSize = 65536;
> 
> How did you arrive at this value (64kb)? The vast majority of inspector messages will fit into that size. However there are some inspector messages that will certainly end up larger (for example image resources >64kb that have been base64 encoded). It may be useful to increase this if resources allow. Just something worth considering.

Our UIProcess is currently pretty memory limited so having smaller receiving buffers seems safer for now but it's something we should look at.

[Christopher Reid]

Created attachment 362388 [details]
Inspector Server

Updated patch

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 19 files


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

[Joseph Pecoraro]

Comment on attachment 362388 [details]
Inspector Server

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

This looks good to me. One remaining threading issue you may want to address.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorConnectionClient.h:44
> +    struct InspectorEvent {

I suppose this could just be named `Event` and not `InspectorEvent` given this is inside RemoteInspectorConnectionClient.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorConnectionClient.h:51
> +
> +

Style: Remove accidental empty line.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorMessageParser.h:39
> +    /*
> +    | <--- one message for send / didReceiveData ---> |
> +    +--------------+----------------------------------+--------------
> +    |    size      |               data               | (next message)
> +    |    4byte     |          variable length         |
> +    +--------------+----------------------------------+--------------
> +                   | <------------ size ------------> |
> +    */

This is already in the implementation file. It is probably only needed in one place.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorMessageParser.h:48
> +    void setDidParseMessageListener(Function<void(ClientID, Vector<uint8_t>)>&& listener) { m_didParseMessageListener =
> +WTFMove(listener); }

Style: Remove accidental newline

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorMessageParser.h:54
> +    bool parse();
> +    Function<void(ClientID, Vector<uint8_t>&&)> m_didParseMessageListener;

Style: Add a newline between the methods and members.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorServerPlayStation.cpp:56
> +        m_inspectorConnections.append(clientID);

`m_inspectorConnections` still looks like it has multi-threaded mutation / access issues. There are functions with ASSERT(isMainThread()) and ASSERT(!isMainThread()) that mutate or access this map without a lock. This is very likely to be rare, especially if there is only 1 client in the Inet case, but given it is possible we should probably address that now if we can.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:71
> +    struct connection {

Style: Normally struct/class names are capitalized. I'd have expected `struct Connection` here.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocket.h:79
> +    Lock m_connectionsLock;
> +    HashMap<ClientID, std::unique_ptr<struct connection>> m_connections;

Nice! This looks cleaner, I don't see any threading issues here now.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:194
> +        if (m_inspectorClient)
> +            m_inspectorClient->didClose(clientID);

Nit: You could call the client without the lock.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:271
> +        if (newID && m_inspectorClient)
> +            m_inspectorClient->didAccept(newID.value(), DomainType::Inet);

Nit: You could call the client without the lock.

[Christopher Reid]

Created attachment 362940 [details]
Inspector Server

Updated patch, fixing potential threading issue with m_inspectorConnections. Also fixed some build issues when compiling these latest changes on PlayStation.

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 19 files


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

[Christopher Reid]

Comment on attachment 362388 [details]
Inspector Server

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

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:271
>> +            m_inspectorClient->didAccept(newID.value(), DomainType::Inet);
> 
> Nit: You could call the client without the lock.

I think this case is okay, the lock is already released before the createClient call.

[Joseph Pecoraro]

Comment on attachment 362940 [details]
Inspector Server

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

r=me!

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketClient.h:42
> +    RemoteInspectorSocketClient(RemoteInspectorConnectionClient*);

Style: You can make this single argument constructor `explicit`.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketClientPlayStation.cpp:48
> +    struct sockaddr_in saServer = { 0 };
> +    saServer.sin_family = AF_INET;

The names in here that are abbreviations are unusual for WebKit style (`saServer`, `ret`).

Normally I see initialization with memset:

    memset(&saServer, 0, sizeof(struct sockaddr_in));

I'm not sure if the { 0 } alone would be enough.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketServer.h:41
> +    RemoteInspectorSocketServer(RemoteInspectorConnectionClient*);

Style: You can make this single argument constructor `explicit`.

> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketServer.h:43
> +    bool listenInet(int port);

Not important but if you want you could reduce the type of port throughout this patch if you want. Most of the time a port is a uint16_t and not exactly an int. In your patches it ultimately goes into a sockaddr_in.sin_port.

[Christopher Reid]

Created attachment 363397 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 19 files


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

[Christopher Reid]

Created attachment 363398 [details]
Patch for landing

[EWS Watchlist]

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


ERROR: Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlayStation.cpp:100:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 19 files


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

[WebKit Commit Bot]

Comment on attachment 363398 [details]
Patch for landing

Clearing flags on attachment: 363398

Committed r242306: <https://trac.webkit.org/changeset/242306>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48531587>

[Darin Adler]

Comment on attachment 362940 [details]
Inspector Server

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

>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketClientPlayStation.cpp:48
>> +    saServer.sin_family = AF_INET;
> 
> The names in here that are abbreviations are unusual for WebKit style (`saServer`, `ret`).
> 
> Normally I see initialization with memset:
> 
>     memset(&saServer, 0, sizeof(struct sockaddr_in));
> 
> I'm not sure if the { 0 } alone would be enough.

I am almost sure that { 0 } style will work, and it is pretty common in Windows code, but not a common idiom outside Windows as far as I know. I believe that in Modern C++ it can just be { }.

[Christopher Reid]

Comment on attachment 362940 [details]
Inspector Server

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

>>> Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketClientPlayStation.cpp:48
>>> +    saServer.sin_family = AF_INET;
>> 
>> The names in here that are abbreviations are unusual for WebKit style (`saServer`, `ret`).
>> 
>> Normally I see initialization with memset:
>> 
>>     memset(&saServer, 0, sizeof(struct sockaddr_in));
>> 
>> I'm not sure if the { 0 } alone would be enough.
> 
> I am almost sure that { 0 } style will work, and it is pretty common in Windows code, but not a common idiom outside Windows as far as I know. I believe that in Modern C++ it can just be { }.

The { 0 } style was raising -Wmissing-field-initializers in GCC and clang with -Wall and -Wextra but that warning does seem harmless in this case.
The { } style seems to work fine without having the warning, it looks like clang even compiles out `struct sockaddr_in address = { }` to the equivalent memset call. I can change it to { } in a followup patch.