Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
140573003:
Connect PPAPI IPC channels for non-SFI mode. (Closed)
|
Created:
6 years, 10 months ago by hidehiko Modified:
6 years, 10 months ago Reviewers:
jam, dmichael (off chromium), teravest, jln (very slow on Chromium), Mark Seaborn, Tom Sepez, brettw CC:
chromium-reviews, darin-cc_chromium.org, jam, hamaji, mazda, rmcilroy Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConnect PPAPI IPC channels for non-SFI mode.
By this CL, plugin starts to talk with hosts via IPC.
For that purpose, ppapi_proxy is linked to nacl_helper (temporarily). This increase the size of nacl_helper, but when we split nacl_helper for non-sfi mode, it'll be resolved.
In SFI mode, this CL shouldn't affect the concept of IPC
channel connection between the plugin and the hosts. We
still use NaClIPCAdapter to wrap the IPC channel, and
NaClDesc for the plugin-side IPC file descriptors.
In non-SFI mode, we neither intercept nor rewrite the
message using NaClIPCAdapter, and the channels are
connected between the plugin and the hosts directly.
Note: plugin_main_nacl.cc is renamed to plugin_main.cc, because files with _nacl.cc suffix are automatically excluded from the sources of non-untrusted libs.
This increases the size of nacl_helper (temporarily) intentionally.
GYP_DEFINES="target_arch=ia32 remove_webcore_debug_symbols=1 linux_strip_symbols=1" GYP_GENERATORS="ninja" gclient runhooks
Before:
text data bss dec hex filename
1469882 15576 108644 1594102 1852f6 out/Release/nacl_helper
After:
text data bss dec hex filename
5641443 124636 126980 5893059 59ebc3 out/Release/nacl_helper
GYP_DEFINES="target_arch=x64 remove_webcore_debug_symbols=1 linux_strip_symbols=1" GYP_GENERATORS="ninja" gclient runhooks
Before:
text data bss dec hex filename
2063530 27910 213872 2305312 232d20 out/Release/nacl_helper
After:
text data bss dec hex filename
6304467 234424 247984 6786875 678f3b out/Release/nacl_helper
BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=Ran trybot.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252503
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=252556
Patch Set 1 : #
Total comments: 28
Patch Set 2 : Rebase and address review comments. #
Total comments: 8
Patch Set 3 : #
Total comments: 13
Patch Set 4 : #
Total comments: 34
Patch Set 5 : #
Total comments: 43
Patch Set 6 : Rebase #Patch Set 7 : #Patch Set 8 : Rebase #Patch Set 9 : Fix build error #
Messages
Total messages: 55 (0 generated)
https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:272: if (params.enable_nonsfi_mode) { You should be able to use this new SocketPair()-based code path for all OS_POSIX, not conditional on enable_nonsfi_mode. This would help reduce the number of conditionals and code paths to test. https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... File components/nacl/loader/nonsfi/irt_filename.cc (right): https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... components/nacl/loader/nonsfi/irt_filename.cc:4: Same comment as on https://codereview.chromium.org/139993009/ about leaving out irt_filename.cc. https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/plugin.cc:388: #if 0 So the trusted plugin will need to know whether we're using Non-SFI Mode if we're going to turn this into a runtime check. I think we need to have the browser tell the renderer (and trusted plugin) whether Non-SFI Mode is enabled. This probably needs to be plumbed through via ppapi/c/private/ppb_nacl_private.h. In the long term, the trusted plugin needs to know something about Non-SFI Mode anyway. Eventually we'll want to have separate "arm" and "arm-nonsfi" entries in the NaCl manifest files. The question would be whether we want to start implementing that yet. Even if we have that, I think the plugin still needs to know whether to ignore the "arm-nonsfi" entry based on whether Non-SFI Mode is enabled/disabled browser-wide. Another option here is to implement enough of the SRPC "reverse service" stuff in components/nacl/loader/nonsfi/nonsfi_main.cc such that StartSrpcServices() here succeeds. I'm reluctant to recommend doing that, given that the code for that will have a limited lifetime. https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.cc:516: #if 0 Same comments here as in plugin.cc about runtime enabling. https://codereview.chromium.org/140573003/diff/30001/ppapi/ppapi_proxy.gypi File ppapi/ppapi_proxy.gypi (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/ppapi_proxy.gypi#n... ppapi/ppapi_proxy.gypi:272: ['>(nacl_untrusted_build)==0 and <(chromeos)!=1 and OS!="linux"', { Why "<(chromeos)!=1"? This could use a comment explaining that this is for Non-SFI Mode. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.cc File ppapi/proxy/plugin_main.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:4: Can you keep this file named as plugin_main_nacl.cc? I'd say Non-SFI Mode is still NaCl -- most of the code is in components/nacl/ already. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:165: #if defined(__native_client_) Can you comment why this is conditional? https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:327: #if defined(__native_client__) Can you add a comment explaining why, with a TODO? https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:347: #if defined(__native_client__) Can you add a comment saying why this is conditional? https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:361: ppapi::proxy::g_ppapi_start_event.Pointer()->Signal(); As I commented on https://codereview.chromium.org/139993009/, this shouldn't be necessary. load_module can return immediately. It doesn't have to wait until user code calls ppapi_start() before returning.
One other thing: When this is committed, it will increase the size of nacl_helper, because the PPAPI proxy will start to be linked into it. Can you measure the size increase, please? You will likely need to increase the sizes in perf_expectations after committing this. I think the size increase will be OK, because this will be in Linux builds only, and it will be temporary until we switch to building a separate executable for Non-SFI Mode (using newlib eventually).
Thank you for your comment. First of all, sorry for the confusion with [WIP] CLs. (It is convenient for my work to keep some WIP CLs open (like, sharing, tracking, updating some demos)...). I'll keep silent on such CLs, and ask you with CLs which I'd like you to read. Note: This is still a "giant CL for the demo," and I will extract some part from this and create a new CL for the code review, with clean up. Before that, I'd like to make it clear some implementation direction. Could you focus on nacl_listener.cc:272, and plugin_main.cc:361? Also, I'd appreciate if you give me a comment for plugin.cc:388 for the next step. Thanks! - hidehiko https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:272: if (params.enable_nonsfi_mode) { On 2014/02/07 23:54:10, Mark Seaborn wrote: > You should be able to use this new SocketPair()-based code path for all > OS_POSIX, not conditional on enable_nonsfi_mode. This would help reduce the > number of conditionals and code paths to test. Just for clarification: Even if we use raw SocketPair approach for the OS_POSIX, we still need to have "if (enable_nonsfi_mode) {...}" check somehow, because it is necessary to set up NaClIPCAdapter for SFI-mode, but on the other hand it is necessary to set global FD var for non-SFI-mode. Also, we need to keep the current code path (or something similar) for OS_WIN, so IMHO we couldn't simplify much the code around here, in terms of reducing the condition? WDYT? This is just a working demo CL, so I'll address it in a new CL for the code review. https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... File components/nacl/loader/nonsfi/irt_filename.cc (right): https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... components/nacl/loader/nonsfi/irt_filename.cc:4: On 2014/02/07 23:54:10, Mark Seaborn wrote: > Same comment as on https://codereview.chromium.org/139993009/ about leaving out > irt_filename.cc. Sure. This is just a "giant demo CL", and this quick-hack irt is very convenient for us to debug/test with our app. I'll remove them from the CL for the code review. https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/plugin.cc:388: #if 0 On 2014/02/07 23:54:10, Mark Seaborn wrote: > So the trusted plugin will need to know whether we're using Non-SFI Mode if > we're going to turn this into a runtime check. > > I think we need to have the browser tell the renderer (and trusted plugin) > whether Non-SFI Mode is enabled. This probably needs to be plumbed through via > ppapi/c/private/ppb_nacl_private.h. > > In the long term, the trusted plugin needs to know something about Non-SFI Mode > anyway. Eventually we'll want to have separate "arm" and "arm-nonsfi" entries > in the NaCl manifest files. The question would be whether we want to start > implementing that yet. Even if we have that, I think the plugin still needs to > know whether to ignore the "arm-nonsfi" entry based on whether Non-SFI Mode is > enabled/disabled browser-wide. > > Another option here is to implement enough of the SRPC "reverse service" stuff > in components/nacl/loader/nonsfi/nonsfi_main.cc such that StartSrpcServices() > here succeeds. I'm reluctant to recommend doing that, given that the code for > that will have a limited lifetime. Thank you for pointing this out. So, I think this is the next work I should focus on. (I'm planning to exclude the part from the pepper-connection CL which will be sent to the code review). My understanding is that: Currently, the browser has a flag, and it checks if the plugin is non-sfi mode or not. (i.e., all NaCl plugin runs non-sfi mode, right now). Instead, we'd probably like to; - pass the flag to the renderer somehow (by directly passing the flag or using private ppapi as you suggested?) - if non-sfi mode is enabled and there is an entry for "***-nonsfi" in NMF, the rednerer sends a message to launch non-sfi NaCl. - Considering that we'll eventually split the nacl_helper binary and bare_metal_helper binary, it is necessary for the renderer to know if non-sfi is enabled or not at very beginning of the NaCl plugin initialization, in order to tell browser/zygote to launch the appropriate binary. As for reimplementing SRPC "reverse service", IIUC it is for the irt_resource_open implementation. Is it correct? If so, actually we need to have the IRT, so we need either 1) waiting for SRPC replacement or 2) reusing the current SRPC service for a short term solution. IMHO it's up to when SRPC will be retired? So, if we need to use SRPC somehow (before the retire), "working on it first, then working on the implementation of non-sfi switching on the renderer" may be an option. WDYT? https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/service_runtime.cc:516: #if 0 On 2014/02/07 23:54:10, Mark Seaborn wrote: > Same comments here as in plugin.cc about runtime enabling. Acknowledged. https://codereview.chromium.org/140573003/diff/30001/ppapi/ppapi_proxy.gypi File ppapi/ppapi_proxy.gypi (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/ppapi_proxy.gypi#n... ppapi/ppapi_proxy.gypi:272: ['>(nacl_untrusted_build)==0 and <(chromeos)!=1 and OS!="linux"', { On 2014/02/07 23:54:10, Mark Seaborn wrote: > Why "<(chromeos)!=1"? > > This could use a comment explaining that this is for Non-SFI Mode. I just wanted to include this even for chromeos, but it turned out the OS=="linux" is true for chromeos. I'll address this in the code review CL. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.cc File ppapi/proxy/plugin_main.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:4: On 2014/02/07 23:54:10, Mark Seaborn wrote: > Can you keep this file named as plugin_main_nacl.cc? > > I'd say Non-SFI Mode is still NaCl -- most of the code is in components/nacl/ > already. We need to remove _nacl.cc suffix, because we need to include the file into non-unstrusted lib. Note that files with _nacl.cc suffix will be automatically removed from the sources list, if the lib is not for untrusted. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:165: #if defined(__native_client_) On 2014/02/07 23:54:10, Mark Seaborn wrote: > Can you comment why this is conditional? Oops sorry, I don't think we need this ifdef guard (this is just a junk from my test code). I'll remove this from the code-review CL. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:327: #if defined(__native_client__) On 2014/02/07 23:54:10, Mark Seaborn wrote: > Can you add a comment explaining why, with a TODO? This is just not yet implemented on non-SFI mode. Will add TODO. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:347: #if defined(__native_client__) On 2014/02/07 23:54:10, Mark Seaborn wrote: > Can you add a comment saying why this is conditional? IIUC, this SRPC is for open_resource, and it will be replaced by IPC channels. Is it correct? Also, if so, can we postpone to work on re-enabling SRPC server for now with TODO? https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:361: ppapi::proxy::g_ppapi_start_event.Pointer()->Signal(); On 2014/02/07 23:54:10, Mark Seaborn wrote: > As I commented on https://codereview.chromium.org/139993009/, this shouldn't be > necessary. load_module can return immediately. It doesn't have to wait until > user code calls ppapi_start() before returning. I think there is a circular dependency again here. If we return load-module SRPC immediately, as we already created the socket pair and the client-side FD is already passed to the browser/renderer, they "can" send a message to the plugin, even before here. (It'd be queued). IIUC, actually the renderer sends an IPC to the plugin just after load-module (and following procedures I skipped by "#if 0" guard) is done. Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it starts to listen the channel on IO thread. This could happen before adding some filters (timing issue), so that some initial messages would be wrongly handled. Can we change the initialization order inside Dispatchers, instead? Otherwise, it is necessary to wait for the Dispatcher initialization completion. WDYT?
On 10 February 2014 00:18, <hidehiko@chromium.org> wrote: > Reviewers: Mark Seaborn, > > Message: > Thank you for your comment. > > First of all, sorry for the confusion with [WIP] CLs. (It is convenient > for my > work to keep some WIP CLs open (like, sharing, tracking, updating some > demos)...). I'll keep silent on such CLs, and ask you with CLs which I'd > like > you to read. > > Note: This is still a "giant CL for the demo," and I will extract some > part from > this and create a new CL for the code review, with clean up. > Can you upload your updates to this CL rather than creating a third one, please? It easier for me to compare changes (and my own comments) within the same Rietveld CL than across multiple Rietveld CLs. If you want to keep around a CL that includes the irt_filename changes, maybe you can create that as the new CL (and never send me a link to it, to avoid confusing me ;-) ). Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/30001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:272: if (params.enable_nonsfi_mode) { On 2014/02/10 08:18:34, hidehiko wrote: > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > You should be able to use this new SocketPair()-based code path for all > > OS_POSIX, not conditional on enable_nonsfi_mode. This would help reduce the > > number of conditionals and code paths to test. > > Just for clarification: > Even if we use raw SocketPair approach for the OS_POSIX, > we still need to have "if (enable_nonsfi_mode) {...}" check somehow, because it > is necessary to set up NaClIPCAdapter for SFI-mode, but on the other hand it is > necessary to set global FD var for non-SFI-mode. > Also, we need to keep the current code path (or something similar) for OS_WIN, > so IMHO we couldn't simplify much the code around here, in terms of reducing the > condition? > WDYT? Oops, yes, I see what you mean. Your current code is OK. https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/native_client/src/... ppapi/native_client/src/trusted/plugin/plugin.cc:388: #if 0 On 2014/02/10 08:18:34, hidehiko wrote: > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > So the trusted plugin will need to know whether we're using Non-SFI Mode if > > we're going to turn this into a runtime check. > > > > I think we need to have the browser tell the renderer (and trusted plugin) > > whether Non-SFI Mode is enabled. This probably needs to be plumbed through > via > > ppapi/c/private/ppb_nacl_private.h. > > > > In the long term, the trusted plugin needs to know something about Non-SFI > Mode > > anyway. Eventually we'll want to have separate "arm" and "arm-nonsfi" entries > > in the NaCl manifest files. The question would be whether we want to start > > implementing that yet. Even if we have that, I think the plugin still needs > to > > know whether to ignore the "arm-nonsfi" entry based on whether Non-SFI Mode is > > enabled/disabled browser-wide. > > > > Another option here is to implement enough of the SRPC "reverse service" stuff > > in components/nacl/loader/nonsfi/nonsfi_main.cc such that StartSrpcServices() > > here succeeds. I'm reluctant to recommend doing that, given that the code for > > that will have a limited lifetime. > > Thank you for pointing this out. > So, I think this is the next work I should focus on. (I'm planning to exclude > the part from the pepper-connection CL which will be sent to the code review). > > My understanding is that: > > Currently, the browser has a flag, and it checks if the plugin is non-sfi mode > or not. (i.e., all NaCl plugin runs non-sfi mode, right now). > > Instead, we'd probably like to; > - pass the flag to the renderer somehow (by directly passing the flag or using > private ppapi as you suggested?) Yes. > - if non-sfi mode is enabled and there is an entry for "***-nonsfi" in NMF, the > rednerer sends a message to launch non-sfi NaCl. Yes, but this is not necessary for the current change. > - Considering that we'll eventually split the nacl_helper binary and > bare_metal_helper binary, it is necessary for the renderer to know if non-sfi is > enabled or not at very beginning of the NaCl plugin initialization, in order to > tell browser/zygote to launch the appropriate binary. You mean at browser startup? It's not really up to the renderer to tell the browser what zygote processes to launch at startup. > As for reimplementing SRPC "reverse service", IIUC it is for the > irt_resource_open implementation. Is it correct? Yes. > If so, actually we need to have > the IRT, so we need either 1) waiting for SRPC replacement or 2) reusing the > current SRPC service for a short term solution. IMHO it's up to when SRPC will > be retired? Right. For open_resource(), I'm inclined to wait for Justin TerAvest to refactor this to use Chrome IPC before we try to hook this up to Non-SFI Mode. But that will involve co-ordinating with Justin. > So, if we need to use SRPC somehow (before the retire), "working on it first, > then working on the implementation of non-sfi switching on the renderer" may be > an option. WDYT? Well, whichever way things go, the trusted plugin will eventually need to know whether Non-SFI Mode is enabled. So I think it makes most sense to plumb that through to the trusted plugin now, and not add any more SRPC code to components/nacl/loader/nonsfi/ for now. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.cc File ppapi/proxy/plugin_main.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:4: On 2014/02/10 08:18:34, hidehiko wrote: > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > Can you keep this file named as plugin_main_nacl.cc? > > > > I'd say Non-SFI Mode is still NaCl -- most of the code is in components/nacl/ > > already. > > We need to remove _nacl.cc suffix, because we need to include the file into > non-unstrusted lib. Note that files with _nacl.cc suffix will be automatically > removed from the sources list, if the lib is not for untrusted. OK, I see. Can you explain that in the commit message, please? https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:347: #if defined(__native_client__) On 2014/02/10 08:18:34, hidehiko wrote: > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > Can you add a comment saying why this is conditional? > > IIUC, this SRPC is for open_resource, and it will be replaced by IPC > channels. Is it correct? Actually, the SRPC server launched by this call probably isn't used at all. (open_resource() is handled by different code.) > Also, if so, can we postpone to work on re-enabling SRPC server for now > with TODO? Yes, of course. I was just asking for a comment in the code explaining why the conditional is there... https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:361: ppapi::proxy::g_ppapi_start_event.Pointer()->Signal(); On 2014/02/10 08:18:34, hidehiko wrote: > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > As I commented on https://codereview.chromium.org/139993009/, this shouldn't > be > > necessary. load_module can return immediately. It doesn't have to wait until > > user code calls ppapi_start() before returning. > > I think there is a circular dependency again here. > If we return load-module SRPC immediately, as we already created the socket pair > and the client-side FD is already passed to the browser/renderer, they "can" > send a message to the plugin, even before here. (It'd be queued). IIUC, actually > the renderer sends an IPC to the plugin just after load-module (and following > procedures I skipped by "#if 0" guard) is done. That should be OK because, as you say, the message is queued... > Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it starts > to listen the channel on IO thread. This could happen before adding some filters > (timing issue), so that some initial messages would be wrongly handled. Can we > change the initialization order inside Dispatchers, instead? Otherwise, it is > necessary to wait for the Dispatcher initialization completion. Are you speculating, or have you observed this to be a problem? e.g. Is there a problem with startup if you insert a sleep in a place that would make the race wider? I would guess there isn't a problem, because: 1) the message can arrive early currently under NaCl and this presumably works; 2) I'd guess channel()->AddFilter() sets a filter that will be used by the current thread after the current thread returns to the event loop and receives the message, so it doesn't matter if the IO thread has already received the message when AddFilter() is called. But I could be wrong about (2) (since AddFilter() claims a lock), in which case I'd be wrong about (1) as well.
On 10 February 2014 10:57, <mseaborn@chromium.org> wrote: > > Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it >> starts > > to listen the channel on IO thread. This could happen before adding some >> filters > > (timing issue), so that some initial messages would be wrongly handled. >> Can we > > change the initialization order inside Dispatchers, instead? Otherwise, >> it is > > necessary to wait for the Dispatcher initialization completion. >> > > Are you speculating, or have you observed this to be a problem? e.g. Is > there a problem with startup if you insert a sleep in a place that would > make the race wider? > > I would guess there isn't a problem, because: > 1) the message can arrive early currently under NaCl and this > presumably works; > 2) I'd guess channel()->AddFilter() sets a filter that will be used by > the current thread after the current thread returns to the event loop > and receives the message, so it doesn't matter if the IO thread has > already received the message when AddFilter() is called. > > But I could be wrong about (2) (since AddFilter() claims a lock), in > which case I'd be wrong about (1) as well. > I was looking into SyncChannel and ChannelProxy in the context of some refactoring Justin is doing (https://codereview.chromium.org/149403005/). I noticed this comment: void ChannelProxy::Context::OnChannelConnected(int32 peer_pid) { // Add any pending filters. This avoids a race condition where someone // creates a ChannelProxy, calls AddFilter, and then right after starts the // peer process. The IO thread could receive a message before the task to add // the filter is run on the IO thread. OnAddFilter(); Is that the problem you're referring to? Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi, thank you for your review. (I have a bit trouble on my local env, and today was a holiday in Japan. I'll update the CL tomorrow JST, but please let me reply to your last comment in advance). On 2014/02/11 00:56:16, Mark Seaborn wrote: > On 10 February 2014 10:57, <mailto:mseaborn@chromium.org> wrote: > > > > > Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it > >> starts > > > > to listen the channel on IO thread. This could happen before adding some > >> filters > > > > (timing issue), so that some initial messages would be wrongly handled. > >> Can we > > > > change the initialization order inside Dispatchers, instead? Otherwise, > >> it is > > > > necessary to wait for the Dispatcher initialization completion. > >> > > > > Are you speculating, or have you observed this to be a problem? e.g. Is > > there a problem with startup if you insert a sleep in a place that would > > make the race wider? > > > > I would guess there isn't a problem, because: > > 1) the message can arrive early currently under NaCl and this > > presumably works; > > 2) I'd guess channel()->AddFilter() sets a filter that will be used by > > the current thread after the current thread returns to the event loop > > and receives the message, so it doesn't matter if the IO thread has > > already received the message when AddFilter() is called. > > > > But I could be wrong about (2) (since AddFilter() claims a lock), in > > which case I'd be wrong about (1) as well. > > > > I was looking into SyncChannel and ChannelProxy in the context of some > refactoring Justin is doing (https://codereview.chromium.org/149403005/). > > I noticed this comment: > > void ChannelProxy::Context::OnChannelConnected(int32 peer_pid) { > // Add any pending filters. This avoids a race condition where someone > // creates a ChannelProxy, calls AddFilter, and then right after starts > the > // peer process. The IO thread could receive a message before the task > to add > // the filter is run on the IO thread. > OnAddFilter(); > > Is that the problem you're referring to? Yes. In more precise, my concern is that "Filter"s are called on IPC::Channel thread, which is IO thread here. (Pls see ChannelProxy::Context::OnMessageReceived and TryFilters). So, they'd not be called for some messages depending on the initialization timing. I'll try to reproduce it by adding sleep as you suggested (and will keep you updated), but IMHO starting IPC channels before adding Filters is not a good manner, even if it weren't reproduced by some reason... Assuming it is actually problem, I used an WaitableEvent for synchronization. An alternative would be refactoring the existing Dispatchers somehow to add Filters before the IPC channel is connected, I think. WDYT? - hidehiko > > Cheers, > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
PTAL? I think it's time to move to the code review, from the brief discussion of the design based on the CL. Note that it was impossible to "git cl patch" on trunk, so I manually patched needed files. (And that's why I'd like to create a brand new patch..., anyway). Could you tell me who should I ask for ipc_channel.h and ppapi/... files? Thanks, - hidehiko On 2014/02/11 19:30:20, hidehiko wrote: > Hi, thank you for your review. > > (I have a bit trouble on my local env, and today was a holiday in Japan. I'll > update the CL tomorrow JST, but please let me reply to your last comment in > advance). > > > On 2014/02/11 00:56:16, Mark Seaborn wrote: > > On 10 February 2014 10:57, <mailto:mseaborn@chromium.org> wrote: > > > > > > > > Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it > > >> starts > > > > > > to listen the channel on IO thread. This could happen before adding some > > >> filters > > > > > > (timing issue), so that some initial messages would be wrongly handled. > > >> Can we > > > > > > change the initialization order inside Dispatchers, instead? Otherwise, > > >> it is > > > > > > necessary to wait for the Dispatcher initialization completion. > > >> > > > > > > Are you speculating, or have you observed this to be a problem? e.g. Is > > > there a problem with startup if you insert a sleep in a place that would > > > make the race wider? > > > > > > I would guess there isn't a problem, because: > > > 1) the message can arrive early currently under NaCl and this > > > presumably works; > > > 2) I'd guess channel()->AddFilter() sets a filter that will be used by > > > the current thread after the current thread returns to the event loop > > > and receives the message, so it doesn't matter if the IO thread has > > > already received the message when AddFilter() is called. > > > > > > But I could be wrong about (2) (since AddFilter() claims a lock), in > > > which case I'd be wrong about (1) as well. > > > > > > > I was looking into SyncChannel and ChannelProxy in the context of some > > refactoring Justin is doing (https://codereview.chromium.org/149403005/). > > > > I noticed this comment: > > > > void ChannelProxy::Context::OnChannelConnected(int32 peer_pid) { > > // Add any pending filters. This avoids a race condition where someone > > // creates a ChannelProxy, calls AddFilter, and then right after starts > > the > > // peer process. The IO thread could receive a message before the task > > to add > > // the filter is run on the IO thread. > > OnAddFilter(); > > > > Is that the problem you're referring to? > > Yes. In more precise, my concern is that "Filter"s are called on IPC::Channel > thread, > which is IO thread here. (Pls see ChannelProxy::Context::OnMessageReceived and > TryFilters). > So, they'd not be called for some messages depending on the initialization > timing. > I'll try to reproduce it by adding sleep as you suggested (and will keep you > updated), > but IMHO starting IPC channels before adding Filters is not a good manner, > even if it weren't reproduced by some reason... > > Assuming it is actually problem, I used an WaitableEvent for synchronization. > An alternative would be refactoring the existing Dispatchers somehow to add > Filters before > the IPC channel is connected, I think. > > WDYT? > > - hidehiko > > > > > > Cheers, > > Mark > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
Oops, sorry, I forgot to send replies... https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.cc File ppapi/proxy/plugin_main.cc (right): https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:4: On 2014/02/10 18:57:35, Mark Seaborn wrote: > On 2014/02/10 08:18:34, hidehiko wrote: > > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > > Can you keep this file named as plugin_main_nacl.cc? > > > > > > I'd say Non-SFI Mode is still NaCl -- most of the code is in > components/nacl/ > > > already. > > > > We need to remove _nacl.cc suffix, because we need to include the file into > > non-unstrusted lib. Note that files with _nacl.cc suffix will be automatically > > removed from the sources list, if the lib is not for untrusted. > > OK, I see. Can you explain that in the commit message, please? Done. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:347: #if defined(__native_client__) On 2014/02/10 18:57:35, Mark Seaborn wrote: > On 2014/02/10 08:18:34, hidehiko wrote: > > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > > Can you add a comment saying why this is conditional? > > > > IIUC, this SRPC is for open_resource, and it will be replaced by IPC > > channels. Is it correct? > > Actually, the SRPC server launched by this call probably isn't used at all. > (open_resource() is handled by different code.) > > > Also, if so, can we postpone to work on re-enabling SRPC server for now > > with TODO? > > Yes, of course. I was just asking for a comment in the code explaining why the > conditional is there... Done. https://codereview.chromium.org/140573003/diff/30001/ppapi/proxy/plugin_main.... ppapi/proxy/plugin_main.cc:361: ppapi::proxy::g_ppapi_start_event.Pointer()->Signal(); On 2014/02/10 18:57:35, Mark Seaborn wrote: > On 2014/02/10 08:18:34, hidehiko wrote: > > On 2014/02/07 23:54:10, Mark Seaborn wrote: > > > As I commented on https://codereview.chromium.org/139993009/, this shouldn't > > be > > > necessary. load_module can return immediately. It doesn't have to wait > until > > > user code calls ppapi_start() before returning. > > > > I think there is a circular dependency again here. > > If we return load-module SRPC immediately, as we already created the socket > pair > > and the client-side FD is already passed to the browser/renderer, they "can" > > send a message to the plugin, even before here. (It'd be queued). IIUC, > actually > > the renderer sends an IPC to the plugin just after load-module (and following > > procedures I skipped by "#if 0" guard) is done. > > That should be OK because, as you say, the message is queued... > > > Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it > starts > > to listen the channel on IO thread. This could happen before adding some > filters > > (timing issue), so that some initial messages would be wrongly handled. Can we > > change the initialization order inside Dispatchers, instead? Otherwise, it is > > necessary to wait for the Dispatcher initialization completion. > > Are you speculating, or have you observed this to be a problem? e.g. Is there a > problem with startup if you insert a sleep in a place that would make the race > wider? > > I would guess there isn't a problem, because: > 1) the message can arrive early currently under NaCl and this presumably works; > 2) I'd guess channel()->AddFilter() sets a filter that will be used by the > current thread after the current thread returns to the event loop and receives > the message, so it doesn't matter if the IO thread has already received the > message when AddFilter() is called. > > But I could be wrong about (2) (since AddFilter() claims a lock), in which case > I'd be wrong about (1) as well. I still think we have the problem as I wrote in the previous reply. However, I found that my fix was wrong (we need to wait until the Listener for the IPC channel between the plugin and the renderer is initialized). Also, we still have an option to change some initialization of IPC channel filters, I think. WDYT? - hidehiko
On 12 February 2014 07:00, <hidehiko@chromium.org> wrote: > On 2014/02/10 18:57:35, Mark Seaborn wrote: > >> On 2014/02/10 08:18:34, hidehiko wrote: >> > Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, >> it starts >> > > to listen the channel on IO thread. This could happen before adding some >> filters > > > (timing issue), so that some initial messages would be wrongly handled. >> Can we > > > change the initialization order inside Dispatchers, instead? Otherwise, >> it is > > > necessary to wait for the Dispatcher initialization completion. >> > > Are you speculating, or have you observed this to be a problem? e.g. Is >> there a > > problem with startup if you insert a sleep in a place that would make the >> race > > wider? >> > > I would guess there isn't a problem, because: >> 1) the message can arrive early currently under NaCl and this >> presumably works; > > 2) I'd guess channel()->AddFilter() sets a filter that will be used by >> the > > current thread after the current thread returns to the event loop and >> receives > > the message, so it doesn't matter if the IO thread has already received >> the > > message when AddFilter() is called. >> > > But I could be wrong about (2) (since AddFilter() claims a lock), in >> which case > > I'd be wrong about (1) as well. >> > > I still think we have the problem as I wrote in the previous reply. > However, I found that my fix was wrong (we need to wait until the > Listener for the IPC channel between the plugin and the renderer is > initialized). > > Also, we still have an option to change some initialization of IPC > channel filters, I think. WDYT? On this AddFilter() problem, two things aren't clear to me still: * Is this a problem with existing NaCl (SFI mode)? If so, can you file a bug, please? * Can you reproduce this problem by adding a sleep (in either SFI and non-SFI mode)? Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Some incomplete, minor comments below. Today I tried out the patch. I ran a small libc-free test program under Non-SFI Mode on x86-64, using PPAPI's CallOnMainThread() successfully. I'll share the test program with you soon... https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... File components/nacl/loader/nonsfi/DEPS (right): https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... components/nacl/loader/nonsfi/DEPS:2: "+native_client/src/untrusted/irt/irt_ppapi.h", Ditto. (I've moved this to ppapi/nacl_irt/irt_ppapi.h.) https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_ppapi.cc (right): https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:8: #include "native_client/src/untrusted/irt/irt_ppapi.h" I've moved this to ppapi/nacl_irt/irt_ppapi.h in a recent change. https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:33: extern "C" { Nit: You shouldn't need this 'extern' if you #include ppapi/c/ppp.h. https://codereview.chromium.org/140573003/diff/270001/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/140573003/diff/270001/ipc/ipc_channel.h#newco... ipc/ipc_channel.h:220: #if defined(OS_LINUX) Nit: OS_POSIX, for consistency with the comment and the .cc file? Or should this decl just go in ipc_channel_posix.h?
Thank you for your review. > * Is this a problem with existing NaCl (SFI mode)? If so, can you file a bug, please? > * Can you reproduce this problem by adding a sleep (in either SFI and non-SFI mode)? I actually checked that the filters are skipped for first two messages by adding sleep(). However, interestingly it doesn't seem to cause a visible problem at the moment, because the filters do nothing at least for the first two messages, so that they are just processed on the main thread. The renderer looks waiting for the completion of its initialization until the reply of the second message arrives. Thus, the plugin looks all over working normally (I'm not sure this is the intentional behavior or not, though...). Questions: - Is this considered a bug in your team? (depending on the team, this kind of case may or may not be a bug...) - What's the recommendation for the next action? 1) Just remove the sync code in this CL (as currently no visible issue is found). Optionally file a bug (or fix it) separately later. 2) Remove the sync code in this CL, and fix the initialization code at the same time. 3) Keep the sync code for the protection. And remove it after fixing the issue. 4) Something else. WDYT? Thanks, - hidehiko https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... File components/nacl/loader/nonsfi/DEPS (right): https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... components/nacl/loader/nonsfi/DEPS:2: "+native_client/src/untrusted/irt/irt_ppapi.h", On 2014/02/13 06:18:25, Mark Seaborn wrote: > Ditto. (I've moved this to ppapi/nacl_irt/irt_ppapi.h.) Done. https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_ppapi.cc (right): https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:8: #include "native_client/src/untrusted/irt/irt_ppapi.h" On 2014/02/13 06:18:25, Mark Seaborn wrote: > I've moved this to ppapi/nacl_irt/irt_ppapi.h in a recent change. Oh, I didn't notice it. Done. https://codereview.chromium.org/140573003/diff/270001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:33: extern "C" { On 2014/02/13 06:18:25, Mark Seaborn wrote: > Nit: You shouldn't need this 'extern' if you #include ppapi/c/ppp.h. Done. https://codereview.chromium.org/140573003/diff/270001/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/140573003/diff/270001/ipc/ipc_channel.h#newco... ipc/ipc_channel.h:220: #if defined(OS_LINUX) On 2014/02/13 06:18:25, Mark Seaborn wrote: > Nit: OS_POSIX, for consistency with the comment and the .cc file? Or should > this decl just go in ipc_channel_posix.h? Done to change the macro. ipc_channel_posix.h seems to provide an implementation of IPC::Channel internal structure for posix?
https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:22: #include "ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h" I don't really want us to add stuff to ppapi/native_client or dependencies on it. Can we move the stuff in ppruntime? Perhaps to the new ppapi/nacl_irt directory? https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:30: PluginMainThread(uintptr_t entry_point) nit: explicit https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:51: uintptr_t entry_point_; It seems like it would make more sense to store this as the function pointer type, since that's the only way you use it. For that matter, it should be passed to the constructor that way. Might also be worth having a typedef for void(*)(uintptr_t*). https://codereview.chromium.org/140573003/diff/530001/ppapi/native_client/src... File ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h (right): https://codereview.chromium.org/140573003/diff/530001/ppapi/native_client/src... ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h:34: namespace proxy { This feels weird to reference ppapi::proxy at all here... this should use a different namespace, I think. Would it make sense to instead declare these somewhere in ppapi/nacl_irt? I'm not clear yet on what that directory is for. https://codereview.chromium.org/140573003/diff/530001/ppapi/proxy/plugin_main.cc File ppapi/proxy/plugin_main.cc (right): https://codereview.chromium.org/140573003/diff/530001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main.cc:18: # error "non-SFI mode is currently supported only on Linux." Why are we changing the name of this file? Are we saying BMM is *not* NaCl? In Pepper, we also have trusted mode, so this has the potential to get confused with that. I wasn't sure if we still consider BMM "NaCl" even though it's a different toolchain and not SFI. If we eventually support PNaCl on BMM, it will still be "PNaCl" as far as external developers are concerned. If we move this file to nacl_irt, then the new name is probably fine. If we leave it here, maybe plugin_main_irt.cc would be clearer? https://codereview.chromium.org/140573003/diff/530001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main.cc:91: void SetIPCFileDescriptors(int ipc_browser_fd, int ipc_renderer_fd) { This and WaitForPpapiStartEvent could maybe go in a different namespace; they're really implementing hooks for the IRT to talk to, and I don't think of it as part of the proxy. (You might make that argument about this whole file, really... it doesn't need to happen for this CL, but it might make sense for us to move this file to nacl_irt).
By the way, please add one of us (dmichael or teravest) at the time you publish the CL if it modifies something we own (like ppapi/). We'll need to do an OWNERS review eventually anwyway, but I would like us to see these sooner rather than later just so we can keep up with your work better and not collide. It might also save you time. Thank you!
https://codereview.chromium.org/140573003/diff/530001/ppapi/native_client/src... File ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h (right): https://codereview.chromium.org/140573003/diff/530001/ppapi/native_client/src... ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h:34: namespace proxy { On 2014/02/13 19:00:34, dmichael wrote: > This feels weird to reference ppapi::proxy at all here... this should use a > different namespace, I think. > > Would it make sense to instead declare these somewhere in ppapi/nacl_irt? I'm > not clear yet on what that directory is for. I intended ppapi/nacl_irt/ to be used just for public headers that define NaCl's stable ABI, not for anything else. So it's similar to ppapi/c/.
[On the AddFilter() question:] On 13 February 2014 02:18, <hidehiko@chromium.org> wrote: > Questions: > - Is this considered a bug in your team? (depending on the team, this kind > of > case may or may not be a bug...) > - What's the recommendation for the next action? > 1) Just remove the sync code in this CL (as currently no visible issue is > found). Optionally file a bug (or fix it) separately later. > 2) Remove the sync code in this CL, and fix the initialization code at > the > same time. > 3) Keep the sync code for the protection. And remove it after fixing the > issue. > 4) Something else. > WDYT? I've filed: http://code.google.com/p/chromium/issues/detail?id=343768 I'd prefer option (1), because I'd prefer to see the Non-SFI Mode and SFI Mode code paths use the same approach. I suspect that blocking load_module()'s return on ppapi_start() being called will cause problems later with open_resource(), anyway. It's legitimate for user code to be doing things like calling open_resource() before ppapi_start(), and it's better for the trusted plugin not to be blocked during that time. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi, thank you for your review! Dave, I'll ask you to review future CLs, too. Thank you for help! Mark, thank you for filing a bug about the race condition. I just removed sync-related code from this CL. Could you take another look? Thanks, - hidehiko https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:22: #include "ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h" On 2014/02/13 19:00:34, dmichael wrote: > I don't really want us to add stuff to ppapi/native_client or dependencies on > it. > > Can we move the stuff in ppruntime? Perhaps to the new ppapi/nacl_irt directory? Moved to ppapi/proxy with renaming it to plugin_main_irt.h. See also my reply to the plugin_main.cc. https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:30: PluginMainThread(uintptr_t entry_point) On 2014/02/13 19:00:34, dmichael wrote: > nit: explicit Done. https://codereview.chromium.org/140573003/diff/530001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:51: uintptr_t entry_point_; On 2014/02/13 19:00:34, dmichael wrote: > It seems like it would make more sense to store this as the function pointer > type, since that's the only way you use it. For that matter, it should be passed > to the constructor that way. > > Might also be worth having a typedef for void(*)(uintptr_t*). Done. https://codereview.chromium.org/140573003/diff/530001/ppapi/native_client/src... File ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h (right): https://codereview.chromium.org/140573003/diff/530001/ppapi/native_client/src... ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h:34: namespace proxy { On 2014/02/14 01:55:36, Mark Seaborn wrote: > On 2014/02/13 19:00:34, dmichael wrote: > > This feels weird to reference ppapi::proxy at all here... this should use a > > different namespace, I think. > > > > Would it make sense to instead declare these somewhere in ppapi/nacl_irt? I'm > > not clear yet on what that directory is for. > > I intended ppapi/nacl_irt/ to be used just for public headers that define NaCl's > stable ABI, not for anything else. So it's similar to ppapi/c/. Reverted this file. See also my reply to plugin_main.cc https://codereview.chromium.org/140573003/diff/530001/ppapi/proxy/plugin_main.cc File ppapi/proxy/plugin_main.cc (right): https://codereview.chromium.org/140573003/diff/530001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main.cc:18: # error "non-SFI mode is currently supported only on Linux." On 2014/02/13 19:00:34, dmichael wrote: > Why are we changing the name of this file? Are we saying BMM is *not* NaCl? In > Pepper, we also have trusted mode, so this has the potential to get confused > with that. I wasn't sure if we still consider BMM "NaCl" even though it's a > different toolchain and not SFI. If we eventually support PNaCl on BMM, it will > still be "PNaCl" as far as external developers are concerned. > > > If we move this file to nacl_irt, then the new name is probably fine. If we > leave it here, maybe plugin_main_irt.cc would be clearer? The main reason why I changed the file name is that files _nacl.cc suffix is automatically removed from sources! list for non-untrusted libs. For BMM, it still is a NaCl, but the code needs to be linked with non-untrusted code. I just renamed the file to plugin_main_irt.cc, because according to Mark, ppapi/nacl_irt is not the appropriate place to put this file. Also, I created its header file to refer functions from nonsfi_main.cc by refactoring the code from ppruntime.h. Also note that I just kept ppruntime.h as is (reverted the change). It is because that the file is referred from the native_client/src/untrusted/irt/irt_ppapi.c, so it seems needs to be kept as is for now. (I.e., to clean it up, we need to change chrome code, NaCl code, NaCl roll, and then we can clean the files as same as other files, IIUC). https://codereview.chromium.org/140573003/diff/530001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main.cc:91: void SetIPCFileDescriptors(int ipc_browser_fd, int ipc_renderer_fd) { On 2014/02/13 19:00:34, dmichael wrote: > This and WaitForPpapiStartEvent could maybe go in a different namespace; they're > really implementing hooks for the IRT to talk to, and I don't think of it as > part of the proxy. (You might make that argument about this whole file, > really... it doesn't need to happen for this CL, but it might make sense for us > to move this file to nacl_irt). In the new file, I just moved this to the global function as similar to other exposed functions.
ppapi lgtm, thanks!
https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:287: // the plugin side, because the IPC::Listener needs to be live on the Did you mean "needs to live"? https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:288: // plugin's main thread. However, on the initialization (i.e. before "on initialization" https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:289: // loading the plugin binary), the FD is needed to be passed to the "FD needs to be passed" https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:305: nacl::nonsfi::SetIPCFileDescriptors( Can this just call the function from ppapi/proxy/plugin_main_irt.h, please, to remove the level of wrapping for SetIPCFileDescriptors() that you have? https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:319: if (!Send(new NaClProcessHostMsg_PpapiChannelsCreated( This call is the same in both paths, right? So it could be de-duplicated. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nonsfi/DEPS (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/DEPS:2: "+ppapi/nacl_irt/irt_ppapi.h", You can put "+ppapi/nacl_irt" since the whole directory is meant to be public headers https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/DEPS:3: "+ppapi/proxy/plugin_main_irt.h", Similarly, "+ppapi/proxy". Normally specific headers are listed when the dependency is something that should be cleaned up later. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_ppapi.cc (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:20: // TODO(hidehiko): Register this thread as main. (g_is_main_thread = 1.) g_is_main_thread only changes the behaviour of open_resource(), so either mention that, or omit the comment, otherwise it's rather cryptic. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:46: nacl::nonsfi::g_pp_functions.PPP_GetInterface(interface_name); Nit: Why not just "return nacl::nonsfi::...;"? https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:86: // We may want to manage the thread pointer after we switch the Do you mean the SimpleThread pointer? https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:90: ANNOTATE_LEAKING_OBJECT_PTR(thread); Maybe use base::PlatformThread::CreateNonJoinable()? It has less baggage: it has no need for a separate Start() or for declaring a leak. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:93: if (done_cls) I don't think you need a NULL check here https://codereview.chromium.org/140573003/diff/730001/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/140573003/diff/730001/ipc/ipc_channel.h#newco... ipc/ipc_channel.h:221: // On POSIX an IPC::Channel wraps a socketpair() with set some attributes "with some attributes set" However, since this is documenting the interface it probably shouldn't specify implementation details. So I'd just say "SocketPair() creates a pair of socket FDs suitable for using with IPC::Channel". https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... File ppapi/proxy/plugin_main_irt.cc (right): https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.cc:18: # define NACL_LINUX 1 This is rather hacky... Which headers do you find need this? Can you put them inside "#if defined(__native_client__)"? https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.cc:37: #include "ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h" You can remove this now that you #include "ppapi/proxy/plugin_main_irt.h" instead (following the distinction between user code and PPAPI proxy code I mentioned elsewhere). https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.cc:341: void SetIPCFileDescriptors(int ipc_browser_fd, int ipc_renderer_fd) { Nit: maybe put this above PpapiPluginRegisterThreadCreator() to reflect the order of use? https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... File ppapi/proxy/plugin_main_irt.h (right): https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.h:4: I see this is partly duplicating the contents of ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h. To make this less confusing, can you also remove the declaration of PpapiPluginRegisterThreadCreator() from ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h, please? [The confusion arises because both user code and the PPAPI proxy have a function called "PpapiPluginMain()". This is left over from 2011 when the proxy could be linked into user nexes. However, the two *could* now use different names.]
Thank you for your review! PTAL? Note that, in order to fix the compile errors on linux_chromeos and linux_clang, I edited .gypi files. Could you double check them, too? Thanks, - hidehiko https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:287: // the plugin side, because the IPC::Listener needs to be live on the On 2014/02/15 03:00:52, Mark Seaborn wrote: > Did you mean "needs to live"? Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:288: // plugin's main thread. However, on the initialization (i.e. before On 2014/02/15 03:00:52, Mark Seaborn wrote: > "on initialization" Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:289: // loading the plugin binary), the FD is needed to be passed to the On 2014/02/15 03:00:52, Mark Seaborn wrote: > "FD needs to be passed" Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:305: nacl::nonsfi::SetIPCFileDescriptors( On 2014/02/15 03:00:52, Mark Seaborn wrote: > Can this just call the function from ppapi/proxy/plugin_main_irt.h, please, to > remove the level of wrapping for SetIPCFileDescriptors() that you have? Done (but I just worried about if it is layer violation...?) https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:319: if (!Send(new NaClProcessHostMsg_PpapiChannelsCreated( On 2014/02/15 03:00:52, Mark Seaborn wrote: > This call is the same in both paths, right? So it could be de-duplicated. Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nonsfi/DEPS (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/DEPS:2: "+ppapi/nacl_irt/irt_ppapi.h", On 2014/02/15 03:00:52, Mark Seaborn wrote: > You can put "+ppapi/nacl_irt" since the whole directory is meant to be public > headers Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/DEPS:3: "+ppapi/proxy/plugin_main_irt.h", On 2014/02/15 03:00:52, Mark Seaborn wrote: > Similarly, "+ppapi/proxy". Normally specific headers are listed when the > dependency is something that should be cleaned up later. Moved to components/nacl/loader/DEPS. I have kept it as is to follow the manner around there. Please let me know if I should use the directory path even in that file. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nonsfi/irt_ppapi.cc (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:20: // TODO(hidehiko): Register this thread as main. (g_is_main_thread = 1.) On 2014/02/15 03:00:52, Mark Seaborn wrote: > g_is_main_thread only changes the behaviour of open_resource(), so either > mention that, or omit the comment, otherwise it's rather cryptic. Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/irt_ppapi.cc:46: nacl::nonsfi::g_pp_functions.PPP_GetInterface(interface_name); On 2014/02/15 03:00:52, Mark Seaborn wrote: > Nit: Why not just "return nacl::nonsfi::...;"? Oops, I forgot to fix the style... Done. (I printf-debugged the returned pointer...) https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:86: // We may want to manage the thread pointer after we switch the On 2014/02/15 03:00:52, Mark Seaborn wrote: > Do you mean the SimpleThread pointer? Acknowledged. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:90: ANNOTATE_LEAKING_OBJECT_PTR(thread); On 2014/02/15 03:00:52, Mark Seaborn wrote: > Maybe use base::PlatformThread::CreateNonJoinable()? It has less baggage: it > has no need for a separate Start() or for declaring a leak. Great to know. Done. https://codereview.chromium.org/140573003/diff/730001/components/nacl/loader/... components/nacl/loader/nonsfi/nonsfi_main.cc:93: if (done_cls) On 2014/02/15 03:00:52, Mark Seaborn wrote: > I don't think you need a NULL check here Done. https://codereview.chromium.org/140573003/diff/730001/ipc/ipc_channel.h File ipc/ipc_channel.h (right): https://codereview.chromium.org/140573003/diff/730001/ipc/ipc_channel.h#newco... ipc/ipc_channel.h:221: // On POSIX an IPC::Channel wraps a socketpair() with set some attributes On 2014/02/15 03:00:52, Mark Seaborn wrote: > "with some attributes set" > > However, since this is documenting the interface it probably shouldn't specify > implementation details. So I'd just say "SocketPair() creates a pair of socket > FDs suitable for using with IPC::Channel". Done. https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... File ppapi/proxy/plugin_main_irt.cc (right): https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.cc:18: # define NACL_LINUX 1 On 2014/02/15 03:00:52, Mark Seaborn wrote: > This is rather hacky... Which headers do you find need this? Can you put them > inside "#if defined(__native_client__)"? nacl_compiler_annotations.h, which is indirectly included by chrome_main.h. Fixed. https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.cc:37: #include "ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h" On 2014/02/15 03:00:52, Mark Seaborn wrote: > You can remove this now that you #include "ppapi/proxy/plugin_main_irt.h" > instead (following the distinction between user code and PPAPI proxy code I > mentioned elsewhere). Done. https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.cc:341: void SetIPCFileDescriptors(int ipc_browser_fd, int ipc_renderer_fd) { On 2014/02/15 03:00:52, Mark Seaborn wrote: > Nit: maybe put this above PpapiPluginRegisterThreadCreator() to reflect the > order of use? Done. https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... File ppapi/proxy/plugin_main_irt.h (right): https://codereview.chromium.org/140573003/diff/730001/ppapi/proxy/plugin_main... ppapi/proxy/plugin_main_irt.h:4: On 2014/02/15 03:00:52, Mark Seaborn wrote: > I see this is partly duplicating the contents of > ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h. > > To make this less confusing, can you also remove the declaration of > PpapiPluginRegisterThreadCreator() from > ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h, please? > > [The confusion arises because both user code and the PPAPI proxy have a function > called "PpapiPluginMain()". This is left over from 2011 when the proxy could be > linked into user nexes. However, the two *could* now use different names.] Done. FYI: I guess we can remove the ppruntime.h file (as irt_ppapi.c has been moved to ppapi/proxy/...), but it needs to change some more files which are conceptually unrelated to this work. So, I think it should be addressed separately?
Can you measure the size increase in nacl_helper this change introduces, please, and put the numbers in the commit message? You should notify the perf sheriff that there will be a size increase just before committing this. Then LGTM. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/DEPS (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/DEPS:25: # For setting FDs on non-SFI mode. "on" -> "in"? https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:290: // In non-SFI mode, we neither intercept nor rewrite the message by "by" -> "using" https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:291: // IPCAdapter, and the channels are connected between the plugin and "IPCAdapter" -> "NaClIPCAdapter" for clarity https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:296: // we create raw FD pairs, and pass the clients side FDs to the hosts, "client side" https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:310: // Set the plugin IPC channel FD. "FDs" https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:321: // Create the PPAPI IPC channels between the NaCl IRT and the hosts "host" (since this is adjectival) https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:328: Nit: remove empty line to match start of block https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nonsfi/irt_ppapi.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nonsfi/irt_ppapi.cc:6: Nit: remove empty line (base/ should be in same #include block) https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nonsfi/nonsfi_main.cc:27: typedef void (*EntryPointType)(uintptr_t *); Nit: "uintptr_t*" (to follow Chrome spacing style) https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nonsfi/nonsfi_main.cc:60: const size_t kStackSize = 1024 * 1024; NaCl uses 16MB for the initial stack size (see NACL_DEFAULT_STACK_MAX), so it would be good to use the same number here. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.cc (left): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:241: // From here, the dispatcher will manage its own lifetime according to the Why remove this? https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.cc (right): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:62: // On SFI mode, the FDs of IPC channels are NACL_CHROME_DESC_BASE and its "On" -> "In" https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:67: // On non-SFI mode, the FDs of IPC channels are different from the hard coded "On" -> "In" https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:69: // The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the This reference to NACL_CHROME_DESC_BASE is incorrect now https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:126: // The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the Please remove this comment now since the reference to NACL_CHROME_DESC_BASE is outdated https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:253: // The second FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the Same for this sentence https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:297: // from the hard-coded numbers. So, this overwrites the values directly. With the "#if defined(__native_client__)" above, this isn't overwriting hard-coded numbers. You could just remove this comment since it's pretty self-explanatory. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.h (right): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:9: #include "ppapi/proxy/ppapi_proxy_export.h" You shouldn't have PPAPI_PROXY_EXPORT here. That adds __attribute__((visibility("default"))), which is something we don't really want on Linux (it would override "-fvisibility=hidden" which most Chrome code is compiled with), and I think it won't affect code linked into the NaCl IRT. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:21: // Overwrites the IPC channels for the browser and the renderer by the given "Overwrites" -> "Sets"? https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:22: // FD #s. This will be used for non-SFI mode. Must be called before Nit: '#s' -> 'numbers'. This will be read more than it is written. :-)
https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.h (right): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:9: #include "ppapi/proxy/ppapi_proxy_export.h" On 2014/02/19 17:00:30, Mark Seaborn wrote: > You shouldn't have PPAPI_PROXY_EXPORT here. > > That adds __attribute__((visibility("default"))), which is something we > don't really want on Linux (it would override "-fvisibility=hidden" > which most Chrome code is compiled with), and I think it won't affect > code linked into the NaCl IRT. Oh, was PPAPI_PROXY_EXPORT just needed to make the components build work? In which case, it's OK as it is.
+jam: Can you do an OWNERS review for the change in ipc/ please?
jam is on paternity leave. cpu@, are you a good reviewer for this ipc change?
Exposing SocketPair() from ipc/ lgtm. One possible change would be to add more opaque encapsulation for the file descriptors of the future IPC::Channel, but it's probably ok as-is. I would love to see a better commit message though. Please, explain what's happening better. It's a complicated change in an already complicated area. Please, make things clear in terms of: 1. how it worked before this CL. 2. How things work after this CL. Both for existing SFI and for the new non-SFI stuff. The comment at components/nacl/loader/nacl_listener.cc should probably be partly in the commit message. If cpu@ is not available to approve the small ipc/ change, tsepez@ could be a good reviewer as well. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:290: // In non-SFI mode, we neither intercept nor rewrite the message by This is key to what's happening in this CL. Could you make sure it's also exposed in the commit message?
Not comfortable exposing that function there. Removed myself and added brettw for IPC review.
Thank you for reviews! PTAL? Thank you for alternate reviewer suggestion, cpu@. But Brett seems not an OWNER of ipc/ (is my understanding correct?). So, Tom, could you kindly review ipc/... as an OWNER (suggested by Julien. Thanks!). Julien, just FYI. Note that this CL shouldn't conceptually change the SFI mode code. I updated the commit message to so. Mark, could you take another look just in case? Thanks, - hidehiko https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/DEPS (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/DEPS:25: # For setting FDs on non-SFI mode. On 2014/02/19 17:00:30, Mark Seaborn wrote: > "on" -> "in"? Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:290: // In non-SFI mode, we neither intercept nor rewrite the message by On 2014/02/19 17:00:30, Mark Seaborn wrote: > "by" -> "using" Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:290: // In non-SFI mode, we neither intercept nor rewrite the message by On 2014/02/19 22:40:09, jln wrote: > This is key to what's happening in this CL. Could you make sure it's also > exposed in the commit message? Updated the commit message. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:291: // IPCAdapter, and the channels are connected between the plugin and On 2014/02/19 17:00:30, Mark Seaborn wrote: > "IPCAdapter" -> "NaClIPCAdapter" for clarity Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:296: // we create raw FD pairs, and pass the clients side FDs to the hosts, On 2014/02/19 17:00:30, Mark Seaborn wrote: > "client side" Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:310: // Set the plugin IPC channel FD. On 2014/02/19 17:00:30, Mark Seaborn wrote: > "FDs" Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:321: // Create the PPAPI IPC channels between the NaCl IRT and the hosts On 2014/02/19 17:00:30, Mark Seaborn wrote: > "host" (since this is adjectival) Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nacl_listener.cc:328: On 2014/02/19 17:00:30, Mark Seaborn wrote: > Nit: remove empty line to match start of block Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nonsfi/irt_ppapi.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nonsfi/irt_ppapi.cc:6: On 2014/02/19 17:00:30, Mark Seaborn wrote: > Nit: remove empty line (base/ should be in same #include block) Oops. Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... File components/nacl/loader/nonsfi/nonsfi_main.cc (right): https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nonsfi/nonsfi_main.cc:27: typedef void (*EntryPointType)(uintptr_t *); On 2014/02/19 17:00:30, Mark Seaborn wrote: > Nit: "uintptr_t*" (to follow Chrome spacing style) Done. https://codereview.chromium.org/140573003/diff/1210015/components/nacl/loader... components/nacl/loader/nonsfi/nonsfi_main.cc:60: const size_t kStackSize = 1024 * 1024; On 2014/02/19 17:00:30, Mark Seaborn wrote: > NaCl uses 16MB for the initial stack size (see NACL_DEFAULT_STACK_MAX), so it > would be good to use the same number here. Good to know. Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.cc (left): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:241: // From here, the dispatcher will manage its own lifetime according to the On 2014/02/19 17:00:30, Mark Seaborn wrote: > Why remove this? Oops.. I somehow accidentally removed this. Reverted. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.cc (right): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:62: // On SFI mode, the FDs of IPC channels are NACL_CHROME_DESC_BASE and its On 2014/02/19 17:00:30, Mark Seaborn wrote: > "On" -> "In" Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:67: // On non-SFI mode, the FDs of IPC channels are different from the hard coded On 2014/02/19 17:00:30, Mark Seaborn wrote: > "On" -> "In" Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:69: // The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the On 2014/02/19 17:00:30, Mark Seaborn wrote: > This reference to NACL_CHROME_DESC_BASE is incorrect now oops, fixed. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:126: // The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the On 2014/02/19 17:00:30, Mark Seaborn wrote: > Please remove this comment now since the reference to NACL_CHROME_DESC_BASE is > outdated Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:253: // The second FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the On 2014/02/19 17:00:30, Mark Seaborn wrote: > Same for this sentence Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.cc:297: // from the hard-coded numbers. So, this overwrites the values directly. On 2014/02/19 17:00:30, Mark Seaborn wrote: > With the "#if defined(__native_client__)" above, this isn't overwriting > hard-coded numbers. You could just remove this comment since it's pretty > self-explanatory. Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... File ppapi/proxy/plugin_main_irt.h (right): https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:9: #include "ppapi/proxy/ppapi_proxy_export.h" On 2014/02/19 17:07:54, Mark Seaborn wrote: > On 2014/02/19 17:00:30, Mark Seaborn wrote: > > You shouldn't have PPAPI_PROXY_EXPORT here. > > > > That adds __attribute__((visibility("default"))), which is something we > > don't really want on Linux (it would override "-fvisibility=hidden" > > which most Chrome code is compiled with), and I think it won't affect > > code linked into the NaCl IRT. > > Oh, was PPAPI_PROXY_EXPORT just needed to make the components build work? In > which case, it's OK as it is. Exactly. Without this component build would be failed. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:21: // Overwrites the IPC channels for the browser and the renderer by the given On 2014/02/19 17:00:30, Mark Seaborn wrote: > "Overwrites" -> "Sets"? Done. https://codereview.chromium.org/140573003/diff/1210015/ppapi/proxy/plugin_mai... ppapi/proxy/plugin_main_irt.h:22: // FD #s. This will be used for non-SFI mode. Must be called before On 2014/02/19 17:00:30, Mark Seaborn wrote: > Nit: '#s' -> 'numbers'. This will be read more than it is written. :-) Done :-)
+jam@ for layering on the IPC side.
On 20 February 2014 10:56, <tsepez@chromium.org> wrote: > +jam@ for layering on the IPC side. As stated earlier in the thread, jam is on paternity leave. Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/20 19:00:01, Mark Seaborn wrote: > On 20 February 2014 10:56, <mailto:tsepez@chromium.org> wrote: > > > +jam@ for layering on the IPC side. > > > As stated earlier in the thread, jam is on paternity leave. > > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Ok. I'm ok with exposing the SocketPair() call from ipc. LGTM.
Again, thank you for reviews! Sending this to CQ. - hidehiko On 2014/02/20 19:02:25, Tom Sepez wrote: > On 2014/02/20 19:00:01, Mark Seaborn wrote: > > On 20 February 2014 10:56, <mailto:tsepez@chromium.org> wrote: > > > > > +jam@ for layering on the IPC side. > > > > > > As stated earlier in the thread, jam is on paternity leave. > > > > Mark > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org. > > Ok. I'm ok with exposing the SocketPair() call from ipc. LGTM.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/140573003/1480001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ppapi/proxy/plugin_main_irt.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
A ppapi/proxy/plugin_main_irt.cc
Copied ppapi/proxy/plugin_main_nacl.cc -> ppapi/proxy/plugin_main_irt.cc
patching file ppapi/proxy/plugin_main_irt.cc
Hunk #6 FAILED at 130.
Hunk #7 succeeded at 266 (offset 1 line).
Hunk #8 succeeded at 308 (offset 1 line).
Hunk #9 succeeded at 351 (offset 1 line).
1 out of 9 hunks FAILED -- saving rejects to file
ppapi/proxy/plugin_main_irt.cc.rej
Patch: NR ppapi/proxy/plugin_main_nacl.cc->ppapi/proxy/plugin_main_irt.cc
Index: ppapi/proxy/plugin_main_irt.cc
diff --git a/ppapi/proxy/plugin_main_nacl.cc b/ppapi/proxy/plugin_main_irt.cc
similarity index 87%
rename from ppapi/proxy/plugin_main_nacl.cc
rename to ppapi/proxy/plugin_main_irt.cc
index
852ff4e4a7f05250faf067d83e4c7003c472a371..5d9760d1411480b4c0de70d0d3c52226f53db721
100644
--- a/ppapi/proxy/plugin_main_nacl.cc
+++ b/ppapi/proxy/plugin_main_irt.cc
@@ -2,8 +2,9 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-#include <unistd.h>
+#include "ppapi/proxy/plugin_main_irt.h"
+#include <unistd.h>
#include <map>
#include <set>
@@ -13,6 +14,7 @@
// IPC_MESSAGE_MACROS_LOG_ENABLED so ppapi_messages.h will generate the
// ViewMsgLog et al. functions.
+#include "base/at_exit.h"
#include "base/command_line.h"
#include "base/memory/scoped_ptr.h"
#include "base/message_loop/message_loop.h"
@@ -23,11 +25,8 @@
#include "ipc/ipc_channel_handle.h"
#include "ipc/ipc_logging.h"
#include "ipc/ipc_message.h"
-#include "native_client/src/public/chrome_main.h"
-#include "native_client/src/shared/srpc/nacl_srpc.h"
#include "ppapi/c/ppp.h"
#include "ppapi/c/ppp_instance.h"
-#include "ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h"
#include "ppapi/proxy/plugin_dispatcher.h"
#include "ppapi/proxy/plugin_globals.h"
#include "ppapi/proxy/plugin_message_filter.h"
@@ -36,6 +35,11 @@
#include "ppapi/shared_impl/ppapi_switches.h"
#include "ppapi/shared_impl/ppb_audio_shared.h"
+#if defined(__native_client__)
+#include "native_client/src/public/chrome_main.h"
+#include "native_client/src/shared/srpc/nacl_srpc.h"
+#endif
+
#if defined(IPC_MESSAGE_LOG_ENABLED)
#include "base/containers/hash_tables.h"
@@ -56,6 +60,19 @@ using ppapi::proxy::SerializedHandle;
namespace {
+#if defined(__native_client__)
+// In SFI mode, the FDs of IPC channels are NACL_CHROME_DESC_BASE and its
+// successor, which is set in nacl_listener.cc.
+int g_nacl_ipc_browser_fd = NACL_CHROME_DESC_BASE;
+int g_nacl_ipc_renderer_fd = NACL_CHROME_DESC_BASE + 1;
+#else
+// In non-SFI mode, the FDs of IPC channels are different from the hard coded
+// ones. These values will be set by SetIPCFileDescriptors() below.
+// At first, both are initialized to invalid FD number (-1).
+int g_nacl_ipc_browser_fd = -1;
+int g_nacl_ipc_renderer_fd = -1;
+#endif
+
// This class manages communication between the plugin and the browser, and
// manages the PluginDispatcher instances for communication between the plugin
// and the renderer.
@@ -113,10 +130,10 @@
PpapiDispatcher::PpapiDispatcher(scoped_refptr<base::MessageLoopProxy> io_loop)
: next_plugin_dispatcher_id_(0),
message_loop_(io_loop),
shutdown_event_(true, false) {
- // The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the
- // browser.
+ DCHECK_NE(g_nacl_ipc_browser_fd, -1)
+ << "g_nacl_ipc_browser_fd must be initialized before the plugin starts";
IPC::ChannelHandle channel_handle(
- "NaCl IPC", base::FileDescriptor(NACL_CHROME_DESC_BASE, false));
+ "NaCl IPC", base::FileDescriptor(g_nacl_ipc_browser_fd, false));
// We don't have/need a PID since handle sharing happens outside of the
// NaCl sandbox.
channel_.reset(new IPC::SyncChannel(
@@ -248,10 +265,10 @@ void PpapiDispatcher::OnMsgInitializeNaClDispatcher(
new PluginDispatcher(::PPP_GetInterface, args.permissions,
args.off_the_record);
// The channel handle's true name is not revealed here.
- // The second FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the
- // renderer.
+ DCHECK_NE(g_nacl_ipc_renderer_fd, -1)
+ << "g_nacl_ipc_renderer_fd must be initialized before the plugin starts";
IPC::ChannelHandle channel_handle(
- "nacl", base::FileDescriptor(NACL_CHROME_DESC_BASE + 1, false));
+ "nacl", base::FileDescriptor(g_nacl_ipc_renderer_fd, false));
if (!dispatcher->InitPluginWithChannel(this, base::kNullProcessId,
channel_handle, false)) {
delete dispatcher;
@@ -290,24 +307,41 @@ void
PpapiDispatcher::SetPpapiKeepAliveThrottleFromCommandLine() {
} // namespace
+void SetIPCFileDescriptors(int ipc_browser_fd, int ipc_renderer_fd) {
+ g_nacl_ipc_browser_fd = ipc_browser_fd;
+ g_nacl_ipc_renderer_fd = ipc_renderer_fd;
+}
+
void PpapiPluginRegisterThreadCreator(
const struct PP_ThreadFunctions* thread_functions) {
+#if defined(__native_client__)
+ // TODO(hidehiko): The thread creation for the PPB_Audio is not yet
+ // implemented on non-SFI mode. Support this. Now, this function invocation
+ // is just ignored.
+
// Initialize all classes that need to create threads that call back into
// user code.
ppapi::PPB_Audio_Shared::SetThreadFunctions(thread_functions);
+#endif
}
int PpapiPluginMain() {
// Though it isn't referenced here, we must instantiate an AtExitManager.
base::AtExitManager exit_manager;
base::MessageLoop loop;
+#if defined(IPC_MESSAGE_LOG_ENABLED)
IPC::Logging::set_log_function_map(&g_log_function_mapping);
+#endif
ppapi::proxy::PluginGlobals plugin_globals;
base::Thread io_thread("Chrome_NaClIOThread");
base::Thread::Options options;
options.message_loop_type = base::MessageLoop::TYPE_IO;
io_thread.StartWithOptions(options);
+#if defined(__native_client__)
+ // Currently on non-SFI mode, we don't use SRPC server on plugin.
+ // TODO(hidehiko): Make sure this SRPC is actually used on SFI-mode.
+
// Start up the SRPC server on another thread. Otherwise, when it blocks
// on an RPC, the PPAPI proxy will hang. Do this before we initialize the
// module and start the PPAPI proxy so that the NaCl plugin can continue
@@ -316,6 +350,7 @@ int PpapiPluginMain() {
if (!NaClSrpcAcceptClientOnThread(srpc_methods)) {
return 1;
}
+#endif
PpapiDispatcher ppapi_dispatcher(io_thread.message_loop_proxy());
plugin_globals.set_plugin_proxy_delegate(&ppapi_dispatcher);
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/140573003/1600001
The CQ bit was unchecked by mseaborn@chromium.org
I unticked the commit box because I think you missed my earlier request: On 2014/02/19 17:00:29, Mark Seaborn wrote: > Can you measure the size increase in nacl_helper this change introduces, > please, and put the numbers in the commit message? You should notify the > perf sheriff that there will be a size increase just before committing this. I don't want this change to get reverted because of a size increase...
CC'ing rmcilroy (perf sheriff for Thursday/Friday): This change is likely to increase the size of the nacl_helper executable, so please don't revert this change if the perf results show a size increase -- it is expected. On 2014/02/20 20:25:24, Mark Seaborn wrote: > I unticked the commit box because I think you missed my earlier request: > > On 2014/02/19 17:00:29, Mark Seaborn wrote: > > Can you measure the size increase in nacl_helper this change introduces, > > please, and put the numbers in the commit message? You should notify the > > perf sheriff that there will be a size increase just before committing this. > > I don't want this change to get reverted because of a size increase...
I checked the size increase on x86-64 Linux: Before: out/Release/nacl_helper is 2539384 bytes (unstripped) $ size out/Release/nacl_helper text data bss dec hex filename 2063578 27910 213872 2305360 232d50 out/Release/nacl_helper After: out/Release/nacl_helper is 8602408 bytes (unstripped) $ size out/Release/nacl_helper text data bss dec hex filename 6290651 233984 247984 6772619 67578b out/Release/nacl_helper So this will increase the stripped size (text+data) of nacl_helper by about 4.1MB.
Sorry for my mistake, and thank you, Mark, for catching this. I measured on ia32 and x64 and I updated the commit message. Sending to CQ, again. - hidehiko On 2014/02/21 02:08:12, Mark Seaborn wrote: > I checked the size increase on x86-64 Linux: > > Before: > out/Release/nacl_helper is 2539384 bytes (unstripped) > $ size out/Release/nacl_helper > text data bss dec hex filename > 2063578 27910 213872 2305360 232d50 out/Release/nacl_helper > > After: > out/Release/nacl_helper is 8602408 bytes (unstripped) > $ size out/Release/nacl_helper > text data bss dec hex filename > 6290651 233984 247984 6772619 67578b out/Release/nacl_helper > > So this will increase the stripped size (text+data) of nacl_helper by about > 4.1MB.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/140573003/1600001
Message was sent while issue was closed.
Change committed as 252503
Message was sent while issue was closed.
On 2014/02/21 05:35:26, I haz the power (commit-bot) wrote: > Change committed as 252503 compilation was broken. The patch has been reverted.
Message was sent while issue was closed.
On 2014/02/21 06:27:45, loislo wrote: > On 2014/02/21 05:35:26, I haz the power (commit-bot) wrote: > > Change committed as 252503 > > compilation was broken. The patch has been reverted. http://build.chromium.org/p/chromium.webkit/builders/Linux%20Aura/builds/1668...
It seems this breaks the linux build due to the implicit confliction with submitted CLs. (Not sure why this passed the CQ check, though). And, unfortunately, it was reverted while the trivial fix tried to being committed (crrev.com/169863003). The revert CL seem to have no review, so please let me note the reason here. r252185 moves nacl_irt_ppapihook structure. r12790 is NaCl side CL, which is rolled by r252376. r252503 (this submission) causes the build failure, which couldn't catch the move. Reopened this, merged the fix (which is LGTM'ed by Mark), and sending it to CQ, again.
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/140573003/1860001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by hidehiko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hidehiko@chromium.org/140573003/1860001
Message was sent while issue was closed.
Change committed as 252556 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
