Add support for logging information about mojo messages retrieved by the mojo debugger

Add support for logging information about mojo messages retrieved by the mojo debugger
from the message pipes.

This includes the following:-
1. Number of bytes in the message.
2. Number of fields in the message.
3. The message id.
4. The request id of the message.
5. Whether the message expects a response message.
6. Whether the message is a response message.
7. The URL of the service.

We also intercept the first message pipe which is used for
vending services in the debugger.

Support for tracing additional information will be added as needed.

BUG=360188
TBR=viettrungluu

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282417

[cpu_(ooo_6.6-7.5), 2014-06-27 01:35:31]

adding vtl

[cpu_(ooo_6.6-7.5), 2014-06-27 01:44:06]

https://codereview.chromium.org/354043003/diff/40001/mojo/spy/websocket_serve...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/40001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:44: 
assume you have:

bool WebSocketServer::connected() const {
 return connection_id_ !=  kNotConnected;
}

here you can do 
if (connected()) {
  str = FormatMojoMessage(....)
  server_->SendOverWebSocket(connection_id, str);
}

See line 91.

[viettrungluu, 2014-06-30 19:16:55]

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h
File mojo/spy/common.h (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h#newcode8
mojo/spy/common.h:8: #include "base/basictypes.h"
You should probably only include <stdint.h> instead.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h#newcode13
mojo/spy/common.h:13: 
nit: extra blank line

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h#newcode42
mojo/spy/common.h:42: 
nit: extra blank line

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/spy.cc
File mojo/spy/spy.cc (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/spy.cc#newcode131
mojo/spy/spy.cc:131: reinterpret_cast<mojo::MojoMessageData*>(data);
static_cast

More importantly, before you do so, you should validate that the header is
actually correct. (Otherwise, while debugging, if the code you're debugging runs
code from an untrusted app, you might get 0wned.)

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:24: #define MOJO_DEBUGGER_MESSAGE_FORMAT "\"Start
of message for url %ls\n" \
Do you really need this macro? It's only used in one place.

Also, using "%d" for uint32_t fields, etc. seems dubious to me (use "%u").

Also: "%ls"? Should just be "%s", I think.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:25: "Message Time Hours : %d\n"\
No space before ':', please.

Also, probably you can format the time as:

Message Time: %02d:%02d:%02d\n

rather than taking three lines.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:30: "Message name : %d\n"\
Nit: name -> Name (capitalized; be consistent -- you can also uncapitalize the
other lines)

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:31: "Message Request ID : %d\n"\
%llu (it's 64-bit)

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:61: url.spec(),
url.spec().c_str()?

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:64: message_header.num_bytes,
Strictly speaking, for the uint32_t fields, you should probably use "%u" and
then static_cast<unsigned>(...) the variables.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:67: message_header.num_fields == 3 ?
message_header.request_id : -1,
Hmmm. You should probably use "%llu" and static_cast<unsigned long long>(...).
But that leaves the question of what you should do if the field isn't
present....

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:72: DLOG(INFO) << "\n\nStart of message for url :"
<< url.spec();
Maybe you should just generate the same string (as above) and DLOG that.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_server.h
File mojo/spy/websocket_server.h (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.h:13: };
No ';'.

[ananta, 2014-07-03 00:04:40]

https://codereview.chromium.org/354043003/diff/40001/mojo/spy/websocket_serve...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/40001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:44: 
On 2014/06/27 01:44:06, cpu wrote:
> assume you have:
> 
> bool WebSocketServer::connected() const {
>  return connection_id_ !=  kNotConnected;
> }
> 
> here you can do 
> if (connected()) {
>   str = FormatMojoMessage(....)
>   server_->SendOverWebSocket(connection_id, str);
> }
> 
> See line 91.
> 

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h
File mojo/spy/common.h (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h#newcode8
mojo/spy/common.h:8: #include "base/basictypes.h"
On 2014/06/30 19:16:54, viettrungluu wrote:
> You should probably only include <stdint.h> instead.

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h#newcode13
mojo/spy/common.h:13: 
On 2014/06/30 19:16:54, viettrungluu wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/common.h#newcode42
mojo/spy/common.h:42: 
On 2014/06/30 19:16:54, viettrungluu wrote:
> nit: extra blank line

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:24: #define MOJO_DEBUGGER_MESSAGE_FORMAT "\"Start
of message for url %ls\n" \
On 2014/06/30 19:16:54, viettrungluu wrote:
> Do you really need this macro? It's only used in one place.
> 
> Also, using "%d" for uint32_t fields, etc. seems dubious to me (use "%u").
> 
> Also: "%ls"? Should just be "%s", I think.

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:24: #define MOJO_DEBUGGER_MESSAGE_FORMAT "\"Start
of message for url %ls\n" \
On 2014/06/30 19:16:54, viettrungluu wrote:
> Do you really need this macro? It's only used in one place.
> 
> Also, using "%d" for uint32_t fields, etc. seems dubious to me (use "%u").
> 
> Also: "%ls"? Should just be "%s", I think.

Leaving the macro as is as it makes it easier to align the format over multiple
lines.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:25: "Message Time Hours : %d\n"\
On 2014/06/30 19:16:55, viettrungluu wrote:
> No space before ':', please.
> 
> Also, probably you can format the time as:
> 
> Message Time: %02d:%02d:%02d\n
> 
> rather than taking three lines.

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:30: "Message name : %d\n"\
On 2014/06/30 19:16:55, viettrungluu wrote:
> Nit: name -> Name (capitalized; be consistent -- you can also uncapitalize the
> other lines)

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:31: "Message Request ID : %d\n"\
On 2014/06/30 19:16:55, viettrungluu wrote:
> %llu (it's 64-bit)

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:61: url.spec(),
On 2014/06/30 19:16:55, viettrungluu wrote:
> url.spec().c_str()?

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:64: message_header.num_bytes,
On 2014/06/30 19:16:55, viettrungluu wrote:
> Strictly speaking, for the uint32_t fields, you should probably use "%u" and
> then static_cast<unsigned>(...) the variables.

Done.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:67: message_header.num_fields == 3 ?
message_header.request_id : -1,
On 2014/06/30 19:16:54, viettrungluu wrote:
> Hmmm. You should probably use "%llu" and static_cast<unsigned long long>(...).
> But that leaves the question of what you should do if the field isn't
> present....

Done. If the field is not present we print beef.

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:72: DLOG(INFO) << "\n\nStart of message for url :"
<< url.spec();
On 2014/06/30 19:16:55, viettrungluu wrote:
> Maybe you should just generate the same string (as above) and DLOG that.

Done.

[ananta, 2014-07-03 00:57:24]

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/spy.cc
File mojo/spy/spy.cc (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/spy.cc#newcode131
mojo/spy/spy.cc:131: reinterpret_cast<mojo::MojoMessageData*>(data);
On 2014/06/30 19:16:54, viettrungluu wrote:
> static_cast
> 
> More importantly, before you do so, you should validate that the header is
> actually correct. (Otherwise, while debugging, if the code you're debugging
runs
> code from an untrusted app, you might get 0wned.)

Added a function to validate the message as per the spec.

[ananta, 2014-07-09 02:33:56]

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/60001/mojo/spy/websocket_serve...
mojo/spy/websocket_server.cc:67: message_header.num_fields == 3 ?
message_header.request_id : -1,
On 2014/07/03 00:04:40, ananta wrote:
> On 2014/06/30 19:16:54, viettrungluu wrote:
> > Hmmm. You should probably use "%llu" and static_cast<unsigned long
long>(...).
> > But that leaves the question of what you should do if the field isn't
> > present....
> 
> Done. If the field is not present we print beef.

If the field is not present we print 0 followed by a string which says not a
request or response message.

[cpu_(ooo_6.6-7.5), 2014-07-09 18:18:45]

https://codereview.chromium.org/354043003/diff/180001/mojo/spy/websocket_serv...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/180001/mojo/spy/websocket_serv...
mojo/spy/websocket_server.cc:30: "Message Type: %s\n"\
for this to come out properly formatted you need
not \n but \\n, see the question and answer here

http://stackoverflow.com/questions/17487058/how-to-solve-json-parse-bad-contr...

[ananta, 2014-07-09 21:24:55]

https://codereview.chromium.org/354043003/diff/180001/mojo/spy/websocket_serv...
File mojo/spy/websocket_server.cc (right):

https://codereview.chromium.org/354043003/diff/180001/mojo/spy/websocket_serv...
mojo/spy/websocket_server.cc:30: "Message Type: %s\n"\
On 2014/07/09 18:18:45, cpu wrote:
> for this to come out properly formatted you need
> not \n but \\n, see the question and answer here
> 
>
http://stackoverflow.com/questions/17487058/how-to-solve-json-parse-bad-contr...

Leaving \n as is. Fixed overall formatting.

[cpu_(ooo_6.6-7.5), 2014-07-10 02:01:20]

lgtm

[ananta, 2014-07-10 02:20:35]

TBR'ing vtl. Will address comments in a follow up.

[ananta, 2014-07-10 02:20:41]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-07-10 02:22:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/354043003/200001

[commit-bot: I haz the power, 2014-07-10 04:51:36]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-10 04:54:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-10 04:54:58]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[ananta, 2014-07-10 18:20:16]

The CQ bit was checked by ananta@chromium.org

[commit-bot: I haz the power, 2014-07-10 18:21:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ananta@chromium.org/354043003/220001

[commit-bot: I haz the power, 2014-07-10 20:27:44]

Change committed as 282417