Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1094653003:
Refactor NaClProcessHost. Reduce chances to leak the resource. (Closed)
|
Created:
5 years, 8 months ago by hidehiko Modified:
5 years, 7 months ago Reviewers:
Mark Seaborn CC:
chromium-reviews, hamaji, mazda Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor NaClProcessHost. Reduce chances to leak the resource.
This is preparation to address review comments of
https://codereview.chromium.org/1051243002/
Currently, there are some resource leak paths in nacl_process_host.cc.
This CL removes those leaks, which are related to NaCl IPC types,
defined in nacl_types.h.
TEST=Ran bots.
BUG=358417
Committed: https://crrev.com/b577bc48a00cd70600aa310eaf66faf3ec80758d
Cr-Commit-Position: refs/heads/master@{#328698}
Patch Set 1 : #
Total comments: 29
Patch Set 2 : #
Total comments: 22
Patch Set 3 : Rebase #Patch Set 4 : #
Total comments: 2
Messages
Total messages: 26 (11 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
hidehiko@chromium.org changed reviewers: + mseaborn@chromium.org
PTAL. Note: This is the CL for the preparation of addressing your comment https://codereview.chromium.org/1051243002/ (about resource leaking). I know, around the code, there are many potential resource leak code paths, but fixing all of them does not sound reasonable. My goal is complete the https://codereview.chromium.org/1051243002/, and I believe, with this change, I will be able to address your comment in sane manner. And, everything other is out of scope (at least in this CL). Thank you for review in advance, - hidehiko https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:1033: if (base::PostTaskAndReplyWithResult( Note: this code also has a potential resource leak. However, fixing this also needs bigger refactoring. In this CL, it is out of scope.
On 2015/04/17 07:25:33, hidehiko wrote: > PTAL. > > Note: This is the CL for the preparation of addressing your comment > https://codereview.chromium.org/1051243002/ (about resource leaking). > I know, around the code, there are many potential resource leak code paths, but > fixing all of them does not sound reasonable. My goal is complete the > https://codereview.chromium.org/1051243002/, and I believe, with this change, I > will be able to address your comment in sane manner. And, everything other is > out of scope (at least in this CL). > > Thank you for review in advance, > - hidehiko > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > File components/nacl/browser/nacl_process_host.cc (right): > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:1033: if > (base::PostTaskAndReplyWithResult( > Note: this code also has a potential resource leak. > However, fixing this also needs bigger refactoring. In this CL, it is out of > scope. One more thing (reply to your review comment in the CL): Currently, base::File does not seems to support as a parameter of IPC message by default. Also, base::File usually do not work for our cases, because it closes the platform file in its dtor, but our code usually works on IO thread, where file operation is prohibited.
https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:156: // Usually, NaClProcessHost is working on IO thread, where file operation is As an aside: I am sceptical that close() or CloseHandle() really block. I think this might be overzealousness on the part of the blocking checks in base/. But I don't have any hard data about this. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:159: // NaClProcessHost's destructor runnign on IO thread, because BlockingPool "running" https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:161: void CloseFile(base::File file) { CloseFile() is a rather generic name. Maybe CloseFileInBlockingPool() or CloseFileOnFileThread()? https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:168: base::Bind(&(ignore_result<base::File>), base::Passed(&file))); Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"? (I'm not sure.) https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:178: CloseFile(IPC::PlatformFileForTransitToFile(file).Pass()); I think this is unsafe on Windows, because this is a handle in another process. See my comment below about ScopedSharedMemoryHandle. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:252: // because there is neither operator== nor operator!= definition for Did you consider defining those operators on IPC::ChannelHandle? (I'm not sure what the trade off is here.) https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:276: // It is not allowed to reset with the same (non-invalid) handle. Can you say why? https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:282: !handle.socket.auto_close || Why check auto_close as part of the comparison? https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:284: #endif "#else #error Unknown platform"? https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:291: #if defined(OS_WIN) How about adding a Close() method to IPC::ChannelHandle so that you don't need conditionalisation here? Getting an ipc/ owners review for adding that would actually be quite useful. It would be good for an ipc/ owner to look at this change. :-) https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:293: base::win::ScopedHandle(handle_.pipe.handle); Strangely, this *isn't* going through BlockingPool. It's kind of odd that you go to the trouble of using BlockingPool in some cases but not others. I realise that this is probably an artefact of some Chromium APIs being (unnecessarily?) stricter than others. Maybe this warrants a comment to explain the apparent inconsistency? https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:299: #endif Ditto: add an #else #error? https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:778: ScopedSharedMemoryHandle crash_info_shmem_renderer_handle; I think this is unsafe on Windows. Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index into another process's handle table. If you call CloseHandle() on it, potentially you're closing an unrelated handle in the current process, which might be exploitable. Unfortunately, SharedMemory::CloseHandle() doesn't check the return value of CloseHandle(), which is rather dangerous. On Windows, I think your options for error handling are: * Let the handle leak in the destination process. * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle from the destination process's handle table. Does Chromium actually do this anywhere on an error-handling code path? This is somewhat risky given that error code paths are under-tested. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... components/nacl/common/nacl_types.h:18: Nit: remove 1 empty line so that there's only 1 between blocks
Patchset #2 (id:60001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #4 (id:120001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:80001) has been deleted
PTAL. Note: IPC between browser process and a plug-in process is addressed by other CLs (sent to you for review, too). Now, this CL addresses IPC between renderer process and browser process. I rewrote the code on ToT, so maybe diff between PS1 and PS2 wouldn't make sense. Instead, "view" link of PS2 (diff between PS2 and ToT) is the better to see. Though, I reused this CL to reply your comments. Thanks, - hidehiko https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:156: // Usually, NaClProcessHost is working on IO thread, where file operation is On 2015/04/17 22:13:45, Mark Seaborn wrote: > As an aside: I am sceptical that close() or CloseHandle() really block. I think > this might be overzealousness on the part of the blocking checks in base/. But > I don't have any hard data about this. According to my coworker who is the expert of Windows, CloseHandle does in most cases practically (no spec), though close() on, e.g., linux, do not. Though, it depends on what resource we're releasing. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:159: // NaClProcessHost's destructor runnign on IO thread, because BlockingPool On 2015/04/17 22:13:45, Mark Seaborn wrote: > "running" Acknowledged. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:161: void CloseFile(base::File file) { On 2015/04/17 22:13:45, Mark Seaborn wrote: > CloseFile() is a rather generic name. Maybe CloseFileInBlockingPool() or > CloseFileOnFileThread()? Then, probably PostCloseFile etc. would be better. In chrome tree, OnSomeThread suffix is used for functions which run on the thread actually, rather than a function to post a task to the target thread, so maybe it is better to avoid using it here. Reflected in another CL. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:168: base::Bind(&(ignore_result<base::File>), base::Passed(&file))); On 2015/04/17 22:13:45, Mark Seaborn wrote: > Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"? (I'm not > sure.) Reflected in another CL. I just misunderstood it must be needed. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:178: CloseFile(IPC::PlatformFileForTransitToFile(file).Pass()); On 2015/04/17 22:13:45, Mark Seaborn wrote: > I think this is unsafe on Windows, because this is a handle in another process. > See my comment below about ScopedSharedMemoryHandle. You're right. Fixed. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:252: // because there is neither operator== nor operator!= definition for On 2015/04/17 22:13:45, Mark Seaborn wrote: > Did you consider defining those operators on IPC::ChannelHandle? (I'm not sure > what the trade off is here.) Defining operator== or operator!= needs to be in the base or IPC namespace, because of ScopedGeneric implementation, even we have ADL. Our usage is subset of ChannelHandle now, and our implementation depends on that. So, I prefer not to use it to avoid making other namespace dirtier. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:276: // It is not allowed to reset with the same (non-invalid) handle. On 2015/04/17 22:13:45, Mark Seaborn wrote: > Can you say why? Done. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:282: !handle.socket.auto_close || On 2015/04/17 22:13:45, Mark Seaborn wrote: > Why check auto_close as part of the comparison? Acknowledged. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:284: #endif On 2015/04/17 22:13:45, Mark Seaborn wrote: > "#else > #error Unknown platform"? Acknowledged. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:291: #if defined(OS_WIN) On 2015/04/17 22:13:45, Mark Seaborn wrote: > How about adding a Close() method to IPC::ChannelHandle so that you don't need > conditionalisation here? > > Getting an ipc/ owners review for adding that would actually be quite useful. > It would be good for an ipc/ owner to look at this change. :-) Again, our code depends on some constraint, so having such an API in ChannelHandle does not make sense. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:293: base::win::ScopedHandle(handle_.pipe.handle); On 2015/04/17 22:13:45, Mark Seaborn wrote: > Strangely, this *isn't* going through BlockingPool. > > It's kind of odd that you go to the trouble of using BlockingPool in some cases > but not others. I realise that this is probably an artefact of some Chromium > APIs being (unnecessarily?) stricter than others. > > Maybe this warrants a comment to explain the apparent inconsistency? Acknowledged. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:299: #endif On 2015/04/17 22:13:45, Mark Seaborn wrote: > Ditto: add an #else #error? Acknowledged. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... components/nacl/browser/nacl_process_host.cc:778: ScopedSharedMemoryHandle crash_info_shmem_renderer_handle; On 2015/04/17 22:13:45, Mark Seaborn wrote: > I think this is unsafe on Windows. > > Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index into > another process's handle table. If you call CloseHandle() on it, potentially > you're closing an unrelated handle in the current process, which might be > exploitable. > > Unfortunately, SharedMemory::CloseHandle() doesn't check the return value of > CloseHandle(), which is rather dangerous. > > On Windows, I think your options for error handling are: > > * Let the handle leak in the destination process. > * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle from the > destination process's handle table. Does Chromium actually do this anywhere on > an error-handling code path? This is somewhat risky given that error code paths > are under-tested. You're right. Fixed. https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... components/nacl/common/nacl_types.h:18: On 2015/04/17 22:13:45, Mark Seaborn wrote: > Nit: remove 1 empty line so that there's only 1 between blocks Done.
On 2015/04/30 16:33:02, hidehiko wrote: > PTAL. > > Note: IPC between browser process and a plug-in process is addressed by other > CLs (sent to you for review, too). > Now, this CL addresses IPC between renderer process and browser process. > I rewrote the code on ToT, so maybe diff between PS1 and PS2 wouldn't make > sense. Instead, "view" link of PS2 (diff between PS2 and ToT) is the better to > see. Though, I reused this CL to reply your comments. > > Thanks, > - hidehiko > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > File components/nacl/browser/nacl_process_host.cc (right): > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:156: // Usually, NaClProcessHost is > working on IO thread, where file operation is > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > As an aside: I am sceptical that close() or CloseHandle() really block. I > think > > this might be overzealousness on the part of the blocking checks in base/. > But > > I don't have any hard data about this. > > According to my coworker who is the expert of Windows, CloseHandle does in most > cases practically (no spec), though close() on, e.g., linux, do not. Though, it > depends on what resource we're releasing. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:159: // NaClProcessHost's > destructor runnign on IO thread, because BlockingPool > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > "running" > > Acknowledged. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:161: void CloseFile(base::File > file) { > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > CloseFile() is a rather generic name. Maybe CloseFileInBlockingPool() or > > CloseFileOnFileThread()? > > Then, probably PostCloseFile etc. would be better. > In chrome tree, OnSomeThread suffix is used for functions which run on the > thread actually, rather than a function to post a task to the target thread, so > maybe it is better to avoid using it here. > > Reflected in another CL. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:168: > base::Bind(&(ignore_result<base::File>), base::Passed(&file))); > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"? (I'm > not > > sure.) > > Reflected in another CL. I just misunderstood it must be needed. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:178: > CloseFile(IPC::PlatformFileForTransitToFile(file).Pass()); > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > I think this is unsafe on Windows, because this is a handle in another > process. > > See my comment below about ScopedSharedMemoryHandle. > > You're right. Fixed. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:252: // because there is neither > operator== nor operator!= definition for > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Did you consider defining those operators on IPC::ChannelHandle? (I'm not > sure > > what the trade off is here.) > > Defining operator== or operator!= needs to be in the base or IPC namespace, > because of ScopedGeneric implementation, even we have ADL. Our usage is subset > of ChannelHandle now, and our implementation depends on that. So, I prefer not > to use it to avoid making other namespace dirtier. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:276: // It is not allowed to reset > with the same (non-invalid) handle. > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Can you say why? > > Done. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:282: !handle.socket.auto_close || > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Why check auto_close as part of the comparison? > > Acknowledged. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:284: #endif > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > "#else > > #error Unknown platform"? > > Acknowledged. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:291: #if defined(OS_WIN) > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > How about adding a Close() method to IPC::ChannelHandle so that you don't need > > conditionalisation here? > > > > Getting an ipc/ owners review for adding that would actually be quite useful. > > It would be good for an ipc/ owner to look at this change. :-) > > Again, our code depends on some constraint, so having such an API in > ChannelHandle does not make sense. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:293: > base::win::ScopedHandle(handle_.pipe.handle); > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Strangely, this *isn't* going through BlockingPool. > > > > It's kind of odd that you go to the trouble of using BlockingPool in some > cases > > but not others. I realise that this is probably an artefact of some Chromium > > APIs being (unnecessarily?) stricter than others. > > > > Maybe this warrants a comment to explain the apparent inconsistency? > > Acknowledged. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:299: #endif > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Ditto: add an #else #error? > > Acknowledged. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > components/nacl/browser/nacl_process_host.cc:778: ScopedSharedMemoryHandle > crash_info_shmem_renderer_handle; > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > I think this is unsafe on Windows. > > > > Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index into > > another process's handle table. If you call CloseHandle() on it, potentially > > you're closing an unrelated handle in the current process, which might be > > exploitable. > > > > Unfortunately, SharedMemory::CloseHandle() doesn't check the return value of > > CloseHandle(), which is rather dangerous. > > > > On Windows, I think your options for error handling are: > > > > * Let the handle leak in the destination process. > > * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle from > the > > destination process's handle table. Does Chromium actually do this anywhere > on > > an error-handling code path? This is somewhat risky given that error code > paths > > are under-tested. > > You're right. Fixed. > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > File components/nacl/common/nacl_types.h (right): > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > components/nacl/common/nacl_types.h:18: > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > Nit: remove 1 empty line so that there's only 1 between blocks > > Done.
Friendly ping? On 2015/05/01 16:27:01, hidehiko wrote: > On 2015/04/30 16:33:02, hidehiko wrote: > > PTAL. > > > > Note: IPC between browser process and a plug-in process is addressed by other > > CLs (sent to you for review, too). > > Now, this CL addresses IPC between renderer process and browser process. > > I rewrote the code on ToT, so maybe diff between PS1 and PS2 wouldn't make > > sense. Instead, "view" link of PS2 (diff between PS2 and ToT) is the better to > > see. Though, I reused this CL to reply your comments. > > > > Thanks, > > - hidehiko > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > File components/nacl/browser/nacl_process_host.cc (right): > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:156: // Usually, NaClProcessHost > is > > working on IO thread, where file operation is > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > As an aside: I am sceptical that close() or CloseHandle() really block. I > > think > > > this might be overzealousness on the part of the blocking checks in base/. > > But > > > I don't have any hard data about this. > > > > According to my coworker who is the expert of Windows, CloseHandle does in > most > > cases practically (no spec), though close() on, e.g., linux, do not. Though, > it > > depends on what resource we're releasing. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:159: // NaClProcessHost's > > destructor runnign on IO thread, because BlockingPool > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > "running" > > > > Acknowledged. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:161: void CloseFile(base::File > > file) { > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > CloseFile() is a rather generic name. Maybe CloseFileInBlockingPool() or > > > CloseFileOnFileThread()? > > > > Then, probably PostCloseFile etc. would be better. > > In chrome tree, OnSomeThread suffix is used for functions which run on the > > thread actually, rather than a function to post a task to the target thread, > so > > maybe it is better to avoid using it here. > > > > Reflected in another CL. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:168: > > base::Bind(&(ignore_result<base::File>), base::Passed(&file))); > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"? (I'm > > not > > > sure.) > > > > Reflected in another CL. I just misunderstood it must be needed. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:178: > > CloseFile(IPC::PlatformFileForTransitToFile(file).Pass()); > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > I think this is unsafe on Windows, because this is a handle in another > > process. > > > See my comment below about ScopedSharedMemoryHandle. > > > > You're right. Fixed. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:252: // because there is neither > > operator== nor operator!= definition for > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Did you consider defining those operators on IPC::ChannelHandle? (I'm not > > sure > > > what the trade off is here.) > > > > Defining operator== or operator!= needs to be in the base or IPC namespace, > > because of ScopedGeneric implementation, even we have ADL. Our usage is subset > > of ChannelHandle now, and our implementation depends on that. So, I prefer not > > to use it to avoid making other namespace dirtier. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:276: // It is not allowed to > reset > > with the same (non-invalid) handle. > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Can you say why? > > > > Done. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:282: !handle.socket.auto_close || > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Why check auto_close as part of the comparison? > > > > Acknowledged. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:284: #endif > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > "#else > > > #error Unknown platform"? > > > > Acknowledged. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:291: #if defined(OS_WIN) > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > How about adding a Close() method to IPC::ChannelHandle so that you don't > need > > > conditionalisation here? > > > > > > Getting an ipc/ owners review for adding that would actually be quite > useful. > > > It would be good for an ipc/ owner to look at this change. :-) > > > > Again, our code depends on some constraint, so having such an API in > > ChannelHandle does not make sense. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:293: > > base::win::ScopedHandle(handle_.pipe.handle); > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Strangely, this *isn't* going through BlockingPool. > > > > > > It's kind of odd that you go to the trouble of using BlockingPool in some > > cases > > > but not others. I realise that this is probably an artefact of some > Chromium > > > APIs being (unnecessarily?) stricter than others. > > > > > > Maybe this warrants a comment to explain the apparent inconsistency? > > > > Acknowledged. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:299: #endif > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Ditto: add an #else #error? > > > > Acknowledged. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > components/nacl/browser/nacl_process_host.cc:778: ScopedSharedMemoryHandle > > crash_info_shmem_renderer_handle; > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > I think this is unsafe on Windows. > > > > > > Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index > into > > > another process's handle table. If you call CloseHandle() on it, > potentially > > > you're closing an unrelated handle in the current process, which might be > > > exploitable. > > > > > > Unfortunately, SharedMemory::CloseHandle() doesn't check the return value of > > > CloseHandle(), which is rather dangerous. > > > > > > On Windows, I think your options for error handling are: > > > > > > * Let the handle leak in the destination process. > > > * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle from > > the > > > destination process's handle table. Does Chromium actually do this anywhere > > on > > > an error-handling code path? This is somewhat risky given that error code > > paths > > > are under-tested. > > > > You're right. Fixed. > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > > File components/nacl/common/nacl_types.h (right): > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > > components/nacl/common/nacl_types.h:18: > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > Nit: remove 1 empty line so that there's only 1 between blocks > > > > Done.
Friendly ping? On 2015/05/01 16:27:18, hidehiko wrote: > Friendly ping? > > On 2015/05/01 16:27:01, hidehiko wrote: > > On 2015/04/30 16:33:02, hidehiko wrote: > > > PTAL. > > > > > > Note: IPC between browser process and a plug-in process is addressed by > other > > > CLs (sent to you for review, too). > > > Now, this CL addresses IPC between renderer process and browser process. > > > I rewrote the code on ToT, so maybe diff between PS1 and PS2 wouldn't make > > > sense. Instead, "view" link of PS2 (diff between PS2 and ToT) is the better > to > > > see. Though, I reused this CL to reply your comments. > > > > > > Thanks, > > > - hidehiko > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > File components/nacl/browser/nacl_process_host.cc (right): > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:156: // Usually, > NaClProcessHost > > is > > > working on IO thread, where file operation is > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > As an aside: I am sceptical that close() or CloseHandle() really block. I > > > think > > > > this might be overzealousness on the part of the blocking checks in base/. > > > > But > > > > I don't have any hard data about this. > > > > > > According to my coworker who is the expert of Windows, CloseHandle does in > > most > > > cases practically (no spec), though close() on, e.g., linux, do not. Though, > > it > > > depends on what resource we're releasing. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:159: // NaClProcessHost's > > > destructor runnign on IO thread, because BlockingPool > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > "running" > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:161: void CloseFile(base::File > > > file) { > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > CloseFile() is a rather generic name. Maybe CloseFileInBlockingPool() or > > > > CloseFileOnFileThread()? > > > > > > Then, probably PostCloseFile etc. would be better. > > > In chrome tree, OnSomeThread suffix is used for functions which run on the > > > thread actually, rather than a function to post a task to the target thread, > > so > > > maybe it is better to avoid using it here. > > > > > > Reflected in another CL. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:168: > > > base::Bind(&(ignore_result<base::File>), base::Passed(&file))); > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"? > (I'm > > > not > > > > sure.) > > > > > > Reflected in another CL. I just misunderstood it must be needed. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:178: > > > CloseFile(IPC::PlatformFileForTransitToFile(file).Pass()); > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > I think this is unsafe on Windows, because this is a handle in another > > > process. > > > > See my comment below about ScopedSharedMemoryHandle. > > > > > > You're right. Fixed. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:252: // because there is > neither > > > operator== nor operator!= definition for > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Did you consider defining those operators on IPC::ChannelHandle? (I'm not > > > sure > > > > what the trade off is here.) > > > > > > Defining operator== or operator!= needs to be in the base or IPC namespace, > > > because of ScopedGeneric implementation, even we have ADL. Our usage is > subset > > > of ChannelHandle now, and our implementation depends on that. So, I prefer > not > > > to use it to avoid making other namespace dirtier. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:276: // It is not allowed to > > reset > > > with the same (non-invalid) handle. > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Can you say why? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:282: !handle.socket.auto_close > || > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Why check auto_close as part of the comparison? > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:284: #endif > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > "#else > > > > #error Unknown platform"? > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:291: #if defined(OS_WIN) > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > How about adding a Close() method to IPC::ChannelHandle so that you don't > > need > > > > conditionalisation here? > > > > > > > > Getting an ipc/ owners review for adding that would actually be quite > > useful. > > > > It would be good for an ipc/ owner to look at this change. :-) > > > > > > Again, our code depends on some constraint, so having such an API in > > > ChannelHandle does not make sense. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:293: > > > base::win::ScopedHandle(handle_.pipe.handle); > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Strangely, this *isn't* going through BlockingPool. > > > > > > > > It's kind of odd that you go to the trouble of using BlockingPool in some > > > cases > > > > but not others. I realise that this is probably an artefact of some > > Chromium > > > > APIs being (unnecessarily?) stricter than others. > > > > > > > > Maybe this warrants a comment to explain the apparent inconsistency? > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:299: #endif > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Ditto: add an #else #error? > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > components/nacl/browser/nacl_process_host.cc:778: ScopedSharedMemoryHandle > > > crash_info_shmem_renderer_handle; > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > I think this is unsafe on Windows. > > > > > > > > Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index > > into > > > > another process's handle table. If you call CloseHandle() on it, > > potentially > > > > you're closing an unrelated handle in the current process, which might be > > > > exploitable. > > > > > > > > Unfortunately, SharedMemory::CloseHandle() doesn't check the return value > of > > > > CloseHandle(), which is rather dangerous. > > > > > > > > On Windows, I think your options for error handling are: > > > > > > > > * Let the handle leak in the destination process. > > > > * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle > from > > > the > > > > destination process's handle table. Does Chromium actually do this > anywhere > > > on > > > > an error-handling code path? This is somewhat risky given that error code > > > paths > > > > are under-tested. > > > > > > You're right. Fixed. > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > > > File components/nacl/common/nacl_types.h (right): > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > > > components/nacl/common/nacl_types.h:18: > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > Nit: remove 1 empty line so that there's only 1 between blocks > > > > > > Done.
Friendly ping? On 2015/05/04 16:21:43, hidehiko wrote: > Friendly ping? > > On 2015/05/01 16:27:18, hidehiko wrote: > > Friendly ping? > > > > On 2015/05/01 16:27:01, hidehiko wrote: > > > On 2015/04/30 16:33:02, hidehiko wrote: > > > > PTAL. > > > > > > > > Note: IPC between browser process and a plug-in process is addressed by > > other > > > > CLs (sent to you for review, too). > > > > Now, this CL addresses IPC between renderer process and browser process. > > > > I rewrote the code on ToT, so maybe diff between PS1 and PS2 wouldn't make > > > > sense. Instead, "view" link of PS2 (diff between PS2 and ToT) is the > better > > to > > > > see. Though, I reused this CL to reply your comments. > > > > > > > > Thanks, > > > > - hidehiko > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > File components/nacl/browser/nacl_process_host.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:156: // Usually, > > NaClProcessHost > > > is > > > > working on IO thread, where file operation is > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > As an aside: I am sceptical that close() or CloseHandle() really block. > I > > > > think > > > > > this might be overzealousness on the part of the blocking checks in > base/. > > > > > > But > > > > > I don't have any hard data about this. > > > > > > > > According to my coworker who is the expert of Windows, CloseHandle does in > > > most > > > > cases practically (no spec), though close() on, e.g., linux, do not. > Though, > > > it > > > > depends on what resource we're releasing. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:159: // NaClProcessHost's > > > > destructor runnign on IO thread, because BlockingPool > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > "running" > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:161: void > CloseFile(base::File > > > > file) { > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > CloseFile() is a rather generic name. Maybe CloseFileInBlockingPool() > or > > > > > CloseFileOnFileThread()? > > > > > > > > Then, probably PostCloseFile etc. would be better. > > > > In chrome tree, OnSomeThread suffix is used for functions which run on the > > > > thread actually, rather than a function to post a task to the target > thread, > > > so > > > > maybe it is better to avoid using it here. > > > > > > > > Reflected in another CL. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:168: > > > > base::Bind(&(ignore_result<base::File>), base::Passed(&file))); > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"? > > (I'm > > > > not > > > > > sure.) > > > > > > > > Reflected in another CL. I just misunderstood it must be needed. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:178: > > > > CloseFile(IPC::PlatformFileForTransitToFile(file).Pass()); > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > I think this is unsafe on Windows, because this is a handle in another > > > > process. > > > > > See my comment below about ScopedSharedMemoryHandle. > > > > > > > > You're right. Fixed. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:252: // because there is > > neither > > > > operator== nor operator!= definition for > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Did you consider defining those operators on IPC::ChannelHandle? (I'm > not > > > > sure > > > > > what the trade off is here.) > > > > > > > > Defining operator== or operator!= needs to be in the base or IPC > namespace, > > > > because of ScopedGeneric implementation, even we have ADL. Our usage is > > subset > > > > of ChannelHandle now, and our implementation depends on that. So, I prefer > > not > > > > to use it to avoid making other namespace dirtier. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:276: // It is not allowed to > > > reset > > > > with the same (non-invalid) handle. > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Can you say why? > > > > > > > > Done. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:282: > !handle.socket.auto_close > > || > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Why check auto_close as part of the comparison? > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:284: #endif > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > "#else > > > > > #error Unknown platform"? > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:291: #if defined(OS_WIN) > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > How about adding a Close() method to IPC::ChannelHandle so that you > don't > > > need > > > > > conditionalisation here? > > > > > > > > > > Getting an ipc/ owners review for adding that would actually be quite > > > useful. > > > > > It would be good for an ipc/ owner to look at this change. :-) > > > > > > > > Again, our code depends on some constraint, so having such an API in > > > > ChannelHandle does not make sense. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:293: > > > > base::win::ScopedHandle(handle_.pipe.handle); > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Strangely, this *isn't* going through BlockingPool. > > > > > > > > > > It's kind of odd that you go to the trouble of using BlockingPool in > some > > > > cases > > > > > but not others. I realise that this is probably an artefact of some > > > Chromium > > > > > APIs being (unnecessarily?) stricter than others. > > > > > > > > > > Maybe this warrants a comment to explain the apparent inconsistency? > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:299: #endif > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Ditto: add an #else #error? > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/browser... > > > > components/nacl/browser/nacl_process_host.cc:778: ScopedSharedMemoryHandle > > > > crash_info_shmem_renderer_handle; > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > I think this is unsafe on Windows. > > > > > > > > > > Remember, on Windows, ShareToProcess() gives you a HANDLE that's an > index > > > into > > > > > another process's handle table. If you call CloseHandle() on it, > > > potentially > > > > > you're closing an unrelated handle in the current process, which might > be > > > > > exploitable. > > > > > > > > > > Unfortunately, SharedMemory::CloseHandle() doesn't check the return > value > > of > > > > > CloseHandle(), which is rather dangerous. > > > > > > > > > > On Windows, I think your options for error handling are: > > > > > > > > > > * Let the handle leak in the destination process. > > > > > * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle > > from > > > > the > > > > > destination process's handle table. Does Chromium actually do this > > anywhere > > > > on > > > > > an error-handling code path? This is somewhat risky given that error > code > > > > paths > > > > > are under-tested. > > > > > > > > You're right. Fixed. > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > > > > File components/nacl/common/nacl_types.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1094653003/diff/40001/components/nacl/common/... > > > > components/nacl/common/nacl_types.h:18: > > > > On 2015/04/17 22:13:45, Mark Seaborn wrote: > > > > > Nit: remove 1 empty line so that there's only 1 between blocks > > > > > > > > Done.
In the commit message, can you say "FDs/handles" rather than "resources", to be more specific, please? LGTM -- thanks. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:248: // class. This function is just a helper for the validation. Nit: "for validation"? (no "the") https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:744: // Hereafter, always we send an IPC message with handles which are not Nit: "we always" https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:745: // closable in this process. Nit: Clarify that the unclosability applies only to Windows: e.g. "...which, on Windows, are not closable in this process" https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:754: // On error, we do not send "IPC::ChannelHandle"s to the renderer process. Can you also add: "Some other FDs/handles still get sent to the renderer, but will be closed there." since that isn't obvious without looking at ppb_nacl_private_impl.cc. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:761: const ChildProcessData& data = process_->GetData(); Nit: if you're moving this, you could just use "process_->GetData().id" below. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:789: // Though, in the case, unfortunately there is no proper way to release Nit: "in the case" -> "in this case" https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:790: // resources which are already created in |result|. We just give up to Nit: "give up on releasing them" https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... File components/nacl/common/nacl_types.h (left): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... components/nacl/common/nacl_types.h:31: typedef HANDLE FileDescriptor; Thanks for removing this old stuff! https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... components/nacl/common/nacl_types.h:19: // TODO(gregoryd): add a Windows definition for base::FileDescriptor You can remove this comment too. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/render... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/render... components/nacl/renderer/ppb_nacl_private_impl.cc:474: // Even on error, some resources may be passed to here. "resources" -> "FDs/handles" to be more specific? https://codereview.chromium.org/1094653003/diff/140001/components/nacl/render... components/nacl/renderer/ppb_nacl_private_impl.cc:476: // See also nacl_process_host. Nit: "nacl_process_host.cc" (might as well add .cc for completeness)
Patchset #4 (id:200001) has been deleted
Thank you for review. Submitting. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:248: // class. This function is just a helper for the validation. On 2015/05/06 21:24:52, Mark Seaborn wrote: > Nit: "for validation"? (no "the") Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:744: // Hereafter, always we send an IPC message with handles which are not On 2015/05/06 21:24:52, Mark Seaborn wrote: > Nit: "we always" Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:745: // closable in this process. On 2015/05/06 21:24:53, Mark Seaborn wrote: > Nit: Clarify that the unclosability applies only to Windows: e.g. "...which, on > Windows, are not closable in this process" Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:754: // On error, we do not send "IPC::ChannelHandle"s to the renderer process. On 2015/05/06 21:24:52, Mark Seaborn wrote: > Can you also add: > > "Some other FDs/handles still get sent to the renderer, but will be closed > there." > > since that isn't obvious without looking at ppb_nacl_private_impl.cc. Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:761: const ChildProcessData& data = process_->GetData(); On 2015/05/06 21:24:52, Mark Seaborn wrote: > Nit: if you're moving this, you could just use "process_->GetData().id" below. Pls let me keep it as is. |data| is also used for base::GetProcId(data.handle), too. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:789: // Though, in the case, unfortunately there is no proper way to release On 2015/05/06 21:24:52, Mark Seaborn wrote: > Nit: "in the case" -> "in this case" Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:790: // resources which are already created in |result|. We just give up to On 2015/05/06 21:24:52, Mark Seaborn wrote: > Nit: "give up on releasing them" Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... File components/nacl/common/nacl_types.h (left): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... components/nacl/common/nacl_types.h:31: typedef HANDLE FileDescriptor; On 2015/05/06 21:24:53, Mark Seaborn wrote: > Thanks for removing this old stuff! Acknowledged. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... File components/nacl/common/nacl_types.h (right): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/common... components/nacl/common/nacl_types.h:19: // TODO(gregoryd): add a Windows definition for base::FileDescriptor On 2015/05/06 21:24:53, Mark Seaborn wrote: > You can remove this comment too. Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/render... File components/nacl/renderer/ppb_nacl_private_impl.cc (right): https://codereview.chromium.org/1094653003/diff/140001/components/nacl/render... components/nacl/renderer/ppb_nacl_private_impl.cc:474: // Even on error, some resources may be passed to here. On 2015/05/06 21:24:53, Mark Seaborn wrote: > "resources" -> "FDs/handles" to be more specific? Done. https://codereview.chromium.org/1094653003/diff/140001/components/nacl/render... components/nacl/renderer/ppb_nacl_private_impl.cc:476: // See also nacl_process_host. On 2015/05/06 21:24:53, Mark Seaborn wrote: > Nit: "nacl_process_host.cc" (might as well add .cc for completeness) Done.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mseaborn@chromium.org Link to the patchset: https://codereview.chromium.org/1094653003/#ps220001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1094653003/220001
Message was sent while issue was closed.
Committed patchset #4 (id:220001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b577bc48a00cd70600aa310eaf66faf3ec80758d Cr-Commit-Position: refs/heads/master@{#328698}
Message was sent while issue was closed.
https://codereview.chromium.org/1094653003/diff/220001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/220001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:746: IPC::TakeFileHandleForProcess(socket_for_renderer_.Pass(), Looking again, I noticed that you're not checking for an error here. Shouldn't this do the same as the ShareToProcess() call below, and set error_message etc. on error? If imc_handle_for_renderer is invalid, I don't think the later code paths check for that gracefully and abort the process's startup (i.e. LaunchSelLdr() in ppb_nacl_private_impl.cc and its caller in service_runtime.cc).
Message was sent while issue was closed.
https://codereview.chromium.org/1094653003/diff/220001/components/nacl/browse... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/1094653003/diff/220001/components/nacl/browse... components/nacl/browser/nacl_process_host.cc:746: IPC::TakeFileHandleForProcess(socket_for_renderer_.Pass(), On 2015/05/07 22:19:56, Mark Seaborn wrote: > Looking again, I noticed that you're not checking for an error here. > > Shouldn't this do the same as the ShareToProcess() call below, and set > error_message etc. on error? > > If imc_handle_for_renderer is invalid, I don't think the later code paths check > for that gracefully and abort the process's startup (i.e. LaunchSelLdr() in > ppb_nacl_private_impl.cc and its caller in service_runtime.cc). Ah, you're right. I'll send another CL. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
