[Webkit-unassigned] [Bug 193806] [PlayStaton] Upstream playstation's remote inspector server

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=193806

--- Comment #4 from Stephan Szabo <stephan.szabo at sony.com> ---
Comment on attachment 360074
  --> https://bugs.webkit.org/attachment.cgi?id=360074
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.

-- 
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/20190125/0e46e7aa/attachment-0001.html>