Add support for HandleBlockingMessage to NaClMessageScanner

Add support for HandleBlockingMessage to NaClMessageScanner

BUG=555769
R=bbudge

Committed: https://crrev.com/822ba8f1a22d6756f90d5ea004af28ce224433d2
Cr-Commit-Position: refs/heads/master@{#368617}

[bbudge, 2015-11-13 23:59:47]

LGTM Thanks for doing this. I'm amazed you remember the code so well.

[dmichael (off chromium), 2015-11-14 00:01:05]

The CQ bit was checked by dmichael@chromium.org

[commit-bot: I haz the power, 2015-11-14 00:03:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440423003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440423003/40001

[commit-bot: I haz the power, 2015-11-14 01:17:02]

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

[commit-bot: I haz the power, 2015-11-14 01:17:03]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dmichael (off chromium), 2015-11-16 18:20:15]

Noooooo...  it's failing only on Windows release. I don't even have a Windows
machine anymore. What do you think of turning off the Windows tests and checking
in since it fixes Mac & Linux? Leaving you able to revisit Windows when it makes
sense for you. I think this is still an improvement over the current state of
affairs. I wish I knew what was up on Windows, but I'm not sure I have the tools
or time to figure that out.

[Mark Seaborn, 2015-12-17 01:46:54]

mseaborn@chromium.org changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2015-12-17 01:46:54]

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:195: ScanTuple(params, results);
This needs to be:

  bool ScanSyncMessage(ScanningResults* results) {
    typename base::TupleTypes<typename MessageType::Schema::SendParam>
        ::ValueTuple params;
    if (!MessageType::ReadSendParam(msg_, &params))
      return false;
    // If we need to rewrite the message, write the message id first.
    if (results->new_msg) {
      results->new_msg->set_sync();
      int id = IPC::SyncMessage::GetMessageId(*msg_);
      results->new_msg->WriteInt(id);
    }
    ScanTuple(params, results);
    return true;
  }

I figured this out for my change, https://codereview.chromium.org/1512733003/.

I was able to test that on Linux by setting "rewrite_msg = true" below.

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:234: case MESSAGE_TYPE::ID: { \
Nit: can you make the indentation alignment match the macros above and below?

[Mark Seaborn, 2015-12-17 01:49:04]

One more thing, in case you land this before my change
(https://codereview.chromium.org/1512733003/)...

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:213: // TODO(dmichael): Add ScanSyncMessage
for outgoing sync messages, if we ever
This TODO should be removed too.

[dmichael (off chromium), 2015-12-17 16:03:08]

On 2015/12/17 01:49:04, Mark Seaborn wrote:
> One more thing, in case you land this before my change
> (https://codereview.chromium.org/1512733003/)...
> 
>
https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
> File ppapi/proxy/nacl_message_scanner.cc (right):
> 
>
https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
> ppapi/proxy/nacl_message_scanner.cc:213: // TODO(dmichael): Add
ScanSyncMessage
> for outgoing sync messages, if we ever
> This TODO should be removed too.

If you're alright with it, I'd rather just abandon this and let you land yours.
Thanks for figuring the problem out!

[bbudge, 2015-12-17 18:34:09]

On 2015/12/17 16:03:08, dmichael (off chromium) wrote:
> On 2015/12/17 01:49:04, Mark Seaborn wrote:
> > One more thing, in case you land this before my change
> > (https://codereview.chromium.org/1512733003/)...
> > 
> >
>
https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
> > File ppapi/proxy/nacl_message_scanner.cc (right):
> > 
> >
>
https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
> > ppapi/proxy/nacl_message_scanner.cc:213: // TODO(dmichael): Add
> ScanSyncMessage
> > for outgoing sync messages, if we ever
> > This TODO should be removed too.
> 
> If you're alright with it, I'd rather just abandon this and let you land
yours.
> Thanks for figuring the problem out!

But land the tests?

[dmichael (off chromium), 2015-12-17 18:41:57]

On 2015/12/17 18:34:09, bbudge wrote:
> On 2015/12/17 16:03:08, dmichael (off chromium) wrote:
> > On 2015/12/17 01:49:04, Mark Seaborn wrote:
> > > One more thing, in case you land this before my change
> > > (https://codereview.chromium.org/1512733003/)...
> > > 
> > >
> >
>
https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
> > > File ppapi/proxy/nacl_message_scanner.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
> > > ppapi/proxy/nacl_message_scanner.cc:213: // TODO(dmichael): Add
> > ScanSyncMessage
> > > for outgoing sync messages, if we ever
> > > This TODO should be removed too.
> > 
> > If you're alright with it, I'd rather just abandon this and let you land
> yours.
> > Thanks for figuring the problem out!
> 
> But land the tests?

Good point, that should land, and Mark's change doesn't address adding
HandleBlockingMessage. I was honestly just trying to avoid that context switch
;-)  But I'll do it.

[dmichael (off chromium), 2015-12-17 19:04:29]

Comments addressed, PTAL.

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:195: ScanTuple(params, results);
On 2015/12/17 01:46:54, Mark Seaborn wrote:
> This needs to be:
> 
>   bool ScanSyncMessage(ScanningResults* results) {
>     typename base::TupleTypes<typename MessageType::Schema::SendParam>
>         ::ValueTuple params;
>     if (!MessageType::ReadSendParam(msg_, &params))
>       return false;
>     // If we need to rewrite the message, write the message id first.
>     if (results->new_msg) {
>       results->new_msg->set_sync();
>       int id = IPC::SyncMessage::GetMessageId(*msg_);
>       results->new_msg->WriteInt(id);
>     }
>     ScanTuple(params, results);
>     return true;
>   }
> 
> I figured this out for my change, https://codereview.chromium.org/1512733003/.
> 
> I was able to test that on Linux by setting "rewrite_msg = true" below.

Done.

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:213: // TODO(dmichael): Add ScanSyncMessage
for outgoing sync messages, if we ever
On 2015/12/17 01:49:04, Mark Seaborn wrote:
> This TODO should be removed too.

Done.

https://codereview.chromium.org/1440423003/diff/40001/ppapi/proxy/nacl_messag...
ppapi/proxy/nacl_message_scanner.cc:234: case MESSAGE_TYPE::ID: { \
On 2015/12/17 01:46:54, Mark Seaborn wrote:
> Nit: can you make the indentation alignment match the macros above and below?
Done (actually changed the others; not sure why I had them weirdly indented. I
think this is now proper style.)

[dmichael (off chromium), 2016-01-11 17:48:29]

The CQ bit was checked by dmichael@chromium.org

[dmichael (off chromium), 2016-01-11 17:48:29]

The patchset sent to the CQ was uploaded after l-g-t-m from bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1440423003/#ps100001
(title: "merge with mseaborn's changes")

[commit-bot: I haz the power, 2016-01-11 17:48:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1440423003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1440423003/100001

[commit-bot: I haz the power, 2016-01-11 17:55:00]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-01-11 17:55:48]

Description was changed from

==========
Add support for HandleBlockingMessage to NaClMessageScanner

BUG=555769
R=bbudge
==========

to

==========
Add support for HandleBlockingMessage to NaClMessageScanner

BUG=555769
R=bbudge

Committed: https://crrev.com/822ba8f1a22d6756f90d5ea004af28ce224433d2
Cr-Commit-Position: refs/heads/master@{#368617}
==========

[commit-bot: I haz the power, 2016-01-11 17:55:49]

Patchset 6 (id:??) landed as
https://crrev.com/822ba8f1a22d6756f90d5ea004af28ce224433d2
Cr-Commit-Position: refs/heads/master@{#368617}

[bbudge, 2016-01-11 18:18:17]

Thanks David!