Pepper: Make pending_sync_msgs_ local to IO thread.

Pepper: Make pending_sync_msgs_ local to IO thread.

Adding support for PpapiHostMessage_OpenResource to SFI mode is tricky.
Validating the file tokens requires that a message to be sent to the browser
(!) to validate that information in the case of a compromised renderer.

This change is one step to allowing processing for the reply of the
PpapiHostMessage_OpenResource message to send a message to the browser for
constructing a NaClDesc properly.

This makes the encapsulation of NaClMessageScanner a bit worse, but also
reduces the amount of code for which the critical section under lock_ must be
held.

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

[teravest, 2014-08-14 21:22:25]

This patch is currently only for discussion; see http://crbug.com/394130 for why
I'm exploring this approach.

I still need to fix NaClMessageScannerTest::OpenQuotaFile, it looks like.

[teravest, 2014-08-18 17:41:40]

-bbudge +dmichael

[dmichael (off chromium), 2014-08-18 20:06:09]

I haven't taken the time to understand how this will help you for validation
caching, but I think this change looks fine at this level.

https://codereview.chromium.org/472073003/diff/20001/components/nacl/loader/n...
File components/nacl/loader/nacl_ipc_adapter.cc (right):

https://codereview.chromium.org/472073003/diff/20001/components/nacl/loader/n...
components/nacl/loader/nacl_ipc_adapter.cc:666:
io_thread_data_.pending_sync_msgs_[id] = message->type();
It kind of looks like you are adding the id to the map even for messages that
are async? If that's the case, the map is growing unbounded with each async
message.

https://codereview.chromium.org/472073003/diff/20001/ppapi/proxy/nacl_message...
File ppapi/proxy/nacl_message_scanner.cc (right):

https://codereview.chromium.org/472073003/diff/20001/ppapi/proxy/nacl_message...
ppapi/proxy/nacl_message_scanner.cc:294: uint32_t type,
This parameter would be pretty confusing I think to somebody new to this class.
I think it needs a comment that for normal messages it matches msg.id(), but it
will be the id of the original message if this is a sync reply. Or something.

[teravest, 2014-08-18 21:53:12]

On Mon, Aug 18, 2014 at 2:06 PM,  <dmichael@chromium.org> wrote:
> I haven't taken the time to understand how this will help you for validation
> caching, but I think this change looks fine at this level.

There's an explanation in posts #7 and #8 in the linked bug. I mostly
wanted to make sure this still played nice with the threading
requirements of NaClIPCAdapter.

>
>
>
https://codereview.chromium.org/472073003/diff/20001/components/nacl/loader/n...
> File components/nacl/loader/nacl_ipc_adapter.cc (right):
>
>
https://codereview.chromium.org/472073003/diff/20001/components/nacl/loader/n...
> components/nacl/loader/nacl_ipc_adapter.cc:666:
> io_thread_data_.pending_sync_msgs_[id] = message->type();
> It kind of looks like you are adding the id to the map even for messages
> that are async? If that's the case, the map is growing unbounded with
> each async message.

Fixed. Restricted to sync now.

>
>
https://codereview.chromium.org/472073003/diff/20001/ppapi/proxy/nacl_message...
> File ppapi/proxy/nacl_message_scanner.cc (right):
>
>
https://codereview.chromium.org/472073003/diff/20001/ppapi/proxy/nacl_message...
> ppapi/proxy/nacl_message_scanner.cc:294: uint32_t type,
> This parameter would be pretty confusing I think to somebody new to this
> class. I think it needs a comment that for normal messages it matches
> msg.id(), but it will be the id of the original message if this is a
> sync reply. Or something.

I've added an extra comment.

>
> https://codereview.chromium.org/472073003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-08-18 22:43:40]

lgtm

[teravest, 2014-08-21 17:21:57]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-08-21 17:27:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/472073003/60001

[commit-bot: I haz the power, 2014-08-21 23:19:23]

Committed patchset #4 (60001) as 291234