Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
508493003:
Use WaitForDebuggerChildren to let renderer wait for gdb (Closed)
|
Created:
6 years, 3 months ago by Jaekyun Seok (inactive) Modified:
6 years, 3 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionUse WaitForDebuggerChildren to let renderer wait for gdb
BUG=378975
Committed: https://crrev.com/61aa9d539dc5892c5de53e081f9ce05ffa68f1d3
Cr-Commit-Position: refs/heads/master@{#293029}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Rename the flag to kRendererWaitForDebugger #
Total comments: 8
Patch Set 3 : Apply Charlie's comments #Patch Set 4 : Reuse kWaitForDebuggerChildren #Messages
Total messages: 24 (1 generated)
jaekyun@chromium.org changed reviewers: + feng@chromium.org
Please review this patch. Thanks,
https://codereview.chromium.org/508493003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/508493003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_process_host_impl.cc:644: cmd_line->AppendSwitch(switches::kWaitForDebugger); Where is the code in render process that pauses and waits for debugger?
https://codereview.chromium.org/508493003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/508493003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_process_host_impl.cc:644: cmd_line->AppendSwitch(switches::kWaitForDebugger); On 2014/08/27 15:50:46, Feng Qian wrote: > Where is the code in render process that pauses and waits for debugger? You can find it at https://cs.corp.google.com/#clankium/src/content/renderer/renderer_main.cc&sq... .
https://codereview.chromium.org/508493003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/508493003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_process_host_impl.cc:644: cmd_line->AppendSwitch(switches::kWaitForDebugger); On 2014/08/27 16:06:13, Jaekyun Seok wrote: > On 2014/08/27 15:50:46, Feng Qian wrote: > > Where is the code in render process that pauses and waits for debugger? > > You can find it at > https://cs.corp.google.com/#clankium/src/content/renderer/renderer_main.cc&sq... > . Acknowledged. https://codereview.chromium.org/508493003/diff/1/content/public/common/conten... File content/public/common/content_switches.h (right): https://codereview.chromium.org/508493003/diff/1/content/public/common/conten... content/public/common/content_switches.h:283: CONTENT_EXPORT extern const char kWaitForDebuggerRenderer[]; kRendererWaitForDebugger is the format we use.
PTAL. https://codereview.chromium.org/508493003/diff/1/content/public/common/conten... File content/public/common/content_switches.h (right): https://codereview.chromium.org/508493003/diff/1/content/public/common/conten... content/public/common/content_switches.h:283: CONTENT_EXPORT extern const char kWaitForDebuggerRenderer[]; On 2014/08/27 17:52:02, Feng Qian wrote: > kRendererWaitForDebugger is the format we use. Done.
jaekyun@chromium.org changed reviewers: + aelias@chromium.org, creis@chromium.org
lgtm
lgtm
The CQ bit was checked by jaekyun@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jaekyun@chromium.org/508493003/20001
The CQ bit was unchecked by jaekyun@chromium.org
Charlie, could you please review changes on content_switches.*?
Isn't this redundant with kWaitForDebuggerChildren? https://codereview.chromium.org/508493003/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/508493003/diff/20001/content/browser/renderer... content/browser/renderer_host/render_process_host_impl.cc:644: cmd_line->AppendSwitch(switches::kWaitForDebugger); This belongs in PropagateBrowserCommandLineToRenderer, not here. https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... File content/public/common/content_switches.h (right): https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... content/public/common/content_switches.h:237: CONTENT_EXPORT extern const char kWaitForDebuggerChildren[]; How does the new flag differ from this one? https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... content/public/common/content_switches.h:288: CONTENT_EXPORT extern const char kRendererWaitForDebugger[]; See the comment below. Please alphabetize this above.
If kWaitForDebuggerChildren is used to block renderer process, other processes can be blocked unexpectedly. So I want to add a new flag only for renderer process. https://codereview.chromium.org/508493003/diff/20001/content/browser/renderer... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/508493003/diff/20001/content/browser/renderer... content/browser/renderer_host/render_process_host_impl.cc:644: cmd_line->AppendSwitch(switches::kWaitForDebugger); On 2014/08/29 00:04:24, Charlie Reis wrote: > This belongs in PropagateBrowserCommandLineToRenderer, not here. Done. https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... File content/public/common/content_switches.h (right): https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... content/public/common/content_switches.h:237: CONTENT_EXPORT extern const char kWaitForDebuggerChildren[]; On 2014/08/29 00:04:24, Charlie Reis wrote: > How does the new flag differ from this one? Maybe we can reuse it, but it is a flag for all child processes. So, setting it to let renderer process wait for a debugger may affect other processes unexpectedly. https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... content/public/common/content_switches.h:288: CONTENT_EXPORT extern const char kRendererWaitForDebugger[]; On 2014/08/29 00:04:24, Charlie Reis wrote: > See the comment below. Please alphabetize this above. Done.
https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... File content/public/common/content_switches.h (right): https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... content/public/common/content_switches.h:237: CONTENT_EXPORT extern const char kWaitForDebuggerChildren[]; We should reuse it, render is a child process. Right now, only child process is renderer process. On 2014/08/29 01:05:07, Jaekyun Seok wrote: > On 2014/08/29 00:04:24, Charlie Reis wrote: > > How does the new flag differ from this one? > > Maybe we can reuse it, but it is a flag for all child processes. > > So, setting it to let renderer process wait for a debugger may affect other > processes unexpectedly.
https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... File content/public/common/content_switches.h (right): https://codereview.chromium.org/508493003/diff/20001/content/public/common/co... content/public/common/content_switches.h:237: CONTENT_EXPORT extern const char kWaitForDebuggerChildren[]; I didn't know that renderer process is only one, and I realized that kWaitForDebuggerChildren can have a value to determine if the child process should have the kWaitForDebugger flag passed on or not. So I will reuse it.
PTAL. I modified this patch to use WaitForDebuggerChildren to let renderer wait for gdb.
lgtm
The CQ bit was checked by jaekyun@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jaekyun@chromium.org/508493003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as ec33c24e8c9c2fdcbb343d6f4b4e114a1d64da97
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/61aa9d539dc5892c5de53e081f9ce05ffa68f1d3 Cr-Commit-Position: refs/heads/master@{#293029} |
