Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
332463003:
Pepper: Remove LOAD_MODULE SRPC call in SFI mode. (Closed)
|
Created:
6 years, 6 months ago by teravest Modified:
6 years, 5 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, tzik, binji+watch_chromium.org, raymes+watch_chromium.org, teravest+watch_chromium.org, nfullagar1, piman+watch_chromium.org, noelallen1, ihf+watch_chromium.org, hidehiko, jvoung (off chromium) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPepper: Remove LOAD_MODULE SRPC call in SFI mode.
This change puts nexe information in the parameters for starting a NaCl
plugin instead of sending that information over SRPC. This may remove the
need for that IPC round trip entirely; perhaps we could report load failure
as part of the RPCs for starting sel_ldr or performing StartModule().
nacl_defines.gypi as added as a dependency in components/components_tests.gyp
because nacl_process_host.h now includes
"native_client/src/public/nacl_file_info.h", which requires nacl_defines.
BUG=333950
R=bbudge@chromium.org, brettw@chromium.org, hidehiko@chromium.org, jschuh@chromium.org, mseaborn@chromium.org, sky@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285028
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=285418
Patch Set 1 : rebased #
Total comments: 46
Patch Set 2 : fixes for bbudge #
Total comments: 4
Patch Set 3 : fixes for mseaborn #
Total comments: 8
Patch Set 4 : build fix #Patch Set 5 : Rebased #Patch Set 6 : Fixes for jvoung and hidehiko #
Total comments: 13
Patch Set 7 : rebased #Patch Set 8 : fixes for mseaborn #
Total comments: 6
Patch Set 9 : fix leaked file descriptors #
Total comments: 1
Patch Set 10 : fix fd management, again #
Total comments: 4
Patch Set 11 : Remove stale comment and use TakeFileHandleForProcess #Patch Set 12 : rebased #
Total comments: 1
Patch Set 13 : Remove ifdef for taking file handle #Patch Set 14 : Update BUILD.gn after NaCl deps roll #Patch Set 15 : fix for disable_nacl=1 #Patch Set 16 : #
Total comments: 1
Patch Set 17 : add comment to forward declaration #Messages
Total messages: 66 (0 generated)
I still have to track down the FATAL log messages in scoped_handle.cc on Windows on a few tests, but I think this is in pretty good shape to review otherwise.
I couldn't reproduce the earlier test failures on win_chromium_rel, and browser_tests is passing now, so I guess that was a flake.
Minor issues, otherwise LGTM https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:831: #else nit: #elif defined(OS_WIN) to make this a little more obvious. https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:48: // executed. Should we add comments for new params? https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:395: // FIXME: Fix this on windows so that we turn this handle into an int. FIXME->TODO (or remove, it seems like you address it below.) https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:402: #else nit: #elif defined(OS_POSIX) https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:305: // FIXME: Remove this. Remove this. https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:354: #else nit: #elif defined(OS_WIN) https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:355: // Handle duplication is handled on the browser side instead of the renderer. nit wording // Duplicate the handle on the browser side instead of the renderer. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.h (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.h:143: // Load a nacl module from the file specified in file_handle. Update comment to match code.
+cc hidehiko, mseaborn
+mseaborn for components/nacl
https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:917: Nit: remove empty line to make it clear that the comment applies to the part below too. https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:66: uint64_t nexe_token_lo, Could you pass this around as "struct NaClFileToken" where possible, rather than as two int values? https://codereview.chromium.org/332463003/diff/240001/components/nacl/common/... File components/nacl/common/nacl_types.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/common/... components/nacl/common/nacl_types.cc:25: NaClLaunchParams::NaClLaunchParams() Missing initialisation here https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:405: nexe_file_info.file_token.lo = params.nexe_token_lo; FYI, having the browser pass the token through to the NaCl loader process is suboptimal, because the NaCl process will just turn around and send the token values back to the browser (see NaClProcessMsg_ResolveFileToken) in order to get a guaranteed-safe FD back. It would be more efficient for the browser to send a guaranteed-safe FD in the first place! That would save an IPC round trip. (That's what I commented on Hidehiko's earlier change, https://codereview.chromium.org/337463002/#msg3) I'm just mentioning this to make sure you're aware of it. You don't have to change this. https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:483: CHECK(params.version.size() == 0); You could also check that the token is zero, to ensure we're not leaking uninitialised data here. https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:352: if (nexe_file_info->handle != PP_kInvalidFileHandle) If this is false, I think it leaves nexe_for_transit uninitialised. I think IPC::PlatformFileForTransit is not a constructed type. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (left): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:116: // Now actually load the nexe, which can happen on a background thread. What's the reason for removing "which can happen on a background thread"? (I'm not exactly what that comment meant.) https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:110: // Now actually load the nexe. "load" -> "start" https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:162: if (uses_nonsfi_mode) { This should be going away, shouldn't it? https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:167: // Otherwise (i.e. in SFI-mode), LoadModule SRPC is still being used. This is no longer true. :-) https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:244: PP_NaClFileInfo info; Nit: how about "file_info"? ("info" is rather generic) https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:246: info.token_lo = 0; Maybe comment this with: "We never apply validation caching to the PNaCl translator and linker" https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/service_runtime.cc:462: void ServiceRuntime::LoadNexeAndStart() { Since LoadNexeAndStart() no longer loads the nexe, can you rename it to something like StartNexe()? Same goes for the LoadNexeAndStart() in plugin.cc. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/service_runtime.h:180: // Establish an SrpcClient to the sel_ldr instance and load the nexe. "load" -> "start"
+jvoung for checking the validation caching logic.
https://codereview.chromium.org/332463003/diff/260001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/332463003/diff/260001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:182: [in] PP_NaClFileInfo nexe_file_info, rebasing issue? there are now two nexe_file_info parameters? https://codereview.chromium.org/332463003/diff/260001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/332463003/diff/260001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/service_runtime.h:209: bool LoadModule(const PP_NaClFileInfo& file_info); prototype unused now?
https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:831: #else On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > nit: #elif defined(OS_WIN) to make this a little more obvious. Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:917: On 2014/06/30 20:01:03, Mark Seaborn wrote: > Nit: remove empty line to make it clear that the comment applies to the part > below too. Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:48: // executed. On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > Should we add comments for new params? Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:66: uint64_t nexe_token_lo, On 2014/06/30 20:01:03, Mark Seaborn wrote: > Could you pass this around as "struct NaClFileToken" where possible, rather than > as two int values? Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/common/... File components/nacl/common/nacl_types.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/common/... components/nacl/common/nacl_types.cc:25: NaClLaunchParams::NaClLaunchParams() On 2014/06/30 20:01:03, Mark Seaborn wrote: > Missing initialisation here Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:395: // FIXME: Fix this on windows so that we turn this handle into an int. On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > FIXME->TODO (or remove, it seems like you address it below.) Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:402: #else On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > nit: #elif defined(OS_POSIX) Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:405: nexe_file_info.file_token.lo = params.nexe_token_lo; On 2014/06/30 20:01:03, Mark Seaborn wrote: > FYI, having the browser pass the token through to the NaCl loader process is > suboptimal, because the NaCl process will just turn around and send the token > values back to the browser (see NaClProcessMsg_ResolveFileToken) in order to get > a guaranteed-safe FD back. It would be more efficient for the browser to send a > guaranteed-safe FD in the first place! That would save an IPC round trip. > > (That's what I commented on Hidehiko's earlier change, > https://codereview.chromium.org/337463002/#msg3) > > I'm just mentioning this to make sure you're aware of it. You don't have to > change this. I see what you mean. I've added a TODO here to address this, but I'd like to do it in a separate change to keep this one from getting too big. https://codereview.chromium.org/332463003/diff/240001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:483: CHECK(params.version.size() == 0); On 2014/06/30 20:01:03, Mark Seaborn wrote: > You could also check that the token is zero, to ensure we're not leaking > uninitialised data here. Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:305: // FIXME: Remove this. On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > Remove this. Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:352: if (nexe_file_info->handle != PP_kInvalidFileHandle) On 2014/06/30 20:01:03, Mark Seaborn wrote: > If this is false, I think it leaves nexe_for_transit uninitialised. I think > IPC::PlatformFileForTransit is not a constructed type. Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:354: #else On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > nit: #elif defined(OS_WIN) Done. https://codereview.chromium.org/332463003/diff/240001/components/nacl/rendere... components/nacl/renderer/ppb_nacl_private_impl.cc:355: // Handle duplication is handled on the browser side instead of the renderer. On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > nit wording > // Duplicate the handle on the browser side instead of the renderer. Done. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (left): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:116: // Now actually load the nexe, which can happen on a background thread. On 2014/06/30 20:01:04, Mark Seaborn wrote: > What's the reason for removing "which can happen on a background thread"? (I'm > not exactly what that comment meant.) This is an old, wrong comment. Previously LoadNexeAndStart() was safe to call from a background thread, but the code was changed so that's no longer the case. Unfortunately, this stale comment was left behind. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:110: // Now actually load the nexe. On 2014/06/30 20:01:04, Mark Seaborn wrote: > "load" -> "start" Done. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:162: if (uses_nonsfi_mode) { On 2014/06/30 20:01:04, Mark Seaborn wrote: > This should be going away, shouldn't it? Thanks, removed. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:167: // Otherwise (i.e. in SFI-mode), LoadModule SRPC is still being used. On 2014/06/30 20:01:04, Mark Seaborn wrote: > This is no longer true. :-) Removed. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:244: PP_NaClFileInfo info; On 2014/06/30 20:01:03, Mark Seaborn wrote: > Nit: how about "file_info"? ("info" is rather generic) Done. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:246: info.token_lo = 0; On 2014/06/30 20:01:04, Mark Seaborn wrote: > Maybe comment this with: "We never apply validation caching to the PNaCl > translator and linker" Done. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.h (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.h:143: // Load a nacl module from the file specified in file_handle. On 2014/06/30 18:28:53, bbudge (OOO July 1-8) wrote: > Update comment to match code. Done. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/service_runtime.cc:462: void ServiceRuntime::LoadNexeAndStart() { On 2014/06/30 20:01:04, Mark Seaborn wrote: > Since LoadNexeAndStart() no longer loads the nexe, can you rename it to > something like StartNexe()? Same goes for the LoadNexeAndStart() in plugin.cc. Done. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/service_runtime.h:180: // Establish an SrpcClient to the sel_ldr instance and load the nexe. On 2014/06/30 20:01:04, Mark Seaborn wrote: > "load" -> "start" Done.
https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:246: info.token_lo = 0; On 2014/06/30 22:04:56, teravest wrote: > On 2014/06/30 20:01:04, Mark Seaborn wrote: > > Maybe comment this with: "We never apply validation caching to the PNaCl > > translator and linker" > > Done. Wait -- rebase against 280605 ? The should be a test "ValidationCacheOfTranslatorNexes" that checks this: https://codereview.chromium.org/356923004/diff/120001/chrome/test/nacl/nacl_b...
nitpicks, but some compiling looks to fail? https://codereview.chromium.org/332463003/diff/280001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/332463003/diff/280001/components/nacl/common/... components/nacl/common/nacl_types.h:91: IPC::PlatformFileForTransit nexe_file; Probably, it would be better to note that, on Windows, this HANDLE is valid in the renderer's context, (because it seems not a common manner in Chrome?) Or, IMHO, it may be ok to expose the BrokerGetFileHandleForProcess(), because its underlying API BrokerDuplicateHandle is already in the public and BrokerGetFileHandleForProcess looks its simpler wrapper. WDYT? https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:75: bool Plugin::InternalLoadHelperNaClModule(NaClSubprocess* subprocess, nit: "Internal" seems to be usually used as suffix for method names? https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:76: const SelLdrStartParams& params) { nit: let's align the argument indent. https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:117: callback_factory_.NewCallback( nit: can be fit in a line?
I fixed the earlier build error by making sure nacl defines were getting added in components/components_tests.gyp. https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/332463003/diff/240001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:246: info.token_lo = 0; On 2014/06/30 22:34:43, jvoung wrote: > On 2014/06/30 22:04:56, teravest wrote: > > On 2014/06/30 20:01:04, Mark Seaborn wrote: > > > Maybe comment this with: "We never apply validation caching to the PNaCl > > > translator and linker" > > > > Done. > > Wait -- rebase against 280605 ? > > The should be a test "ValidationCacheOfTranslatorNexes" that checks this: > https://codereview.chromium.org/356923004/diff/120001/chrome/test/nacl/nacl_b... Rebased and cleaned this up. Thanks. https://codereview.chromium.org/332463003/diff/260001/ppapi/api/private/ppb_n... File ppapi/api/private/ppb_nacl_private.idl (right): https://codereview.chromium.org/332463003/diff/260001/ppapi/api/private/ppb_n... ppapi/api/private/ppb_nacl_private.idl:182: [in] PP_NaClFileInfo nexe_file_info, On 2014/06/30 21:55:14, jvoung wrote: > rebasing issue? there are now two nexe_file_info parameters? Fixed, thanks. https://codereview.chromium.org/332463003/diff/260001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/service_runtime.h (right): https://codereview.chromium.org/332463003/diff/260001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/service_runtime.h:209: bool LoadModule(const PP_NaClFileInfo& file_info); On 2014/06/30 21:55:14, jvoung wrote: > prototype unused now? Removed. https://codereview.chromium.org/332463003/diff/280001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/332463003/diff/280001/components/nacl/common/... components/nacl/common/nacl_types.h:91: IPC::PlatformFileForTransit nexe_file; On 2014/07/01 05:39:01, hidehiko wrote: > Probably, it would be better to note that, on Windows, this HANDLE is valid in > the renderer's context, (because it seems not a common manner in Chrome?) > > Or, IMHO, it may be ok to expose the BrokerGetFileHandleForProcess(), because > its underlying API BrokerDuplicateHandle is already in the public and > BrokerGetFileHandleForProcess looks its simpler wrapper. WDYT? I've added a comment here as you suggested. I thought about trying to use BrokerGetFileHandleForProcess to duplicate the handle in the renderer, but I think it's simpler to only duplicate the handle once, and not bother involving the broker. https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... File ppapi/native_client/src/trusted/plugin/plugin.cc (right): https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:75: bool Plugin::InternalLoadHelperNaClModule(NaClSubprocess* subprocess, On 2014/07/01 05:39:01, hidehiko wrote: > nit: "Internal" seems to be usually used as suffix for method names? Done. https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:76: const SelLdrStartParams& params) { On 2014/07/01 05:39:01, hidehiko wrote: > nit: let's align the argument indent. Done. https://codereview.chromium.org/332463003/diff/280001/ppapi/native_client/src... ppapi/native_client/src/trusted/plugin/plugin.cc:117: callback_factory_.NewCallback( On 2014/07/01 05:39:01, hidehiko wrote: > nit: can be fit in a line? Done.
https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:412: NaClChromeMainStartApp(nap, args); Since you're not switching to using NaClChromeMainStart() now, presumably this means that the change that added it is no longer necessary (https://src.chromium.org/viewvc/native_client?revision=12937&view=revision)? If so, can you revert that change, so that the currently-in-use interface in chrome_main.h isn't labelled as deprecated? :-)
On Tue, Jul 1, 2014 at 2:32 PM, <mseaborn@chromium.org> wrote: > > https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... > File components/nacl/loader/nacl_listener.cc (right): > > https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... > components/nacl/loader/nacl_listener.cc:412: NaClChromeMainStartApp(nap, > args); > Since you're not switching to using NaClChromeMainStart() now, > presumably this means that the change that added it is no longer > necessary > (https://src.chromium.org/viewvc/native_client?revision=12937&view=revision)? > > If so, can you revert that change, so that the currently-in-use > interface in chrome_main.h isn't labelled as deprecated? :-) Sure, sounds good! > > https://codereview.chromium.org/332463003/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/07/01 21:03:22, teravest wrote: > On Tue, Jul 1, 2014 at 2:32 PM, <mailto:mseaborn@chromium.org> wrote: > > > > > https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... > > File components/nacl/loader/nacl_listener.cc (right): > > > > > https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... > > components/nacl/loader/nacl_listener.cc:412: NaClChromeMainStartApp(nap, > > args); > > Since you're not switching to using NaClChromeMainStart() now, > > presumably this means that the change that added it is no longer > > necessary > > (https://src.chromium.org/viewvc/native_client?revision=12937&view=revision)? > > > > If so, can you revert that change, so that the currently-in-use > > interface in chrome_main.h isn't labelled as deprecated? :-) > > Sure, sounds good! Removed. Mark, do you have any other thoughts on this change? > > > > > https://codereview.chromium.org/332463003/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/332463003/diff/340001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/332463003/diff/340001/components/components_t... components/components_tests.gyp:472: 'nacl/nacl_defines.gypi', Can you comment the reason for this change in the commit message? https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... File components/nacl/browser/DEPS (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... components/nacl/browser/DEPS:4: "+native_client/src/public/nacl_file_info.h", Just put "+native_client/src/public", since the whole directory is intended to be exported. https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:150: base::File( Putting launch_params.nexe_file into a base::File is not safe on Windows. This is a handle ID in the renderer process's handle namespace. But base::File is a managed reference. Its destructor will call CloseHandle() using the current process's namespace. This could give the renderer process the ability to close handle IDs of its choice in the browser. The options are: 1) Avoid using base::File for this. 2) Explicitly call ::DuplicateHandle() here to get a locally-valid handle that you can put into a base::File. (2) seems preferable since it reduces the scope of the remote handle ID, even though it involves two DuplicateHandle() calls instead of one. https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:822: // nexe_file_ still keeps the ownership at this moment, because |params| This comment is not true for the Windows case, because you use DUPLICATE_CLOSE_SOURCE. This could be confusing if the code is changed later. How about: * Use auto_close=False and remove DUPLICATE_CLOSE_SOURCE. * Replace the nexe_file_.TakePlatformFile() call with nexe_file_.Close(). (Only safe with the change in the other comment...) https://codereview.chromium.org/332463003/diff/340001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/common/... components/nacl/common/nacl_types.h:91: // On Windows, the HANDLE passed here is valid in the renderer's context. I just realised that this means the way you're using it is dangerous... https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:406: // TODO(teravest): Resolve the file tokens right now in the browser process Nit: "right now in the browser process" is out of place in loader/ code. :-) This comment belongs in browser/ code -- in nacl_process_host.cc where we send the token.
https://codereview.chromium.org/332463003/diff/340001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/332463003/diff/340001/components/components_t... components/components_tests.gyp:472: 'nacl/nacl_defines.gypi', On 2014/07/02 23:48:36, Mark Seaborn wrote: > Can you comment the reason for this change in the commit message? Done. https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... File components/nacl/browser/DEPS (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... components/nacl/browser/DEPS:4: "+native_client/src/public/nacl_file_info.h", On 2014/07/02 23:48:36, Mark Seaborn wrote: > Just put "+native_client/src/public", since the whole directory is intended to > be exported. Done. https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... File components/nacl/browser/nacl_host_message_filter.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... components/nacl/browser/nacl_host_message_filter.cc:150: base::File( On 2014/07/02 23:48:36, Mark Seaborn wrote: > Putting launch_params.nexe_file into a base::File is not safe on Windows. > > This is a handle ID in the renderer process's handle namespace. > > But base::File is a managed reference. Its destructor will call CloseHandle() > using the current process's namespace. > > This could give the renderer process the ability to close handle IDs of its > choice in the browser. > > The options are: > 1) Avoid using base::File for this. > 2) Explicitly call ::DuplicateHandle() here to get a locally-valid handle that > you can put into a base::File. > > (2) seems preferable since it reduces the scope of the remote handle ID, even > though it involves two DuplicateHandle() calls instead of one. Thanks for catching this! I agree that using ::DuplicateHandle here is the better option. Done. https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:822: // nexe_file_ still keeps the ownership at this moment, because |params| On 2014/07/02 23:48:36, Mark Seaborn wrote: > This comment is not true for the Windows case, because you use > DUPLICATE_CLOSE_SOURCE. This could be confusing if the code is changed later. > > How about: > * Use auto_close=False and remove DUPLICATE_CLOSE_SOURCE. > * Replace the nexe_file_.TakePlatformFile() call with nexe_file_.Close(). > (Only safe with the change in the other comment...) I followed your suggestion here, though I changed the ::DuplicateHandle call here to IPC::GetFileHandleForProcess, which is a bit easier to read. https://codereview.chromium.org/332463003/diff/340001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/common/... components/nacl/common/nacl_types.h:91: // On Windows, the HANDLE passed here is valid in the renderer's context. On 2014/07/02 23:48:36, Mark Seaborn wrote: > I just realised that this means the way you're using it is dangerous... Thanks for noticing that. I've left this comment here since it's still accurate. https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/332463003/diff/340001/components/nacl/loader/... components/nacl/loader/nacl_listener.cc:406: // TODO(teravest): Resolve the file tokens right now in the browser process On 2014/07/02 23:48:36, Mark Seaborn wrote: > Nit: "right now in the browser process" is out of place in loader/ code. :-) > This comment belongs in browser/ code -- in nacl_process_host.cc where we send > the token. Done.
https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:828: base::FileDescriptor(nexe_file_.TakePlatformFile(), false); This leaks the FD. You should either do: // takes ownership of FD // -- would leak on an early return base::FileDescriptor(nexe_file_.TakePlatformFile(), true) or // borrowed reference to FD // -- wouldn't leak on an early return base::FileDescriptor(nexe_file_.GetPlatformFile(), false) https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:830: params.nexe_file = IPC::GetFileHandleForProcess(nexe_file_.TakePlatformFile(), This has a similar leak. https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:912: nexe_file_.Close(); This has no effect, because the code did TakePlatformFile() on nexe_file_ earlier, so nexe_file_ contains no descriptor/handle.
https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:828: base::FileDescriptor(nexe_file_.TakePlatformFile(), false); On 2014/07/08 18:46:00, Mark Seaborn wrote: > This leaks the FD. You should either do: > > // takes ownership of FD > // -- would leak on an early return > base::FileDescriptor(nexe_file_.TakePlatformFile(), true) > > or > > // borrowed reference to FD > // -- wouldn't leak on an early return > base::FileDescriptor(nexe_file_.GetPlatformFile(), false) Yes, you're right. I wanted GetPlatformFile() here. https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:830: params.nexe_file = IPC::GetFileHandleForProcess(nexe_file_.TakePlatformFile(), On 2014/07/08 18:46:00, Mark Seaborn wrote: > This has a similar leak. Done. https://codereview.chromium.org/332463003/diff/380001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:912: nexe_file_.Close(); On 2014/07/08 18:46:00, Mark Seaborn wrote: > This has no effect, because the code did TakePlatformFile() on nexe_file_ > earlier, so nexe_file_ contains no descriptor/handle. Done.
https://codereview.chromium.org/332463003/diff/400001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/400001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:828: base::FileDescriptor(nexe_file_.GetPlatformFile(), false); This means you *can't* do nexe_file_.Close() before sending the IPC message (hence your trybot failures). It would be useful to do nexe_file_.Close() after sending the IPC message.
On 2014/07/08 20:32:48, Mark Seaborn wrote: > https://codereview.chromium.org/332463003/diff/400001/components/nacl/browser... > File components/nacl/browser/nacl_process_host.cc (right): > > https://codereview.chromium.org/332463003/diff/400001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:828: > base::FileDescriptor(nexe_file_.GetPlatformFile(), false); > This means you *can't* do nexe_file_.Close() before sending the IPC message > (hence your trybot failures). It would be useful to do nexe_file_.Close() after > sending the IPC message. Okay. I thought more carefully about this now, and changed to do the assignment to params.nexe_file later in the function, and use TakePlatformFile(). I think that closing the file descriptor at IPC time on POSIX (or duplication time on Windows) is simpler to understand than trying to manually close the file after IPC has sent the message for us. I had debated moving nexe_file_.Close() to happen after Send(), but I don't want to depend on the Send() implementation there not calling PostTask() or something similar, and causing an ordering problem.
LGTM https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:899: // nexe_file_ still keeps the ownership at this moment, because |params| Doesn't this need updating, now that you're using nexe_file_.TakePlatformFile()?
lgtm https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:904: params.nexe_file = nit: maybe: params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(), process_->GetData().handle); is what you want?
https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:899: // nexe_file_ still keeps the ownership at this moment, because |params| On 2014/07/09 22:44:19, Mark Seaborn wrote: > Doesn't this need updating, now that you're using nexe_file_.TakePlatformFile()? Yes, done. https://codereview.chromium.org/332463003/diff/420001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:904: params.nexe_file = On 2014/07/10 06:18:39, hidehiko wrote: > nit: maybe: > > params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(), > process_->GetData().handle); > > is what you want? Done.
The CQ bit was checked by teravest@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/460001
+jschuh for components/nacl/common/*_messages.h
ipc security lgtm (notes: file token 128-bit id)
The CQ bit was unchecked by teravest@chromium.org
The CQ bit was checked by teravest@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/460001
https://codereview.chromium.org/332463003/diff/460001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/332463003/diff/460001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:900: params.nexe_file = FYI: you don't need to switch by ifdef here. You just can use TakeFileHandleForProcess for both platforms.
The CQ bit was unchecked by teravest@chromium.org
The CQ bit was checked by teravest@chromium.org
On 2014/07/10 16:51:24, hidehiko wrote: > https://codereview.chromium.org/332463003/diff/460001/components/nacl/browser... > File components/nacl/browser/nacl_process_host.cc (right): > > https://codereview.chromium.org/332463003/diff/460001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:900: params.nexe_file = > FYI: you don't need to switch by ifdef here. You just can use > TakeFileHandleForProcess for both platforms. done, thanks!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/480001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
The CQ bit was checked by teravest@chromium.org
The CQ bit was unchecked by teravest@chromium.org
+brettw for chrome/browser/BUILD.gn Now that there's been a successful NaCl DEPS roll, the we can use //native_client/build/config/nacl_defines.gni from BUILD.gn
gn lgtm
The CQ bit was checked by teravest@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/500001
+sky since apparently brett's GN approval still doesn't count for chrome/browser/BUILD.gn
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
I'm not actually an owner of all BUILD.gn files. The script that tells you this is wrong. So you don't need my approval for all changes, only if you're unsure of what you're doing or you otherwise need my owners approval.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
BUILD.gn LGTM
The CQ bit was checked by teravest@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/500001
Message was sent while issue was closed.
Committed patchset #14 manually as r285028 (presubmit successful).
Message was sent while issue was closed.
Failed to commit the patch. Sending chrome/browser/BUILD.gn Sending components/components_tests.gyp Sending components/nacl/browser/DEPS Sending components/nacl/browser/nacl_host_message_filter.cc Sending components/nacl/browser/nacl_process_host.cc Sending components/nacl/browser/nacl_process_host.h Sending components/nacl/common/nacl_host_messages.h Sending components/nacl/common/nacl_messages.h Sending components/nacl/common/nacl_types.cc Sending components/nacl/common/nacl_types.h Sending components/nacl/loader/nacl_listener.cc Sending components/nacl/renderer/ppb_nacl_private_impl.cc Sending ppapi/native_client/src/trusted/plugin/plugin.cc Sending ppapi/native_client/src/trusted/plugin/plugin.h Sending ppapi/native_client/src/trusted/plugin/service_runtime.cc Sending ppapi/native_client/src/trusted/plugin/service_runtime.h Transmitting file data ................svn: Commit failed (details follow): svn: File '/trunk/src/chrome/browser/BUILD.gn' is out of date
This change was reverted since it broke the disable_nacl=1 and some TSAN buildbot. I suspect that buildbot is using disable_nacl=1 as well. http://code.google.com/p/chromium/issues/detail?id=396960 I think that should be simple to fix.
The CQ bit was checked by teravest@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/560001
https://codereview.chromium.org/332463003/diff/560001/components/nacl/browser... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/332463003/diff/560001/components/nacl/browser... components/nacl/browser/nacl_process_host.h:198: // TODO(teravest): Use NaClFileInfo here, but without breaking the Why is this necessary? Is "native_client/src/public/nacl_file_info.h" not #includable in the disable_nacl=1 build? I thought native_client/ would still be checked out when disable_nacl=1, or is that not the case?
On 2014/07/24 19:10:59, Mark Seaborn wrote: > https://codereview.chromium.org/332463003/diff/560001/components/nacl/browser... > File components/nacl/browser/nacl_process_host.h (right): > > https://codereview.chromium.org/332463003/diff/560001/components/nacl/browser... > components/nacl/browser/nacl_process_host.h:198: // TODO(teravest): Use > NaClFileInfo here, but without breaking the > Why is this necessary? Is "native_client/src/public/nacl_file_info.h" not > #includable in the disable_nacl=1 build? I thought native_client/ would still > be checked out when disable_nacl=1, or is that not the case? nacl_defines is currently not built when disable_nacl=1. However, nacl_defines is a prerequisite for building native_client/src/public/nacl_file_info.h-- eventually the includes make their way down to native_client/src/include/nacl_compiler_annotations.h, which causes a build error when the nacl defines aren't available. I think a better solution is to not include nacl_process_host.h for disable_nacl=1 builds, but I would prefer to have this change checked in-- it's not too bad to store two uint64 values here in the interim instead of a NaClFileToken structure until this is solved.
On 24 July 2014 12:19, <teravest@chromium.org> wrote: > On 2014/07/24 19:10:59, Mark Seaborn wrote: > > https://codereview.chromium.org/332463003/diff/560001/ > components/nacl/browser/nacl_process_host.h > >> File components/nacl/browser/nacl_process_host.h (right): >> > > > https://codereview.chromium.org/332463003/diff/560001/ > components/nacl/browser/nacl_process_host.h#newcode198 > >> components/nacl/browser/nacl_process_host.h:198: // TODO(teravest): Use >> NaClFileInfo here, but without breaking the >> Why is this necessary? Is "native_client/src/public/nacl_file_info.h" >> not >> #includable in the disable_nacl=1 build? I thought native_client/ would >> still >> be checked out when disable_nacl=1, or is that not the case? >> > > nacl_defines is currently not built when disable_nacl=1. However, > nacl_defines > is a prerequisite for building native_client/src/public/nacl_file_info.h-- > eventually the includes make their way down to > native_client/src/include/nacl_compiler_annotations.h, which causes a > build > error when the nacl defines aren't available. > > I think a better solution is to not include nacl_process_host.h for > disable_nacl=1 builds, but I would prefer to have this change checked in-- > it's > not too bad to store two uint64 values here in the interim instead of a > NaClFileToken structure until this is solved. I see. Thanks for explaining. It might be worth adding a comment next to "struct NaClFileToken;" saying why nacl_file_info.h isn't #includable when disable_nacl=1, otherwise it would be easy for someone to break this again in the future. Cheers, Mark To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by teravest@chromium.org
On 2014/07/24 19:24:40, Mark Seaborn wrote: > On 24 July 2014 12:19, <mailto:teravest@chromium.org> wrote: > > > On 2014/07/24 19:10:59, Mark Seaborn wrote: > > > > https://codereview.chromium.org/332463003/diff/560001/ > > components/nacl/browser/nacl_process_host.h > > > >> File components/nacl/browser/nacl_process_host.h (right): > >> > > > > > > https://codereview.chromium.org/332463003/diff/560001/ > > components/nacl/browser/nacl_process_host.h#newcode198 > > > >> components/nacl/browser/nacl_process_host.h:198: // TODO(teravest): Use > >> NaClFileInfo here, but without breaking the > >> Why is this necessary? Is "native_client/src/public/nacl_file_info.h" > >> not > >> #includable in the disable_nacl=1 build? I thought native_client/ would > >> still > >> be checked out when disable_nacl=1, or is that not the case? > >> > > > > nacl_defines is currently not built when disable_nacl=1. However, > > nacl_defines > > is a prerequisite for building native_client/src/public/nacl_file_info.h-- > > eventually the includes make their way down to > > native_client/src/include/nacl_compiler_annotations.h, which causes a > > build > > error when the nacl defines aren't available. > > > > I think a better solution is to not include nacl_process_host.h for > > disable_nacl=1 builds, but I would prefer to have this change checked in-- > > it's > > not too bad to store two uint64 values here in the interim instead of a > > NaClFileToken structure until this is solved. > > > I see. Thanks for explaining. > > It might be worth adding a comment next to "struct NaClFileToken;" saying > why nacl_file_info.h isn't #includable when disable_nacl=1, otherwise it > would be easy for someone to break this again in the future. Sounds good, I'll do that. > > Cheers, > Mark > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by teravest@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/teravest@chromium.org/332463003/580001
Message was sent while issue was closed.
Change committed as 285418 |
