959114 - [LayerScope] Easy connection between device and viewer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[u459114]

For now, two steps needed to create a connection between device and desktop browser
1. adb shell netcfg << to figure out device ip
2. python ./websockify/websockify.py ....

Create a shell script to combine this two commends into one.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The right thing here is to get rid of the need for websockify and make the gecko-side of LayerScope use the proper Web Socket framing.  This isn't hard, I just didn't want to deal with it when hacking this together, but it would greatly simplify things -- all that you would need then is the IP address.

Comment 2

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency

This patch let LayerScope output WebSocket packet format directly. After apply this patch, we don't need websockify anymore.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8365840 [details] [diff] [review]
Remove Websockify Dependency

Review of attachment 8365840 [details] [diff] [review]:
-----------------------------------------------------------------

Morris, this looks great!  Thanks for doing this.  Really only two minor problems for the r- though, the rest are just comments/nits/typos:

1 - no need to malloc() a web socket header each frame

2 - need to do a handshake on every connection, not just the first

::: gfx/layers/LayerScope.cpp
@@ +58,5 @@
>  
>  class DebugDataSender;
>  
>  static bool gDebugConnected = false;
> +static bool gIsWebSocketHandshaked = false;

Super minor nit -- this is a little awkward naming, maybe call this "gIsWebSocket" (see later comments)

@@ +120,5 @@
>      static bool WriteToStream(void *ptr, uint32_t size) {
>          uint32_t written = 0;
>          nsresult rv;
> +
> +        // Send Websocket header

So this is fine for now, but this means that we're going to send a full websocket frame for every WriteToStream -- which is every packet.  It would be much more efficient to buffer this stuff somehow and only send packets when we hit some number of bytes (2K, 4K, etc).

That might be good for a followup patch -- make WriteToStream buffer up to something like 1400 bytes, sending when the buffer is full (or when it has more than that to send), and then flush at the end of the frame.  Though maybe it doesn't matter, since many of the packets will be large with image data.  We can always capture overhead percentage and as long as it's not too high we should be ok.

@@ +131,5 @@
> +        } else {
> +            ws_header_size = 10;
> +        }
> +
> +        ws_header = std::auto_ptr<uint8_t>((uint8_t*) moz_malloc(ws_header_size));

No reason to allocate this each packet -- the maximum header size is 10 bytes, so just declare

   uint8_t header[10];

later and use it directly.

(r- is because of this, but the fix is simple)

@@ +354,4 @@
>  
> +    bool isWebsocket = false;
> +    nsCString version;
> +    nsCString websocket_key;

Hmm, either use camelCase or use_underlines, but don't mix :)  Maybe "wsKey" or "webSocketKey".

@@ +354,5 @@
>  
> +    bool isWebsocket = false;
> +    nsCString version;
> +    nsCString websocket_key;
> +    nsCString protocal;

misspelling -- "protocol"

@@ +376,5 @@
> +                version = value;
> +            } else if (key == "Sec-WebSocket-Key") {
> +                websocket_key = value;
> +            } else if (key == "Sec-WebSocket-Protocol") {
> +                protocal = value;

HTTP header fields are case-insensitive.  This works because browsers tend to send the proper capitalization, but it really should be doing case-insensitive comparisons.  Just make everything lower case and compare to lowercase versions, so something like

  ToLowerCase(key);

and use "upgrade" etc. as the keys.  The value should also be lowercased where it matters, e.g. when comparing "websocket".

@@ +391,5 @@
> +            return false;
> +        }
> +    } else {
> +        return false;
> +    }

For easier reading, flip this if () block around so that you don't need to nest:

// we only handle WebSocket here
if (!isWebsocket) {
    return false;
}

if (!(version.EqualsLiteral("7") || version.EqualsLiteral("8") || version.EqualsLiteral("13"))) {
    return false;
}

if (!protocol.EqualsLiteral("binary")) {
    return false;
}

@@ +444,5 @@
>      {
>          printf_stderr("*** LayerScope: Accepted connection\n");
>          gDebugConnected = true;
>          gDebugSenderTransport = aTransport;
>          aTransport->OpenOutputStream(nsITransport::OPEN_BLOCKING, 0, 0, getter_AddRefs(gDebugStream));

For a followup patch -- either make this really support multiple connections, or make it reject connections if there's an existing one, or make it close the previous connection.  Supporting multiple connections would probably be best..

@@ +470,5 @@
> +        if (gIsWebSocketHandshaked) {
> +            // do somethings
> +        } else {
> +            gIsWebSocketHandshaked = WebSocketHandshake(stream);
> +        }

Not sure I understand this -- this is going to happen every time the input stream is opened.  We should do a new handshake each time, because each connection will be an entirely new web socket connection.

So this should just be gIsWebSocket = WebSocketHandshake(stream);

And then up above in WriteToStream, you should only send the web socket framing header if gIsWebSocket.  This way if someone wants to write another app to receive data from LayerScope, they don't have to understand web sockets if it's not a web app.

Comment 4

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency

This patch supports multiple connections and updated to address comment.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8366526 [details] [diff] [review]
Remove Websockify Dependency

Review of attachment 8366526 [details] [diff] [review]:
-----------------------------------------------------------------

Great -- for future work though, it would have been better to add support for multiple connections as a separate followup patch.  It makes it much easier to review when work is split up like that, especially for a second review of a patch (don't add new features in a followup review :).

This looks good though -- only suggestion is to move the sHandlers list and the WriteAll method to the LayerScope class itself, instead of on LayerScopeWebSocketHandler.  Likewise, I would add something to the handlers list as soon as it's connected, and maybe set a flag saying whether the handshake was done.

::: gfx/layers/LayerScope.cpp
@@ +137,5 @@
> +
> +    static void InitializeSocket(nsISocketTransport* aTransport) {
> +        printf_stderr("*** LayerScope: Accepted connection\n");
> +        nsRefPtr<LayerScopeWebSocketHandler> temp = new LayerScopeWebSocketHandler();
> +        temp->OpenStream(aTransport);

I'm missing something -- who owns the ref in this case that will keep the handler alive?  I don't think there's any guarantee that AsyncWait will hold a ref (though I think it does).  Not really comfortable with the static array that things get magically added/removed from.

@@ +146,5 @@
> +            nsRefPtr<LayerScopeWebSocketHandler> temp = sHandlers[i];
> +            if (!temp->WriteToStream(ptr, size)) {
> +                // Send failed, remove this handler
> +                printf_stderr("remove handler");
> +                sHandlers.RemoveElementAt(i);

We should really somehow handle this better I think, maybe through a static AddConnection and RemoveConnection method.  Any reason why the list of handlers etc. isn't static on LayerScope?  That seems much cleaner and easier to follow than keeping the list of a thing inside the thing itself, if that makes sense.  You may also need to implement some error callbacks to clear connections out... I guess we're likely to catch everything on WriteToStream, so that's ok.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency

update to address comment.

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8367091 [details] [diff] [review]
Remove Websockify Dependency

Review of attachment 8367091 [details] [diff] [review]:
-----------------------------------------------------------------

(Bugzilla comment -- it's helpful to put "v1" "v2" "v3" in the patch description, because there are places where they all appear in a dropdown... e.g. when using Diff and looking at an interdiff between a previous version.)

Looks good, just one issue to fix!  Can you upload a new patch with the corrections, and I'll r+?

::: gfx/layers/LayerScope.cpp
@@ +67,5 @@
> +public:
> +    NS_DECL_THREADSAFE_ISUPPORTS
> +
> +    enum SocketStateType {
> +        NotHandshake,

"NoHandshake"

@@ +605,5 @@
> +{
> +    sHandlers.RemoveElementAt(aIndex);
> +
> +    // Return previous index to let loop continues
> +    return aIndex - 1;

return isn't needed (see next comment)

@@ +614,5 @@
> +    for (uint32_t i = 0; i < sHandlers.Length(); ++i) {
> +        nsRefPtr<LayerScopeWebSocketHandler> temp = sHandlers[i];
> +        if (!temp->WriteToStream(ptr, size)) {
> +            // Send failed, remove this handler
> +            RemoveConnection(i);

Hmm, this is incorrect.  As written this will do the wrong thing (will skip over a connection, I believe).  Usually the way to handle this is to iterate over the list backwards:

for (int32_t i = sHandlers.Length() - 1; i >= 0; --i) {
   ...
}

That way, even if you remove element[i], the loop iteration continues properly.  (Note the usage of int32_t to handle the sHandlers.Length() == 0 case.)

Comment 8

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency v4

Update with above corrections.

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8367124 [details] [diff] [review]
Remove Websockify Dependency v4

Looks great to me!  Thanks for following up so quickly on this.

Comment 10

[u459114]

Morris, please make sure update wiki page once this patch landed
https://wiki.mozilla.org/Platform/GFX/LayerScope

Comment 11

[u459114]

Comment on attachment 8367124 [details] [diff] [review]
Remove Websockify Dependency v4

Review of attachment 8367124 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/LayerScope.cpp
@@ +64,2 @@
>  static nsCOMPtr<DebugDataSender> gCurrentSender;
>  

Construct a WebSocketManger object(singleton) here to hold
1. nsTArray<nsRefPtr<LayerScopeWebSocketHandler> > sHandlers
2. gDebugServerSocket
3. gDebugSenderThread
4. gDebugSenderTransport
5. gCurrentSender

@@ +64,3 @@
>  static nsCOMPtr<DebugDataSender> gCurrentSender;
>  
> +class LayerScopeWebSocketHandler : public nsIInputStreamCallback {

Add comment here to reveal the purpose of this class.
1. WebSocket handshake 
2. Append web socket header for each LayerScope package.

@@ +64,5 @@
>  static nsCOMPtr<DebugDataSender> gCurrentSender;
>  
> +class LayerScopeWebSocketHandler : public nsIInputStreamCallback {
> +public:
> +    NS_DECL_THREADSAFE_ISUPPORTS

What's the relation between DebugSenderTransport and LayerScopeSocketHandler?
1-to-1 or 1-t-n?

If the relation between these two is 1-to-1, we should remove gDebugSenderTransport and define a DebugSenderTransport member data in LayerScopeSocketHandler

@@ +105,5 @@
> +
> +        uint32_t written = 0;
> +        nsresult rv;
> +
> +        // Send Websocket header

// Generate WebSocket header

@@ +108,5 @@
> +
> +        // Send Websocket header
> +        uint8_t wsHeader[10];
> +        int wsHeaderSize = 0;
> +        uint8_t opcode = 0x2;

const uint8_t opcode = 0x2;

@@ +122,5 @@
> +            wsHeaderSize = 10;
> +            wsHeader[1] = 0x7F;
> +            NetworkEndian::writeUint64(wsHeader + 2, size);
> +        }
> +

// Send WebSocket header

@@ +128,5 @@
> +        rv = mOutputStream->Write(reinterpret_cast<char*>(wsHeader),
> +                                 wsHeaderSize, &cnt);
> +        if (NS_FAILED(rv))
> +            return false;
> +

Move #106 to #132

@@ +196,5 @@
> +                    protocol = value;
> +                }
> +            }
> +        }
> +

Separate "read input stream data" and "parse input stream data" into two functions.

::: gfx/layers/LayerScope.h
@@ +31,5 @@
> +    static void AddConnection(LayerScopeWebSocketHandler* aHandler);
> +    static void RemoveConnection(uint32_t aIndex);
> +    static void RemoveAllConnections();
> +    static bool WriteAll(void *ptr, uint32_t size);
> +    static bool IsConnected();

These 5 new function are used internally. suggest moving them to cpp

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency v5

Update patch to address comment

Comment 13

[u459114]

Comment on attachment 8371303 [details] [diff] [review]
Remove Websockify Dependency v5

Review of attachment 8371303 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/LayerScope.cpp
@@ +77,5 @@
> +    { }
> +
> +    virtual ~LayerScopeWebSocketHandler()
> +    {
> +        mTransport->Close(NS_OK);

mTransport might be null

@@ +81,5 @@
> +        mTransport->Close(NS_OK);
> +    }
> +
> +    void OpenStream(nsISocketTransport* aTransport) {
> +        mTransport = aTransport;

MOZ_ASSERT(aTransport); or runtime validate if you can not make sure this value is not null

@@ +158,5 @@
> +            mState = HandshakeFailed;
> +        }
> +        return NS_OK;
> +    }
> +protected:

Make it private

@@ +207,5 @@
> +                    }
> +                }
> +            }
> +        }
> +

// Validate input parameter
if (aProtocolString.Length() == 0)
 return false; <-- Add an ASSERT here if you think this condition should not be hit.

// Validate web socket handshake message.
// Check that the HTTP method is GET
const char* line = aProtocolString[0].get();
const char* HTTP_METHOD = "GET ";
if (strncmp(line, HTTP_METHOD, strlen(HTTP_METHOD)) != 0) 
  return false; 

// move out if (i==0) statement to reduce indent.
for (uint32_t i = 1; i < aProtocolString.Length(); ++i) {
  const char* prop_pos = strchr(line, ':');
  if (prop_pos != nullptr) {
    nsCString key(line, prop_pos - line);
    nsCString value(prop_pos + 2);
    if (key.EqualsIgnoreCase("upgrade")) {
      if (!value.EqualsIgnoreCase("websocket")))
       return false;
    } else if (key.EqualsIgnoreCase("sec-websocket-version")) {
      version = value;
    } else if (key.EqualsIgnoreCase("sec-websocket-key")) { 
      wsKey = value;
    } else if (key.EqualsIgnoreCase("sec-websocket-protocol")) {
      protocol = value;
    }
}

@@ +263,5 @@
> +
> +class WebSocketManager {
> +public:
> +    WebSocketManager();
> +    virtual ~WebSocketManager();

No need to be virtual

@@ +266,5 @@
> +    WebSocketManager();
> +    virtual ~WebSocketManager();
> +
> +    void AddConnection(LayerScopeWebSocketHandler* aHandler)
> +    {

MOZ_ASSERT(aHandler)

@@ +271,5 @@
> +        mHandlers.AppendElement(aHandler);
> +    }
> +
> +    void RemoveConnection(uint32_t aIndex)
> +    {

MOZ_ASSERT(nIndex < mHandlers.Length());

@@ +295,5 @@
> +    }
> +
> +    bool IsConnected()
> +    {
> +        return mHandlers.Length();

return (mHandlers.Length() == 0) ? false : true;

@@ +298,5 @@
> +    {
> +        return mHandlers.Length();
> +    }
> +
> +    void AppendDebugData(DebugGLData *aDebugData);

Why not just define function body here? just like all the other member functions

@@ +815,5 @@
> +}
> +
> +WebSocketManager::~WebSocketManager()
> +{
> +    mServerSocket = nullptr;

Is this line needed?

@@ +823,5 @@
> +{
> +    if (!mCurrentSender) {
> +        mCurrentSender = new DebugDataSender();
> +    }
> +

The way you create/ destroy mCurrentSender is not thread safe.
Make sure AppendDebugData/ DispatchDebugData will be called in the same thread

Comment 14

[u459114]

Comment on attachment 8371303 [details] [diff] [review]
Remove Websockify Dependency v5

Review of attachment 8371303 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/LayerScope.cpp
@@ +286,5 @@
> +        for (int32_t i = mHandlers.Length() - 1; i >= 0; --i) {
> +            nsRefPtr<LayerScopeWebSocketHandler> temp = mHandlers[i];
> +            if (!temp->WriteToStream(ptr, size)) {
> +                // Send failed, remove this handler
> +                RemoveConnection(i);

Reverse iteration to prevent index be invalidate by remove connection is a good idea
nitty:
// you don't really need temp variable here.
if (!mHandlers[i]->WriteToStream(ptr, size)) {
  // Send failed, remove this handler
  RemoveConnection(i);
}

Comment 15

[u459114]

Comment on attachment 8371303 [details] [diff] [review]
Remove Websockify Dependency v5

Review of attachment 8371303 [details] [diff] [review]:
-----------------------------------------------------------------

Add some inline comment

::: gfx/layers/LayerScope.cpp
@@ +180,5 @@
> +        bool isWebSocket = false;
> +        nsCString version;
> +        nsCString wsKey;
> +        nsCString protocol;
> +

// Validate WebSocket client request.

@@ +220,5 @@
> +        if (!(protocol.EqualsIgnoreCase("binary"))) {
> +            return false;
> +        }
> +
> +        // If reaches here means valid WebSocket connection

// Client request is valid. Start to generate and send server response.

Comment 16

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8371303 [details] [diff] [review]
Remove Websockify Dependency v5

Lots of changes! Looks good still, but please rename WebSocketManager to something like "LayerscopeSocketManager".  Avoid generic names whenever possible :)

Comment 17

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to C.J. Ku[:CJKu] from comment #13)
> @@ +298,5 @@
> > +    {
> > +        return mHandlers.Length();
> > +    }
> > +
> > +    void AppendDebugData(DebugGLData *aDebugData);
> 
> Why not just define function body here? just like all the other member
> functions
> 
Because function AppendDebugData uses class DebugDataSender and DebugGLData and vice versa. So if I define function body like all other member functions then compilation won't pass. Thus I have to move those body to the bottom of source.

Comment 18

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency v6 (carry: vladimir:r+)

update to address comment

Comment 19

[u459114]

Comment on attachment 8371996 [details] [diff] [review]
Remove Websockify Dependency v6 (carry: vladimir:r+)

Review of attachment 8371996 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks

::: gfx/layers/LayerScope.cpp
@@ +62,3 @@
>  
> +/* This class handle websocket protocol which included
> + * handshake and data frame's header */

Move "*/" to next line

@@ +150,5 @@
> +        return true;
> +    }
> +
> +    // nsIInputStreamCallback
> +    NS_IMETHODIMP OnInputStreamReady(nsIAsyncInputStream *stream)

MOZ_OVERRIDE

@@ +560,4 @@
>          printf_stderr("*** LayerScope: Accepted connection\n");
> +        nsRefPtr<LayerScopeWebSocketHandler> temp = new LayerScopeWebSocketHandler();
> +        gLayerScopeWebSocketManager->AddConnection(temp.get());
> +        temp->OpenStream(aTransport);

1. if OpenStream is must have, you may change signature of AddConnection to
void AddConnection(LayerScopeWebSocketHandler* aHandler,nsISocketTransport *aTransport);
And call OpenStream in LayerScopeWebSocketManager.

2. Or, change AddConnection to
void AddConnection(nsISocketTransport *aTransport) - new a handler and open stream.

Since you create a new class, LayerScopeWebSocketManager, hide detailed of using LayerScopeWebSocketHandler in that class

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: Remove Websockify Dependency v7 (carries: vladimir:r+, cku:r+)

update to address comment.
try push: https://tbpl.mozilla.org/?tree=Try&rev=37eecd30725a

Comment 21

[u459114]

https://wiki.mozilla.org/Platform/GFX/LayerScope

In this bug, we remove websockify dependency, this command is not needed anymore
  python ./websockify/websockify.py localhost:23457 localhost:23456
Please update wiki accordingly 

And file another bug for removing manually input URL in LayerScope by Firefox AddOn
  Replace ws://192.168.112.129:23457 with ws://localhost:23457, and press connect. 
  Replace localhost:23456 with address of B2G device. Eg. If device has address 192.168.1.22 
Ideally, user should be able to connect to device or browser with a button click on AddOn, just like GeckoProfiler.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/5f8d42207dfd

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f8d42207dfd