Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11236025:
Test that debug stub works with browser. (Closed)
|
Created:
8 years, 2 months ago by halyavin Modified:
8 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, native-client-reviews_googlegroups.com, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionNaCl: Add test to check that GDB debug stub works in Chrome
Test runs empty NaCl module in a browser under debugging
and checks that it completes successfully when debugger
sends continue command.
BUG=157312
TEST= NaClGdbDebugStubRspTest.Empty
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171805
Patch Set 1 #Patch Set 2 : #Patch Set 3 : rebase #
Total comments: 26
Patch Set 4 : #
Total comments: 2
Patch Set 5 : rebase #
Total comments: 28
Patch Set 6 : #Patch Set 7 : raise exception for invalid test names #Patch Set 8 : add missing file #Patch Set 9 : remove W00 case - it can't happen #
Total comments: 14
Patch Set 10 : #
Total comments: 4
Patch Set 11 : #Patch Set 12 : #
Messages
Total messages: 23 (0 generated)
http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:3271: 'browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc', In general we list all sources and exclude where not appropriate.
http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... File chrome/browser/nacl_host/nacl_process_host.cc (right): http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... chrome/browser/nacl_host/nacl_process_host.cc:659: NaClBrowser* nacl_browser = NaClBrowser::GetInstance(); Instead of adding HasDebugStubPortListener() etc., could you just add global function pointer and do something like this here: if (g_replacement_socket_binder_for_testing) return g_replacement_socket_binder_for_testing() The new code in nacl_browser.{h,cc} seems like overkill. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... chrome/browser/nacl_host/nacl_process_host.cc:663: LOG(INFO) << "allocate ephemeral port"; These are too many LOG(INFO)s to commit. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... chrome/browser/nacl_host/nacl_process_host.cc:678: close(s); This is fixing an existing leak, right? A separate change would be better, but please at least note the fix in the commit message. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:10: assert 'W00' == connection.RspRequest('vCont;c') Please make sure this logs the actual reply if the assertion fails, to make this more easily debuggable. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:14: name = args[1] There's no need for a 'name' arg if you only support one test here. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... File chrome/browser/nacl_host/test/gdb_rsp.py (right): http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/gdb_rsp.py:1: # Copyright (c) 2012 The Native Client Authors. All rights reserved. Shouldn't have "The Native Client Authors" in the Chromium repo. Please add a comment that this is based on gdb_rsp.py from the NaCl repo. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/gdb_rsp.py:13: def EnsurePortIsAvailable(addr=SEL_LDR_RSP_SOCKET_ADDR): Please remove functions you're not using, like this one. http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:3269: ['OS=="linux"', { Why not Mac too? There should be a TODO to enable this for Windows eventually. You should probably file an issue to cover testing the debug stub in Chromium on all OSes. http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:79: int TCPListenSocket::GetBindedPort(SocketDescriptor s) { Should the return type be uint16 to match NetToHost16()? http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h File net/base/tcp_listen_socket.h (right): http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.h:28: static int GetBindedPort(SocketDescriptor s); GetBoundPort()? This needs a comment to explain that it returns the port number bound in the case where the OS chooses the port, when port=0 is passed to CreateAndBind(). Actually, it would be cleaner to instead add: static SocketDescriptor CreateAndBindAnyPort(const std::string& ip, int *port);
I only skimmed the CL, but in general I feel working directly with SocketDescriptor is not the right way here. http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h File net/base/tcp_listen_socket.h (right): http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.h:28: static int GetBindedPort(SocketDescriptor s); suggest: GetBoundPort. I'd generally prefer if all three of those methods were wrapped in a proper class and NaClProcessHost held an instance of a TCPListenSocket.
On 22 October 2012 19:08, <szym@chromium.org> wrote: > I only skimmed the CL, but in general I feel working directly with > SocketDescriptor is not the right way here. > > > > http://codereview.chromium.**org/11236025/diff/1011/net/** > base/tcp_listen_socket.h<http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h> > File net/base/tcp_listen_socket.h (right): > > http://codereview.chromium.**org/11236025/diff/1011/net/** > base/tcp_listen_socket.h#**newcode28<http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h#newcode28> > net/base/tcp_listen_socket.h:**28: static int > GetBindedPort(SocketDescriptor s); > suggest: GetBoundPort. > > I'd generally prefer if all three of those methods were wrapped in a > proper class and NaClProcessHost held an instance of a TCPListenSocket. > That doesn't seem appropriate because NaClProcessHost needs to pass the socket FD to the (sandboxed) NaCl process via IPC. I don't think an instance of TCPListenSocket lets you get at the underlying socket FD in order to do that, does it? Given our use of IPC to send FDs, using SocketDescriptors seems appropriate. Mark
On 22 October 2012 19:08, <szym@chromium.org> wrote: > I only skimmed the CL, but in general I feel working directly with > SocketDescriptor is not the right way here. > > > > http://codereview.chromium.**org/11236025/diff/1011/net/** > base/tcp_listen_socket.h<http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h> > File net/base/tcp_listen_socket.h (right): > > http://codereview.chromium.**org/11236025/diff/1011/net/** > base/tcp_listen_socket.h#**newcode28<http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h#newcode28> > net/base/tcp_listen_socket.h:**28: static int > GetBindedPort(SocketDescriptor s); > suggest: GetBoundPort. > > I'd generally prefer if all three of those methods were wrapped in a > proper class and NaClProcessHost held an instance of a TCPListenSocket. > That doesn't seem appropriate because NaClProcessHost needs to pass the socket FD to the (sandboxed) NaCl process via IPC. I don't think an instance of TCPListenSocket lets you get at the underlying socket FD in order to do that, does it? Given our use of IPC to send FDs, using SocketDescriptors seems appropriate. Mark -- You received this message because you are subscribed to the Google Groups "Native-Client-Reviews" group. To post to this group, send email to native-client-reviews@googlegroups.com. To unsubscribe from this group, send email to native-client-reviews+unsubscribe@googlegroups.com. For more options, visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... File chrome/browser/nacl_host/nacl_process_host.cc (right): http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... chrome/browser/nacl_host/nacl_process_host.cc:659: NaClBrowser* nacl_browser = NaClBrowser::GetInstance(); On 2012/10/23 01:14:50, Mark Seaborn wrote: > Instead of adding HasDebugStubPortListener() etc., could you just add global > function pointer and do something like this here: > > if (g_replacement_socket_binder_for_testing) > return g_replacement_socket_binder_for_testing() > > The new code in nacl_browser.{h,cc} seems like overkill. Test lives in an object. A function can't call an object unless I introduce one more global variable. Also global variables introduce inter-test dependencies. Another test can fail, if previous test forgot to clear some global variable. So I don't think this is a good idea. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... chrome/browser/nacl_host/nacl_process_host.cc:663: LOG(INFO) << "allocate ephemeral port"; On 2012/10/23 01:14:50, Mark Seaborn wrote: > These are too many LOG(INFO)s to commit. I inserted them for debugging and forgot to remove after upload. Done. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/na... chrome/browser/nacl_host/nacl_process_host.cc:678: close(s); On 2012/10/23 01:14:50, Mark Seaborn wrote: > This is fixing an existing leak, right? A separate change would be better, but > please at least note the fix in the commit message. Done. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:10: assert 'W00' == connection.RspRequest('vCont;c') On 2012/10/23 01:14:50, Mark Seaborn wrote: > Please make sure this logs the actual reply if the assertion fails, to make this > more easily debuggable. Done. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:14: name = args[1] On 2012/10/23 01:14:50, Mark Seaborn wrote: > There's no need for a 'name' arg if you only support one test here. This is only the first debugger test. We need more of them and I don't sure that we can always use only single continue command. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... File chrome/browser/nacl_host/test/gdb_rsp.py (right): http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/gdb_rsp.py:1: # Copyright (c) 2012 The Native Client Authors. All rights reserved. On 2012/10/23 01:14:50, Mark Seaborn wrote: > Shouldn't have "The Native Client Authors" in the Chromium repo. > > Please add a comment that this is based on gdb_rsp.py from the NaCl repo. Done. http://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/te... chrome/browser/nacl_host/test/gdb_rsp.py:13: def EnsurePortIsAvailable(addr=SEL_LDR_RSP_SOCKET_ADDR): On 2012/10/23 01:14:50, Mark Seaborn wrote: > Please remove functions you're not using, like this one. Done. http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:3269: ['OS=="linux"', { On 2012/10/23 01:14:50, Mark Seaborn wrote: > Why not Mac too? There should be a TODO to enable this for Windows eventually. > You should probably file an issue to cover testing the debug stub in Chromium on > all OSes. Done. http://codereview.chromium.org/11236025/diff/1011/chrome/chrome_tests.gypi#ne... chrome/chrome_tests.gypi:3271: 'browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc', On 2012/10/22 20:41:26, sky wrote: > In general we list all sources and exclude where not appropriate. Done. http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.cc File net/base/tcp_listen_socket.cc (right): http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.cc:79: int TCPListenSocket::GetBindedPort(SocketDescriptor s) { On 2012/10/23 01:14:50, Mark Seaborn wrote: > Should the return type be uint16 to match NetToHost16()? port numbers are generally saved in int in Chrome. Search shows that int for port variable is used 5 times more often than uint16. http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.h File net/base/tcp_listen_socket.h (right): http://codereview.chromium.org/11236025/diff/1011/net/base/tcp_listen_socket.... net/base/tcp_listen_socket.h:28: static int GetBindedPort(SocketDescriptor s); On 2012/10/23 01:14:50, Mark Seaborn wrote: > GetBoundPort()? This needs a comment to explain that it returns the port number > bound in the case where the OS chooses the port, when port=0 is passed to > CreateAndBind(). > > Actually, it would be cleaner to instead add: > > static SocketDescriptor CreateAndBindAnyPort(const std::string& ip, int > *port); Done.
Currently this test fails because it needs a change in NaCl which is located here: http://codereview.chromium.org/11189140/
NaCl change have been committed and pulled into chrome tree. Please, take another look.
Mostly good... https://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/n... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/n... chrome/browser/nacl_host/nacl_process_host.cc:659: NaClBrowser* nacl_browser = NaClBrowser::GetInstance(); On 2012/10/23 11:59:29, halyavin wrote: > On 2012/10/23 01:14:50, Mark Seaborn wrote: > > Instead of adding HasDebugStubPortListener() etc., could you just add global > > function pointer and do something like this here: > > > > if (g_replacement_socket_binder_for_testing) > > return g_replacement_socket_binder_for_testing() > > > > The new code in nacl_browser.{h,cc} seems like overkill. > > Test lives in an object. A function can't call an object unless > I introduce one more global variable. Also global variables > introduce inter-test dependencies. Another test can fail, if > previous test forgot to clear some global variable. So I don't > think this is a good idea. NaClBrowser::GetInstance() returns a global singleton argument, so the problem still applies. But maybe scoping the globals inside an object, as you've done, is cleaner. https://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/t... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/t... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:14: name = args[1] On 2012/10/23 11:59:29, halyavin wrote: > On 2012/10/23 01:14:50, Mark Seaborn wrote: > > There's no need for a 'name' arg if you only support one test here. > > This is only the first debugger test. We need more of them and I don't > sure that we can always use only single continue command. So you can add a 'name' arg in a later change when you need it. There's no need to complicate the code now. At a minimum, this code needs to raise an error if 'name' is not recognised. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_browser.h (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_browser.h:56: void DebugStubNewPortAllocated(int port); Please group these 4 methods together by removing empty lines between them, and add a comment that these are used for testing only. Also rename DebugStubNewPortAllocated so that the name matches up with the others, e.g. CallDebugStubPortListener(). Also, maybe s/DebugStub/GdbDebugStub/ to make this as obvious as possible to the uninitiated reader? https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:667: if (nacl_browser->HasDebugStubPortListener()) { Please add a comment to explain that this code path is used during testing only. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:682: close(s); Please CHECK that close() doesn't return an error https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:4: Nit: "tests" -> "test" in filename? There is only one test here. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:12: assert result == 'W00' or result == 'X09', result FYI, in Python you could write: assert result in ('W00', 'X09'), result But why are you accepting one of two results here? Can't you accept only one? If not, please comment why. Also commenting on what W00 and X09 mean would be good. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:21: Nit: 2 empty lines between top-level blocks https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/gdb_rsp.py (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/gdb_rsp.py:10: def RspChecksum(data): Please add a file-level comment to say this file is based on gdb_rsp.py from the NaCl repo. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:4: Maybe change the filename to "gdb_debug_stub_browsertest.cc"? "enable" doesn't add anything, and "nacl" is redundant given the location. Adding "gdb" makes this more obvious to newcomers. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:15: class NaClDebugStubRspTest : public PPAPINaClNewlibTest { Maybe "NaClGdbDebugStubTest" (along the lines of other comments)? https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:35: CommandLine cmd(FilePath(FILE_PATH_LITERAL("python"))); Maybe comment: We launch a Python script in order to reuse some existing Python code for testing GDB RSP connections. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:47: #if defined(ADDRESS_SANITIZER) Please comment this, if only to say that this is because all NaCl tests are disabled under ASan. https://codereview.chromium.org/11236025/diff/20002/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/20002/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:79: SocketDescriptor TCPListenSocket::CreateAndBindAnyPort(const std::string& ip, Nit: s/std::string/string/ to match the rest of the file https://codereview.chromium.org/11236025/diff/20002/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:89: LOG(ERROR) << "Could not determine binded port, getsockname() failed"; 'binded' -> 'bound'
https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_browser.h (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_browser.h:56: void DebugStubNewPortAllocated(int port); On 2012/12/04 21:46:42, Mark Seaborn wrote: > Please group these 4 methods together by removing empty lines between them, and > add a comment that these are used for testing only. Done > > Also rename DebugStubNewPortAllocated so that the name matches up with the > others, e.g. CallDebugStubPortListener(). I renamed it according to java conventions. > > Also, maybe s/DebugStub/GdbDebugStub/ to make this as obvious as possible to the > uninitiated reader? Done https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:667: if (nacl_browser->HasDebugStubPortListener()) { On 2012/12/04 21:46:42, Mark Seaborn wrote: > Please add a comment to explain that this code path is used during testing only. Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:682: close(s); On 2012/12/04 21:46:42, Mark Seaborn wrote: > Please CHECK that close() doesn't return an error I don't think we should kill release browser for this, so I added PLOG here. Search shows that this is more common approach than DPCHECK. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:4: On 2012/12/04 21:46:42, Mark Seaborn wrote: > Nit: "tests" -> "test" in filename? There is only one test here. I am going to add one or two more simple tests here. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:12: assert result == 'W00' or result == 'X09', result On 2012/12/04 21:46:42, Mark Seaborn wrote: > FYI, in Python you could write: > assert result in ('W00', 'X09'), result > > But why are you accepting one of two results here? Can't you accept only one? > If not, please comment why. Also commenting on what W00 and X09 mean would be > good. Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:21: On 2012/12/04 21:46:42, Mark Seaborn wrote: > Nit: 2 empty lines between top-level blocks Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/gdb_rsp.py (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/gdb_rsp.py:10: def RspChecksum(data): On 2012/12/04 21:46:42, Mark Seaborn wrote: > Please add a file-level comment to say this file is based on gdb_rsp.py from the > NaCl repo. Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:4: On 2012/12/04 21:46:42, Mark Seaborn wrote: > Maybe change the filename to "gdb_debug_stub_browsertest.cc"? "enable" doesn't > add anything, and "nacl" is redundant given the location. Adding "gdb" makes > this more obvious to newcomers. Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:15: class NaClDebugStubRspTest : public PPAPINaClNewlibTest { On 2012/12/04 21:46:42, Mark Seaborn wrote: > Maybe "NaClGdbDebugStubTest" (along the lines of other comments)? Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:35: CommandLine cmd(FilePath(FILE_PATH_LITERAL("python"))); On 2012/12/04 21:46:42, Mark Seaborn wrote: > Maybe comment: We launch a Python script in order to reuse some existing Python > code for testing GDB RSP connections. Done. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/nacl_enable_debug_stub_browsertest.cc:47: #if defined(ADDRESS_SANITIZER) On 2012/12/04 21:46:42, Mark Seaborn wrote: > Please comment this, if only to say that this is because all NaCl tests are > disabled under ASan. Done. https://codereview.chromium.org/11236025/diff/20002/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/20002/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:79: SocketDescriptor TCPListenSocket::CreateAndBindAnyPort(const std::string& ip, On 2012/12/04 21:46:42, Mark Seaborn wrote: > Nit: s/std::string/string/ to match the rest of the file Done. https://codereview.chromium.org/11236025/diff/20002/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:89: LOG(ERROR) << "Could not determine binded port, getsockname() failed"; On 2012/12/04 21:46:42, Mark Seaborn wrote: > 'binded' -> 'bound' Done.
https://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/t... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/1011/chrome/browser/nacl_host/t... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:14: name = args[1] On 2012/12/04 21:46:42, Mark Seaborn wrote: > On 2012/10/23 11:59:29, halyavin wrote: > > On 2012/10/23 01:14:50, Mark Seaborn wrote: > > > There's no need for a 'name' arg if you only support one test here. > > > > This is only the first debugger test. We need more of them and I don't > > sure that we can always use only single continue command. > > So you can add a 'name' arg in a later change when you need it. There's no need > to complicate the code now. We certainly need more than one test in chrome. So this is not a complication. There is no sense to remove it only to add it back a couple days later. > > At a minimum, this code needs to raise an error if 'name' is not recognised. Done
https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:12: assert result == 'W00' or result == 'X09', result I investigated further I found that NaCl test module doesn't exit, it is always killed by browser. So I removed W00 case.
LGTM with fixes. Please fill out a relevant issue number in BUG=. https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/11236025/diff/20002/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:682: close(s); On 2012/12/05 10:05:57, halyavin wrote: > On 2012/12/04 21:46:42, Mark Seaborn wrote: > > Please CHECK that close() doesn't return an error > > I don't think we should kill release browser for this, I disagree. EBADF from close() indicates a programming error as serious as a double free(), which should abort the process ASAP because it could be a security hole. But I am not going to push this here, because this is just one instance of close(), so you can leave it as it is. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_browser.h (right): https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_browser.h:56: // port listener, chrome will open ephemeral port for debug stub server 'chrome' -> 'Chrome'. I don't think "ephemeral" is the right adjective here. It's no more temporary than the normal GDB port. "Chrome will allocate a currently-unused TCP port number" would be better. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_browser.h:57: // instead of fixed one. "a fixed one" https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:667: // We allocate ephemeral port for debug stub tests. The port number is passed "ephemeral port" -> "currently unused TCP port" https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:12: # Once NaCl test module reports that test passed, NaCl embed tag is removed Nit: Add articles - "the NaCl <embed> element is removed" etc. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:25: raise AssertionError, "Unknown test name" raise AssertionError('Unknown test name: %r' % name) - use new-style exception constructor - use '' not "" - log the name https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:84: bool failed = getsockname(s, (struct sockaddr *) &addr, &addr_size) != 0; Use reinterpret_cast<> here
https://codereview.chromium.org/11236025/diff/12004/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/12004/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:81: SocketDescriptor s = CreateAndBind(ip, 0); You are passing port == 0 to |bind|, which invokes unspecified behavior, although it seems that most Unix stacks support this. I suggest that instead of adding custom |CreateAndBindAnyPort| and externally calling |listen|, you do: scoped_refptr<TCPListenSocket> sock = TCPListenSocket::CreateAndListen(ip, 0, NULL); IPEndPoint endpoint; GetLocalAddress(&endpoint); int port = endpoint.port(); https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:81: SocketDescriptor s = CreateAndBind(ip, 0); Check if (s == kInvalidSocket) and return early here. Otherwise, you will be closing the socket again.
https://codereview.chromium.org/11236025/diff/12004/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/12004/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:81: SocketDescriptor s = CreateAndBind(ip, 0); On 2012/12/05 18:21:37, szym wrote: > You are passing port == 0 to |bind|, which invokes unspecified behavior, > although it seems that most Unix stacks support this. > > I suggest that instead of adding custom |CreateAndBindAnyPort| and externally > calling |listen|, you do: > > scoped_refptr<TCPListenSocket> sock = > TCPListenSocket::CreateAndListen(ip, 0, NULL); > > IPEndPoint endpoint; > GetLocalAddress(&endpoint); > int port = endpoint.port(); Sorry, ignore this comment. I forgot to delete it.
https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_browser.h (right): https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_browser.h:56: // port listener, chrome will open ephemeral port for debug stub server On 2012/12/05 18:00:47, Mark Seaborn wrote: > 'chrome' -> 'Chrome'. > > I don't think "ephemeral" is the right adjective here. It's no more temporary > than the normal GDB port. "Chrome will allocate a currently-unused TCP port > number" would be better. Done. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_browser.h:57: // instead of fixed one. On 2012/12/05 18:00:47, Mark Seaborn wrote: > "a fixed one" Done. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... File chrome/browser/nacl_host/nacl_process_host.cc (right): https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/nacl_process_host.cc:667: // We allocate ephemeral port for debug stub tests. The port number is passed On 2012/12/05 18:00:47, Mark Seaborn wrote: > "ephemeral port" -> "currently unused TCP port" Done. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... File chrome/browser/nacl_host/test/debug_stub_browser_tests.py (right): https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:12: # Once NaCl test module reports that test passed, NaCl embed tag is removed On 2012/12/05 18:00:47, Mark Seaborn wrote: > Nit: Add articles - "the NaCl <embed> element is removed" etc. Done. https://codereview.chromium.org/11236025/diff/40003/chrome/browser/nacl_host/... chrome/browser/nacl_host/test/debug_stub_browser_tests.py:25: raise AssertionError, "Unknown test name" On 2012/12/05 18:00:47, Mark Seaborn wrote: > raise AssertionError('Unknown test name: %r' % name) > > - use new-style exception constructor > - use '' not "" > - log the name Done. https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:81: SocketDescriptor s = CreateAndBind(ip, 0); On 2012/12/05 18:21:37, szym wrote: > Check if (s == kInvalidSocket) and return early here. > > Otherwise, you will be closing the socket again. Done. https://codereview.chromium.org/11236025/diff/40003/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:84: bool failed = getsockname(s, (struct sockaddr *) &addr, &addr_size) != 0; On 2012/12/05 18:00:47, Mark Seaborn wrote: > Use reinterpret_cast<> here Done.
lgtm https://codereview.chromium.org/11236025/diff/42004/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/42004/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:82: if (s == kInvalidSocket) { nit: codestyle suggests no braces around single-line if bodies. https://codereview.chromium.org/11236025/diff/42004/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:89: if (addr_size != sizeof(addr)) { nit: ditto
https://codereview.chromium.org/11236025/diff/42004/net/base/tcp_listen_socke... File net/base/tcp_listen_socket.cc (right): https://codereview.chromium.org/11236025/diff/42004/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:82: if (s == kInvalidSocket) { On 2012/12/07 15:00:31, szym wrote: > nit: codestyle suggests no braces around single-line if bodies. Done. https://codereview.chromium.org/11236025/diff/42004/net/base/tcp_listen_socke... net/base/tcp_listen_socket.cc:89: if (addr_size != sizeof(addr)) { On 2012/12/07 15:00:31, szym wrote: > nit: ditto Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halyavin@google.com/11236025/48001
Presubmit check for 11236025-48001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Warnings ** License must match: .*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The Chromium Authors\. All rights reserved\.\n.*? Use of this source code is governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?: \*/)?\n Found a bad license header in these files: chrome/browser/nacl_host/test/debug_stub_browser_tests.py Presubmit checks took 1.5s to calculate. Was the presubmit check useful? Please send feedback & hate mail to maruel@chromium.org!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halyavin@google.com/11236025/57001
Message was sent while issue was closed.
Change committed as 171805 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
