Bug 193806 - [PlayStation] Upstream playstation's remote inspector server
Summary: [PlayStation] Upstream playstation's remote inspector server
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 197030
  Show dependency treegraph
 
Reported: 2019-01-24 19:03 PST by Christopher Reid
Modified: 2019-04-17 13:59 PDT (History)
11 users (show)

See Also:


Attachments
patch (54.19 KB, patch)
2019-01-24 19:40 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
[WIP] Inspector Server (53.12 KB, patch)
2019-02-01 18:01 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Proof of Concept (19.80 KB, patch)
2019-02-01 18:03 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Inspector Server (50.36 KB, patch)
2019-02-11 00:09 PST, Christopher Reid
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.45 MB, application/zip)
2019-02-11 02:10 PST, EWS Watchlist
no flags Details
Inspector Server (50.45 KB, patch)
2019-02-11 11:48 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Inspector Server (50.50 KB, patch)
2019-02-11 13:34 PST, Christopher Reid
joepeck: review-
Details | Formatted Diff | Diff
Inspector Server (66.71 KB, patch)
2019-02-19 09:49 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Inspector Server (67.17 KB, patch)
2019-02-25 16:11 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (76.51 KB, patch)
2019-03-01 18:44 PST, Christopher Reid
no flags Details | Formatted Diff | Diff
Patch for landing (76.51 KB, patch)
2019-03-01 18:48 PST, Christopher Reid
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Reid 2019-01-24 19:03:26 PST
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.
Comment 1 Christopher Reid 2019-01-24 19:40:37 PST
Created attachment 360074 [details]
patch
Comment 2 EWS Watchlist 2019-01-24 19:41:56 PST
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.
Comment 3 Don Olmstead 2019-01-25 09:41:17 PST
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
Comment 4 Stephan Szabo 2019-01-25 14:29:31 PST
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.
Comment 5 Christopher Reid 2019-02-01 18:01:40 PST
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.
Comment 6 Christopher Reid 2019-02-01 18:03:40 PST
Created attachment 360940 [details]
Proof of Concept

Rough patch used for testing on GTK
Comment 7 EWS Watchlist 2019-02-07 16:42:38 PST
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.
Comment 8 Joseph Pecoraro 2019-02-07 17:07:39 PST
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.
Comment 9 Joseph Pecoraro 2019-02-07 17:08:33 PST
> 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 =)
Comment 10 Christopher Reid 2019-02-07 18:51:35 PST
(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.
Comment 11 Christopher Reid 2019-02-11 00:09:06 PST
Created attachment 361665 [details]
Inspector Server

Fixing up review items and storing connections as a HashMap with random IDs as keys.
Comment 12 EWS Watchlist 2019-02-11 00:13:23 PST
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.
Comment 13 EWS Watchlist 2019-02-11 02:10:52 PST
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
Comment 14 EWS Watchlist 2019-02-11 02:10:54 PST
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
Comment 15 Christopher Reid 2019-02-11 11:48:26 PST
Created attachment 361701 [details]
Inspector Server

Fixing some smaller items I noticed since the last patch.
Comment 16 EWS Watchlist 2019-02-11 11:50:24 PST
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.
Comment 17 Christopher Reid 2019-02-11 11:55:06 PST
(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.
Comment 18 Christopher Reid 2019-02-11 13:34:09 PST
Created attachment 361707 [details]
Inspector Server

Fixing a build error
Comment 19 EWS Watchlist 2019-02-11 13:35:52 PST
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.
Comment 20 Joseph Pecoraro 2019-02-11 15:21:52 PST
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.
Comment 21 Joseph Pecoraro 2019-02-11 15:22:56 PST
Oh, and of course there should be ChangeLogs in the next patch! See `prepare-ChangeLog`  or webkit-patch:
https://webkit.org/contributing-code/
Comment 22 Carlos Garcia Campos 2019-02-12 02:31:45 PST
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
Comment 23 Devin Rousso 2019-02-12 14:55:07 PST
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.
Comment 24 Joseph Pecoraro 2019-02-12 16:01:15 PST
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.
Comment 25 Christopher Reid 2019-02-12 19:00:42 PST
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.
Comment 26 Christopher Reid 2019-02-19 09:49:05 PST
Created attachment 362388 [details]
Inspector Server

Updated patch
Comment 27 EWS Watchlist 2019-02-19 09:51:59 PST
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.
Comment 28 Joseph Pecoraro 2019-02-19 11:41:00 PST
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.
Comment 29 Christopher Reid 2019-02-25 16:11:51 PST
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.
Comment 30 EWS Watchlist 2019-02-25 16:14:06 PST
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.
Comment 31 Christopher Reid 2019-02-25 16:14:21 PST
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.
Comment 32 Joseph Pecoraro 2019-03-01 13:31:16 PST
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.
Comment 33 Christopher Reid 2019-03-01 18:44:45 PST
Created attachment 363397 [details]
Patch for landing
Comment 34 EWS Watchlist 2019-03-01 18:47:28 PST
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.
Comment 35 Christopher Reid 2019-03-01 18:48:40 PST
Created attachment 363398 [details]
Patch for landing
Comment 36 EWS Watchlist 2019-03-01 18:58:28 PST
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.
Comment 37 WebKit Commit Bot 2019-03-01 19:28:02 PST
Comment on attachment 363398 [details]
Patch for landing

Clearing flags on attachment: 363398

Committed r242306: <https://trac.webkit.org/changeset/242306>
Comment 38 WebKit Commit Bot 2019-03-01 19:28:04 PST
All reviewed patches have been landed.  Closing bug.
Comment 39 Radar WebKit Bug Importer 2019-03-01 19:30:14 PST
<rdar://problem/48531587>
Comment 40 Darin Adler 2019-03-01 22:15:58 PST
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 { }.
Comment 41 Christopher Reid 2019-03-04 15:31:40 PST
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.