Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
198083006:
Update the task manager with the debug stub port chosen by nacl. (Closed)
|
Created:
6 years, 9 months ago by bradn Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate the task manager with the debug stub port chosen by nacl.
On mac/linux the debug stub port is allocated chrome side and provided to
Native Client. On Windows (where socket handles cannot be passed between
processes), the debug port must be allocated by the nacl process (which can
only happen when the outer sandbox is disable).
Propagate the debug port selected by the nacl side to the task manager to make
it available to the user and debugging extensions.
Depends on this nacl side change:
https://codereview.chromium.org/206493005
BUG=328714
TEST=None
R=mseaborn@chromium.org,jschuh@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269499
Patch Set 1 #
Total comments: 12
Patch Set 2 : fix #
Total comments: 19
Patch Set 3 : fixes #
Total comments: 2
Patch Set 4 : fix #
Messages
Total messages: 23 (0 generated)
Depends on the nacl side change.
https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.cc:913: // This method is called when NaClProcessHostMsg_OnDebugStubPortSelected is It's NaClProcessHostMsg_DebugStubPortSelected. No "On". But isn't this comment a bit redundant given the naming coventions? https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:136: // Mark the process as using a particular debug stub port and notify Nit: if you say "GDB debug stub" in the comments it provides a little more context to unfamiliar readers. https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:138: void ChangeDebugStubPort(int port); uint16_t for consistency with other cases? Maybe s/Change/Set/? https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:167: // Called when the debug stub port has be selected. "has been" https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:168: void OnDebugStubPortSelected(uint16_t debug_stub_port); Nit: Can you keep the decl ordering the same as in the .cc file? Same for ChangeDebugStubPort(). https://codereview.chromium.org/198083006/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/198083006/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_listener.cc:87: NaClListener* g_listener; Shouldn't this be unconditional? Doesn't look like this will compile.
Ping -- are you working on this still?
ptal https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.cc:913: // This method is called when NaClProcessHostMsg_OnDebugStubPortSelected is On 2014/03/26 23:34:04, Mark Seaborn wrote: > It's NaClProcessHostMsg_DebugStubPortSelected. No "On". But isn't this comment > a bit redundant given the naming coventions? Dropped comment, but the name is this way for consistency with the other message handlers, all starting with 'On' (ie its a message name). https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:136: // Mark the process as using a particular debug stub port and notify On 2014/03/26 23:34:04, Mark Seaborn wrote: > Nit: if you say "GDB debug stub" in the comments it provides a little more > context to unfamiliar readers. Done. https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:138: void ChangeDebugStubPort(int port); On 2014/03/26 23:34:04, Mark Seaborn wrote: > uint16_t for consistency with other cases? Maybe s/Change/Set/? Done. https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:167: // Called when the debug stub port has be selected. On 2014/03/26 23:34:04, Mark Seaborn wrote: > "has been" Done. https://codereview.chromium.org/198083006/diff/1/components/nacl/browser/nacl... components/nacl/browser/nacl_process_host.h:168: void OnDebugStubPortSelected(uint16_t debug_stub_port); On 2014/03/26 23:34:04, Mark Seaborn wrote: > Nit: Can you keep the decl ordering the same as in the .cc file? Same for > ChangeDebugStubPort(). Done. https://codereview.chromium.org/198083006/diff/1/components/nacl/loader/nacl_... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/198083006/diff/1/components/nacl/loader/nacl_... components/nacl/loader/nacl_listener.cc:87: NaClListener* g_listener; On 2014/03/26 23:34:04, Mark Seaborn wrote: > Shouldn't this be unconditional? Doesn't look like this will compile. Oops. Done. Not sure why it ended up like that.
https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:649: IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected, FYI, this needs rebasing. I changed this method yesterday. :-) Can you make this conditional on OS_WIN? https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:936: ChangeDebugStubPort(debug_stub_port); Can you add: CHECK(!uses_nonsfi_mode_) to make this consistent with the other methods (after rebasing on the change I made yesterday). https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... components/nacl/browser/nacl_process_host.h:111: void ChangeDebugStubPort(uint16_t port); Can you rename "Change" to "Set", please? This is just a setter. "Change" sounds odd for a setter. https://codereview.chromium.org/198083006/diff/20001/components/nacl/common/n... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/common/n... components/nacl/common/nacl_messages.h:99: // Notify the browser process that the debug stub port has been selected. How about "Notify the browser process that the NaCl process has bound the given TCP port number to use for the GDB debug stub". (i.e. Avoid passive voice; mention GDB and TCP for context.) https://codereview.chromium.org/198083006/diff/20001/components/nacl/common/n... components/nacl/common/nacl_messages.h:100: IPC_MESSAGE_CONTROL1(NaClProcessHostMsg_DebugStubPortSelected, This can be conditionalised on OS_WIN, and go after AttachDebugExceptionHandler. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (left): https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:90: NaClListener* g_listener; This should stay conditional on OS_WIN. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:214: #if defined(OS_WIN) These conditionals can stay. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:115: void DebugStubPortSelectedHandler(uint16_t port) { Also only needed on OS_WIN. Currently you have werrors under Clang about DebugStubPortSelectedHandler being defined but not used. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:460: args->debug_stub_server_port_selected_handler_func = Nit: maybe put this after "args->attach_debug_exception_handler_func = ..." since it's less important?
Ok, more sane on when windows stuff is gated in. SetDebugStubPort is present on all platforms, but invoked differently. Everything windows specific, including the messages are gated out on non-windows. PTAL https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:649: IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected, On 2014/05/08 15:52:23, Mark Seaborn wrote: > FYI, this needs rebasing. I changed this method yesterday. :-) > > Can you make this conditional on OS_WIN? Done. https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:936: ChangeDebugStubPort(debug_stub_port); On 2014/05/08 15:52:23, Mark Seaborn wrote: > Can you add: CHECK(!uses_nonsfi_mode_) to make this consistent with the other > methods (after rebasing on the change I made yesterday). Done. https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.h (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/browser/... components/nacl/browser/nacl_process_host.h:111: void ChangeDebugStubPort(uint16_t port); On 2014/05/08 15:52:23, Mark Seaborn wrote: > Can you rename "Change" to "Set", please? This is just a setter. "Change" > sounds odd for a setter. It has side effects in that it triggers listeners, so I was trying to convey that. But I suppose 'Set' is fine too. Changed. https://codereview.chromium.org/198083006/diff/20001/components/nacl/common/n... File components/nacl/common/nacl_messages.h (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/common/n... components/nacl/common/nacl_messages.h:99: // Notify the browser process that the debug stub port has been selected. On 2014/05/08 15:52:23, Mark Seaborn wrote: > How about "Notify the browser process that the NaCl process has bound the given > TCP port number to use for the GDB debug stub". (i.e. Avoid passive voice; > mention GDB and TCP for context.) Done. https://codereview.chromium.org/198083006/diff/20001/components/nacl/common/n... components/nacl/common/nacl_messages.h:100: IPC_MESSAGE_CONTROL1(NaClProcessHostMsg_DebugStubPortSelected, On 2014/05/08 15:52:23, Mark Seaborn wrote: > This can be conditionalised on OS_WIN, and go after AttachDebugExceptionHandler. Done. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (left): https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:90: NaClListener* g_listener; On 2014/05/08 15:52:23, Mark Seaborn wrote: > This should stay conditional on OS_WIN. Er, ok, back again... https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:214: #if defined(OS_WIN) On 2014/05/08 15:52:23, Mark Seaborn wrote: > These conditionals can stay. Done. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:115: void DebugStubPortSelectedHandler(uint16_t port) { On 2014/05/08 15:52:23, Mark Seaborn wrote: > Also only needed on OS_WIN. > > Currently you have werrors under Clang about DebugStubPortSelectedHandler being > defined but not used. You've flip flopped here, in the previous version you seemed to be pushing for these existing on all platforms, but conditional seems more consistent anyhow. Switching. https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:460: args->debug_stub_server_port_selected_handler_func = On 2014/05/08 15:52:23, Mark Seaborn wrote: > Nit: maybe put this after "args->attach_debug_exception_handler_func = ..." > since it's less important? Done.
LGTM https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... File components/nacl/loader/nacl_listener.cc (right): https://codereview.chromium.org/198083006/diff/20001/components/nacl/loader/n... components/nacl/loader/nacl_listener.cc:115: void DebugStubPortSelectedHandler(uint16_t port) { On 2014/05/08 17:00:24, bradn wrote: > On 2014/05/08 15:52:23, Mark Seaborn wrote: > > Also only needed on OS_WIN. > > > > Currently you have werrors under Clang about DebugStubPortSelectedHandler > > being defined but not used. > > You've flip flopped here, in the previous version you seemed to be pushing for > these existing on all platforms, but conditional seems more consistent anyhow. > Switching. To be fair, you sent me a change that didn't compile. :-p So I made a very quick suggestion without looking much at the context. https://codereview.chromium.org/198083006/diff/70001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/198083006/diff/70001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:948: void NaClProcessHost::OnDebugStubPortSelected(uint16_t debug_stub_port) { Nit: Can you try to keep the same ordering as in the header, so put this after GetDebugStubSocketHandle()?
thanks:-) https://codereview.chromium.org/198083006/diff/70001/components/nacl/browser/... File components/nacl/browser/nacl_process_host.cc (right): https://codereview.chromium.org/198083006/diff/70001/components/nacl/browser/... components/nacl/browser/nacl_process_host.cc:948: void NaClProcessHost::OnDebugStubPortSelected(uint16_t debug_stub_port) { On 2014/05/08 18:04:44, Mark Seaborn wrote: > Nit: Can you try to keep the same ordering as in the header, so put this after > GetDebugStubSocketHandle()? Done.
The CQ bit was checked by bradnelson@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bradnelson@google.com/198083006/90001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. 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...)
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...)
The CQ bit was checked by bradnelson@google.com
Adding jschuh for OWNERS on component/nacl/common/*.h
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bradnelson@google.com/198083006/90001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
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...)
ipc security lgtm
The CQ bit was checked by bradnelson@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bradnelson@google.com/198083006/90001
Message was sent while issue was closed.
Change committed as 269499 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
