Refactor params of NaClProcessMsg_Start.

Refactor params of NaClProcessMsg_Start.

This is a preparation to remove the resource leak related to NaClProcessMsg_Start IPC message.
- Get rid of std::pair<std::string, std::string> for the prefetch request, instead introduced a struct NaClResourcePrefetchRequest.
- Use the struct for JsonManifest::GetPrefetchableFiles.
- Expand handles to named fields to CHECK the passed handles more properly in nacl_listener.cc
- Get rid of nacl::FileDescriptor for the param, instead use IPC::PlatformFileForTransit.

TEST=Ran bots.
BUG=358417
Committed: https://crrev.com/0e949f991ae48c1f4ef91e78a96f986931093220
Cr-Commit-Position: refs/heads/master@{#327666}

[hidehiko, 2015-04-21 16:38:32]

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

[hidehiko, 2015-04-21 16:38:33]

PTAL.

Mark, during addressing your comments of
https://codereview.chromium.org/1094653003/, it turned out applying all your
comments need bigger change. So, I extract some part from that to be accelerate
code review speed. I'll rebase the CL on top of this, when this is submitted.

Yusuke,
FYI: as we chatted offline, this CL conflicts yours
https://codereview.chromium.org/1010183002/.
Although the lines are conflicting, the concept is independent, so I believe
this CL does not block your work.

Thank you for review in advance,
- hidehiko

[hidehiko, 2015-04-22 15:06:32]

On 2015/04/21 16:38:33, hidehiko wrote:
> PTAL.
> 
> Mark, during addressing your comments of
> https://codereview.chromium.org/1094653003/, it turned out applying all your
> comments need bigger change. So, I extract some part from that to be
accelerate
> code review speed. I'll rebase the CL on top of this, when this is submitted.
> 
> Yusuke,
> FYI: as we chatted offline, this CL conflicts yours
> https://codereview.chromium.org/1010183002/.
> Although the lines are conflicting, the concept is independent, so I believe
> this CL does not block your work.
> 
> Thank you for review in advance,
> - hidehiko

ping?

[Mark Seaborn, 2015-04-23 00:10:11]

OK, there are multiple parts to this:
 1) Replacing the 'handles' vector with fields
 2) Replacing a std::pair with "struct NaClResourcePrefetchInfo"
 3) Further refactorings (which I haven't wrapped my head around yet)

Can I suggest you do (1) and (2) in one change and (3) in another, to make the
review more incremental?

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:85: IPC::PlatformFileForTransit
imc_bootstrap_handle;
Splitting up 'handles' seems reasonable but the reason for it isn't clear.  Is
it a necessary part of the change or a bonus cleanup?  Please explain in the
commit message.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:87: IPC::PlatformFileForTransit mac_shm_fd;
If I understand this correctly, there's a subtle bug here:

On Unix, this is a base::FileDescriptor class -- safely initialised with a
default.

On Windows, this is a base::PlatformFile, which is a HANDLE -- not safely
initialised with a default.  So you'd need to add a default initialiser value in
nacl_types.cc for this to be safe.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:113: // Metadata to prefetch a resource
file.
Similarly, using a struct with names fields instead of std::pair<> seems
reasonable, but the reason for it isn't clear.  Is this a bonus cleanup or a
necessary change?

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:114: struct NaClResourcePrefetchInfo {
How about putting this next to NaClResourceFileInfo (above) and making the
relationship between them clearer?

One is a request (renderer->browser), the other is the result
(browser->nacl_loader).  How about reflecting that in the names?

Maybe:
NaClResourceFilePrefetchRequest,
NaClResourceFilePrefetchResult

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:120: std::string manifest_key;
In NaClResourceFileInfo, this is called "file_key".  Can you make the two
consistent?

You could also add a comment here to match NaClResourceFileInfo's field.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:135: const
std::vector<NaClResourcePrefetchInfo>& resource_prefetch_info_list,
Hmm, "files_to_prefetch" seems more descriptive to me than "prefetch_info_list".

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:430:
manifest->GetPrefetchableFiles(&candidates);
Would it make sense for GetPrefetchableFiles to use NaClResourcePrefetchInfo
instead of std::vector too?

[hidehiko, 2015-04-23 11:51:28]

hidehiko@chromium.org changed reviewers:
+ jln@chromium.org

[hidehiko, 2015-04-23 11:51:29]

R += jln@. Could you review *message* files as an OWNER?

> OK, there are multiple parts to this:
>  1) Replacing the 'handles' vector with fields
>  2) Replacing a std::pair with "struct NaClResourcePrefetchInfo"
>  3) Further refactorings (which I haven't wrapped my head around yet)
>
> Can I suggest you do (1) and (2) in one change and (3) in another, to make the
> review more incremental?

Done. This CL now contains only (1) and (2).
Note that (1) and (2) are just a simple refactoring, to write codes in a simpler
form. So, not mandatory.

(3), in your context, is the main part, which is;

- delay the IPC::PlatformFileForTransit and base::SharedMemory::ShareToProces,
until we decide sending an IPC to the plugin process, and delegate error
handling to the plugin process.

It includes;
- Move around some code inside StartNaClExecution().
- Move BatchOpenResourceFiles to nacl_processs_host.cc.
- Regadless of whether handles are created or not, send the IPC message, once we
decide. The error afterwords should be handled in the callee. (In this case,
just CHECKing them should work).

Note: I'm making similar CLs for IPC between a browser and a renderer process.
The goal of those CLs is to establish a way to manage resources properly, in
order to address your comments of crrev.com/1051243002. If it's ok for you to
leave the resource leaking on error cases in the CL, I'm ok to leave the current
resource leaking untouched. Otherwise, this (and following CLs) is the blocker
of crbug.com/358417 (indirectly), because without this kind of fix, there is no
direction/proper-way to manage resources to address your comments.

Thanks,
- hidehiko

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:85: IPC::PlatformFileForTransit
imc_bootstrap_handle;
On 2015/04/23 00:10:11, Mark Seaborn wrote:
> Splitting up 'handles' seems reasonable but the reason for it isn't clear.  Is
> it a necessary part of the change or a bonus cleanup?  Please explain in the
> commit message.

For cleaner handle check in nacl_listener.cc. Updated the description.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:87: IPC::PlatformFileForTransit mac_shm_fd;
On 2015/04/23 00:10:11, Mark Seaborn wrote:
> If I understand this correctly, there's a subtle bug here:
> 
> On Unix, this is a base::FileDescriptor class -- safely initialised with a
> default.
> 
> On Windows, this is a base::PlatformFile, which is a HANDLE -- not safely
> initialised with a default.  So you'd need to add a default initialiser value
in
> nacl_types.cc for this to be safe.

Ah, right. Fixed (including existing debug_stub_server_bound_socket).

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:113: // Metadata to prefetch a resource
file.
On 2015/04/23 00:10:10, Mark Seaborn wrote:
> Similarly, using a struct with names fields instead of std::pair<> seems
> reasonable, but the reason for it isn't clear.  Is this a bonus cleanup or a
> necessary change?

Bonus, but I was unhappy to distribute std::vector<std::pair<std::string,
std::string>>, and the meaning of first/second was confusing to me while I read
the code.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:114: struct NaClResourcePrefetchInfo {
On 2015/04/23 00:10:11, Mark Seaborn wrote:
> How about putting this next to NaClResourceFileInfo (above) and making the
> relationship between them clearer?
> 
> One is a request (renderer->browser), the other is the result
> (browser->nacl_loader).  How about reflecting that in the names?
> 
> Maybe:
> NaClResourceFilePrefetchRequest,
> NaClResourceFilePrefetchResult

Done.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:120: std::string manifest_key;
On 2015/04/23 00:10:11, Mark Seaborn wrote:
> In NaClResourceFileInfo, this is called "file_key".  Can you make the two
> consistent?
> 
> You could also add a comment here to match NaClResourceFileInfo's field.

Done.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:135: const
std::vector<NaClResourcePrefetchInfo>& resource_prefetch_info_list,
On 2015/04/23 00:10:11, Mark Seaborn wrote:
> Hmm, "files_to_prefetch" seems more descriptive to me than
"prefetch_info_list".

Renamed to resource_prefetch_request_list.
Note that I do not want to use files_to_prefetch, because it is not/does not
contain a file (FD/HANDLE), which is the important key for resource management
I'm fixing.

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:430:
manifest->GetPrefetchableFiles(&candidates);
On 2015/04/23 00:10:11, Mark Seaborn wrote:
> Would it make sense for GetPrefetchableFiles to use NaClResourcePrefetchInfo
> instead of std::vector too?

Done.

[Mark Seaborn, 2015-04-24 00:53:25]

Please add a BUG= field referencing the issue that motivated the change.  That's
BUG=358417, right?

LGTM

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/1085583005/diff/20001/components/nacl/common/...
components/nacl/common/nacl_types.h:135: const
std::vector<NaClResourcePrefetchInfo>& resource_prefetch_info_list,
On 2015/04/23 11:51:29, hidehiko wrote:
> On 2015/04/23 00:10:11, Mark Seaborn wrote:
> > Hmm, "files_to_prefetch" seems more descriptive to me than
> "prefetch_info_list".
> 
> Renamed to resource_prefetch_request_list.
> Note that I do not want to use files_to_prefetch, because it is not/does not
> contain a file (FD/HANDLE), which is the important key for resource management
> I'm fixing.

That reason seems a bit pedantic to me.  Files are not the same thing as file
descriptors or file handles, anyway.  So I think it's OK to name something as
"files" if it represents a set of files in the abstract (such as a set of
filenames).

FWIW, for that reason, I think "base::File" wasn't well named.

But anyhow, as long as NaClResourcePrefetchRequest/Result are commented as
representing files (as you've done in nacl_types.h), I don't mind if you use
"files_to_prefetch" or "prefetch_request_list".

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:269: // Note: this does not work on
Windows, though we currently support this
"this feature" -> "this prefetching feature"

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:271: // so it should be ok.
I'm not sure what you mean by "it should be ok".  Are you saying this code path
would not be correct for Windows?

If so, you could do something like:
#ifdef OS_WINDOWS
DCHECK_EQ(prefetched_resource_files_.size(), 0);
#endif

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:61: // Request to prefetch a resource file.
Please make the comment match up with NaClResourcePrefetchResult, which says:

// Represents a single prefetched file that's listed in the "files" section of
// a NaCl manifest file.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:63: NaClResourcePrefetchRequest();
Not used?  Avoiding defining a default ctor would be beneficial.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:68: std::string file_key;
Add comment "// a key for open_resource" to match NaClResourcePrefetchResult

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:75: NaClResourcePrefetchResult();
Is this default ctor unused now?

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:98: IPC::PlatformFileForTransit mac_shm_fd;
Can you comment that this is only used on Mac OS X?  Or better, put this in an
#ifdef.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:356: # if defined(OS_MACOSX)
You could move this outside "#if defined(OS_LINUX) || defined(OS_MACOSX)".  See
next comment...

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:360: CHECK(params.mac_shm_fd ==
IPC::InvalidPlatformFileForTransit());
Note that you're doing this check on OS_LINUX but not OS_WINDOWS.  If you define
mac_shm_fd conditionally that problem goes away.

[hidehiko, 2015-04-24 10:05:18]

Thank you for review.
jln@, friendly ping?

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/browser...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:269: // Note: this does not work on
Windows, though we currently support this
On 2015/04/24 00:53:24, Mark Seaborn wrote:
> "this feature" -> "this prefetching feature"

Done.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/browser...
components/nacl/browser/nacl_process_host.cc:271: // so it should be ok.
On 2015/04/24 00:53:24, Mark Seaborn wrote:
> I'm not sure what you mean by "it should be ok".  Are you saying this code
path
> would not be correct for Windows?
> 
> If so, you could do something like:
> #ifdef OS_WINDOWS
> DCHECK_EQ(prefetched_resource_files_.size(), 0);
> #endif

Done.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
File components/nacl/common/nacl_types.h (right):

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:61: // Request to prefetch a resource file.
On 2015/04/24 00:53:24, Mark Seaborn wrote:
> Please make the comment match up with NaClResourcePrefetchResult, which says:
> 
> // Represents a single prefetched file that's listed in the "files" section of
> // a NaCl manifest file.

Done.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:63: NaClResourcePrefetchRequest();
On 2015/04/24 00:53:25, Mark Seaborn wrote:
> Not used?  Avoiding defining a default ctor would be beneficial.

Used by IPC framework.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:68: std::string file_key;
On 2015/04/24 00:53:24, Mark Seaborn wrote:
> Add comment "// a key for open_resource" to match NaClResourcePrefetchResult

Done.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:75: NaClResourcePrefetchResult();
On 2015/04/24 00:53:24, Mark Seaborn wrote:
> Is this default ctor unused now?

Acknowledged.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/common/...
components/nacl/common/nacl_types.h:98: IPC::PlatformFileForTransit mac_shm_fd;
On 2015/04/24 00:53:24, Mark Seaborn wrote:
> Can you comment that this is only used on Mac OS X?  Or better, put this in an
> #ifdef.

Done.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/loader/...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:356: # if defined(OS_MACOSX)
On 2015/04/24 00:53:25, Mark Seaborn wrote:
> You could move this outside "#if defined(OS_LINUX) || defined(OS_MACOSX)". 
See
> next comment...

Acknowledged.

https://codereview.chromium.org/1085583005/diff/60001/components/nacl/loader/...
components/nacl/loader/nacl_listener.cc:360: CHECK(params.mac_shm_fd ==
IPC::InvalidPlatformFileForTransit());
On 2015/04/24 00:53:25, Mark Seaborn wrote:
> Note that you're doing this check on OS_LINUX but not OS_WINDOWS.  If you
define
> mac_shm_fd conditionally that problem goes away.

Now the field is guraded by OS_MACOSX #ifdef, so I reverted the condition to the
original.

[Mark Seaborn, 2015-04-24 14:26:13]

In case you missed this comment: "Please add a BUG= field referencing the issue
that motivated the change.  That's BUG=358417, right?"

https://codereview.chromium.org/1085583005/diff/100001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1085583005/diff/100001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:273:
CHECK(prefetched_resource_files_.empty());
Please use DCHECK to avoid the risk that this gives a renderer a way to crash
the browser process

[hidehiko, 2015-04-24 18:07:53]

On 2015/04/24 14:26:13, Mark Seaborn wrote:
> In case you missed this comment: "Please add a BUG= field referencing the
issue
> that motivated the change.  That's BUG=358417, right?"

Oops, sorry. Done. I mistakenly removed the line (and TEST= one) when I fully
rewrote the description. Thank you for pointing it out.

https://codereview.chromium.org/1085583005/diff/100001/components/nacl/browse...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/1085583005/diff/100001/components/nacl/browse...
components/nacl/browser/nacl_process_host.cc:273:
CHECK(prefetched_resource_files_.empty());
On 2015/04/24 14:26:12, Mark Seaborn wrote:
> Please use DCHECK to avoid the risk that this gives a renderer a way to crash
> the browser process

Oh, right. Done.

[hidehiko, 2015-04-27 17:44:41]

Julien, friendly ping?
- hidehiko

On 2015/04/24 18:07:53, hidehiko wrote:
> On 2015/04/24 14:26:13, Mark Seaborn wrote:
> > In case you missed this comment: "Please add a BUG= field referencing the
> issue
> > that motivated the change.  That's BUG=358417, right?"
> 
> Oops, sorry. Done. I mistakenly removed the line (and TEST= one) when I fully
> rewrote the description. Thank you for pointing it out.
> 
>
https://codereview.chromium.org/1085583005/diff/100001/components/nacl/browse...
> File components/nacl/browser/nacl_process_host.cc (right):
> 
>
https://codereview.chromium.org/1085583005/diff/100001/components/nacl/browse...
> components/nacl/browser/nacl_process_host.cc:273:
> CHECK(prefetched_resource_files_.empty());
> On 2015/04/24 14:26:12, Mark Seaborn wrote:
> > Please use DCHECK to avoid the risk that this gives a renderer a way to
crash
> > the browser process
> 
> Oh, right. Done.

[jln (very slow on Chromium), 2015-04-29 22:54:45]

lgtm because this seems like a net improvement, but I'm not very comfortable
with that code in general. I didn't do a thorough review, given that mseaborn
previously reviewed this.

mseaborn: when you review IPC CL, could you indicate if you approve of the IPC
changes? This would allow me to speed-up my review.

palmer: do you have any opinion on passing untrustworthy strings as a GURL
constructor? This doesn't strike me as particularly safe. It's not a change from
this CL, but worries me.
In fact, the very first line of the GURL constructor looks like it has an int
overflow.

[jln (very slow on Chromium), 2015-04-29 22:56:23]

abarth: do you have any opinion on whether GURL was design to take potentially
hostile arguments (from IPC) in its constructors?

[palmer, 2015-04-29 23:16:19]

Easy or hard, we simply have to make sure GURL can handle hostile inputs.

[hidehiko, 2015-04-30 04:14:01]

On 2015/04/29 23:16:19, palmer wrote:
> Easy or hard, we simply have to make sure GURL can handle hostile inputs.

Thank you for review and comment.
So, please let me submit this because concerns discussed in the thread do not
seem directly related this CL.

I filed a bug crbug.com/482819 for more discussion.

Thanks!
- hidehiko

[hidehiko, 2015-04-30 04:14:06]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-04-30 04:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1085583005/#ps120001
(title: " ")

[commit-bot: I haz the power, 2015-04-30 04:14:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085583005/120001

[commit-bot: I haz the power, 2015-04-30 04:17:30]

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

[commit-bot: I haz the power, 2015-04-30 04:17:30]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[hidehiko, 2015-04-30 06:18:42]

On 2015/04/30 04:17:30, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>   ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
>   ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
>   mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Oops, conflicting. Rebased and resubmitting.

[hidehiko, 2015-04-30 06:18:46]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2015-04-30 06:18:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, jln@chromium.org
Link to the patchset: https://codereview.chromium.org/1085583005/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-30 06:19:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1085583005/140001

[commit-bot: I haz the power, 2015-04-30 07:03:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-04-30 07:05:38]

Patchset 8 (id:??) landed as
https://crrev.com/0e949f991ae48c1f4ef91e78a96f986931093220
Cr-Commit-Position: refs/heads/master@{#327666}

[lpique, 2015-05-01 21:45:09]

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

[lpique, 2015-05-01 21:45:10]

I'll send out the fix shortly.

https://codereview.chromium.org/1085583005/diff/140001/components/nacl/browse...
File components/nacl/browser/nacl_host_message_filter.cc (right):

https://codereview.chromium.org/1085583005/diff/140001/components/nacl/browse...
components/nacl/browser/nacl_host_message_filter.cc:208: for (size_t i = 0;
request_list.size(); ++i) {
crbug.com/483633