[webkit-reviews] review denied: [Bug 193806] [PlayStaton] Upstream playstation's remote inspector server : [Attachment 361707] Inspector Server

Mon Feb 11 15:21:52 PST 2019

Joseph Pecoraro <joepeck at webkit.org> has denied Christopher Reid
<chris.reid at sony.com>'s request for review:
Bug 193806: [PlayStaton] Upstream playstation's remote inspector server
https://bugs.webkit.org/show_bug.cgi?id=193806

Attachment 361707: Inspector Server

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

--- Comment #20 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 361707
  --> https://bugs.webkit.org/attachment.cgi?id=361707
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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.cpp:213
> +	       LOG_ERROR("read error (errno = %d)", errno);

Should you also close in the negative read case?

>
Source/JavaScriptCore/inspector/remote/playstation/RemoteInspectorSocketPlaySta
tion.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/RemoteInspectorSocketPlaySta
tion.cpp:262
> +		   return writeSize;

This looks like it could be `return true`.

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

And this `return false`.

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

Style: Remove accidental empty line.

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

Style: Remove accidental empty line.