[Webkit-unassigned] [Bug 193806] [PlayStaton] Upstream playstation's remote inspector server
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Feb 11 15:21:52 PST 2019
https://bugs.webkit.org/show_bug.cgi?id=193806
Joseph Pecoraro <joepeck at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #361707|review? |review-
Flags| |
--- 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/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.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20190211/9f2d563c/attachment-0001.html>
More information about the webkit-unassigned
mailing list