Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2646623004:
Query NaCl processes' GDB debug port on the correct thread. (Closed)
DescriptionQuery NaCl processes' GDB debug port on the correct thread.
TaskGroup previously called NaClBrowser on the UI thread, to refresh the
GDB debug port information for NaCl processes. NaClBrowser is designed
to be accessed only from the Browser's IO thread; this patch fixes that.Fix NaCl debug stub port fetching in Task Manager.
This is a re-land of crrev.com/2634573003 which had a couple of issues.
BUG=630861, 684042
Review-Url: https://codereview.chromium.org/2646623004
Cr-Commit-Position: refs/heads/master@{#451188}
Committed: https://chromium.googlesource.com/chromium/src/+/91503efd94b63d9be85603e7d1d6edf7e837f4c9
Patch Set 1 #
Total comments: 1
Patch Set 2 : Remove optimization #
Total comments: 4
Patch Set 3 : Fix GetNaClPortText() expectation and rename new_visibility #
Total comments: 8
Patch Set 4 : Fix kBackgroundRefreshTypeMasks, which fixes git cl format'ting #Patch Set 5 : Pull changes from 2634573003 #
Total comments: 4
Patch Set 6 : Add TaskGroup unit-tests #
Total comments: 21
Patch Set 7 : Address review comments #Patch Set 8 : Fix typo #
Total comments: 6
Patch Set 9 : Address review comments #
Messages
Total messages: 54 (38 generated)
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wez@chromium.org changed reviewers: + afakhry@chromium.org
afakhry: Sorry, my previous patch was not quite correct (though not actually broken, thankfully ;). https://codereview.chromium.org/2646623004/diff/1/chrome/browser/task_manager... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/1/chrome/browser/task_manager... chrome/browser/task_manager/sampling/task_group.cc:185: nacl_debug_stub_port_ == nacl::kGdbDebugStubPortUnknown && bradnelson: I thought of adding this, but I think for it to work we would need NaClBrowser to return Unknown for the port number until the process checks-in, because of async port allocation under Windows - we'd probably want to hard-wire non-NACL process types to Unused and update the GetDebugGdbStubPort() API to return Unknown rather than Unused. Probably not worth optimizing this, though.. :P
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CL description still says: "prevents TaskGroup from repeatedly refreshing the port for a process" but I see you removed that part from the second patch. https://codereview.chromium.org/2646623004/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:93: nacl_debug_stub_port_(nacl::kGdbDebugStubPortUnknown), While we are here, can you please revise the expectation of the shown text here: https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... Are we showing the correct text? nacl::kGdbDebugStubPortUnknown shows the text "Unknown". nacl::kGdbDebugStubPortUnused shows the text N/A text "-". -2, which we use when NaCl debugging is disabled, shows the text "Disabled". Your change here doesn't change the behavior at all since nacl::kGdbDebugStubPortUnknown == -1, but just to verify we're showing the right meaningful thing. https://codereview.chromium.org/2646623004/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:261: static int GetNaClDebugStubPort(int process_id) { Can you please add a DCHECK_CURRENTLY_ON(content::BrowserThread::IO); to make it clear?
Description was changed from ========== Fix NaCl debug stub port fetching in Task Manager. Previous patch moved the actual fetching of the NaCl debug stub port to run on the IO thread, but still called NaClBrowser::GetInstance() from the UI thread, which is not valid. This fix also prevents TaskGroup from repeatedly refreshing the port for a process, since the port is stable once it has been set. BUG=630861 ========== to ========== Fix NaCl debug stub port fetching in Task Manager. Previous patch moved the actual fetching of the NaCl debug stub port to run on the IO thread, but still called NaClBrowser::GetInstance() from the UI thread, which is not valid. BUG=630861 ==========
On 2017/01/23 17:41:55, afakhry wrote: > The CL description still says: "prevents TaskGroup from repeatedly refreshing > the port for a process" but I see you removed that part from the second patch. Yes; it seemed to be a little too complex to be worth optimizing, given that NaCl debug stub port enabling is a developer-only feature.
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2646623004/diff/20001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:93: nacl_debug_stub_port_(nacl::kGdbDebugStubPortUnknown), On 2017/01/23 17:41:55, afakhry wrote: > While we are here, can you please revise the expectation of the shown text here: > https://cs.chromium.org/chromium/src/chrome/browser/ui/task_manager/task_mana... > > Are we showing the correct text? nacl::kGdbDebugStubPortUnknown shows the text > "Unknown". nacl::kGdbDebugStubPortUnused shows the text N/A text "-". -2, which > we use when NaCl debugging is disabled, shows the text "Disabled". > > Your change here doesn't change the behavior at all since > nacl::kGdbDebugStubPortUnknown == -1, but just to verify we're showing the right > meaningful thing. Hmmm, GetNaClPortText() doesn't need the -2 case, since it'll only be called if NaCl debug ports are enabled; I'll update the impl to remove -2. Unfortunately the enabled flag is not available to DCHECK there. That said, perhaps it would be cleaner to have the TaskGroup sampler code "know" that NaCl debug ports are disabled, and return kDisabled, kUnknown, kUnused, or a value, to simplify the table model implementation? https://codereview.chromium.org/2646623004/diff/20001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:261: static int GetNaClDebugStubPort(int process_id) { On 2017/01/23 17:41:55, afakhry wrote: > Can you please add a DCHECK_CURRENTLY_ON(content::BrowserThread::IO); to make it > clear? I'd prefer to express that via the name, since this is a static function sitting alongside its only caller, so a DCHECK seems overkill. https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL || Ick, I think this indentation got messed-up by git cl format in my previous CL for NaCL debug stub port - noticed a similar issue on another CL - will look into it. https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:188: #endif // !defined(DISABLE_NACL) nit: REFRESH_TYPE_NACL exists even in DISABLE_NACL, so should this code be either asserting that it's not set, or explicitly ignoring it in those builds? (Same applies to REFRESH_TYPE_HANDLES, above). https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:688: bool needs_refresh = visibility; I update this method not to bother enabling NaCl refresh if it is disabled, even if the column is displayed. Also renamed this variable since it appears to refer to whether or not the item should be refreshed, not whether it's visible. Is the TaskManagerTableModel unit-tested anywhere, or is that not feasible due to dependencies?
The CQ bit was checked by wez@chromium.org to run a CQ dry run
https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL || OK, worse than that, the indentation is missed up because of the || in the term I added in https://codereview.chromium.org/2634573003, which is of course wrong :( I'm surprised that this didn't break the Task Manager, since it would have left it waiting for _everything_ to refresh in the background?
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Fix NaCl debug stub port fetching in Task Manager. Previous patch moved the actual fetching of the NaCl debug stub port to run on the IO thread, but still called NaClBrowser::GetInstance() from the UI thread, which is not valid. BUG=630861 ========== to ========== Query NaCl processes' GDB debug port on the correct thread. TaskGroup previously called NaClBrowser on the UI thread, to refresh the GDB debug port information for NaCl processes. NaClBrowser is designed to be accessed only from the Browser's IO thread; this patch fixes that.Fix NaCl debug stub port fetching in Task Manager. This is a re-land of crrev.com/2634573003 which had a couple of issues. BUG=630861, 684042 ==========
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL | REFRESH_TYPE_PRIORITY; IIUC getting these wrong should mean that we never signal completion of the refresh back to the TaskManagerImpl, if one or more non-background refresh flags are set - I think we can add a TaskManagerImpl test-case for that, but adding one directly on TaskGroup might be cleaner?
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping!
On 2017/02/08 19:17:17, Wez wrote: > Ping! I'll take a look at it this evening.
https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:188: #endif // !defined(DISABLE_NACL) On 2017/01/23 19:21:02, Wez wrote: > nit: REFRESH_TYPE_NACL exists even in DISABLE_NACL, so should this code be > either asserting that it's not set, or explicitly ignoring it in those builds? > (Same applies to REFRESH_TYPE_HANDLES, above). We just used to ignore it since it used to be performed synchronously and we didn't need to wait for it in order to fire that all background calculations were performed. Now that it is performed on the background, it's still should be OK (provided that you do my comment on task_group.cc:34 on the last patch) even if the user requested REFRESH_TYPE_NACL while DISABLE_NACL. https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:688: bool needs_refresh = visibility; On 2017/01/23 19:21:02, Wez wrote: > I update this method not to bother enabling NaCl refresh if it is disabled, even > if the column is displayed. Also renamed this variable since it appears to refer > to whether or not the item should be refreshed, not whether it's visible. > > Is the TaskManagerTableModel unit-tested anywhere, or is that not feasible due > to dependencies? Check out the users of c/b/task_manager/task_manager_tester.cc it exposes the APIs of the TaskManagerTableModel to many browser_tests. It is not unit tested in isolation. https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL | REFRESH_TYPE_PRIORITY; On 2017/01/23 21:49:10, Wez wrote: > IIUC getting these wrong should mean that we never signal completion of the > refresh back to the TaskManagerImpl, if one or more non-background refresh flags > are set - I think we can add a TaskManagerImpl test-case for that, but adding > one directly on TaskGroup might be cleaner? Your understanding is correct. That's why you must keep the REFRESH_TYPE_NACL here inside an #if !defined(DISABLE_NACL) for the: expected_on_bg_done_flags_ = refresh_flags & kBackgroundRefreshTypesMask; that we do at the beginning of Refresh() to work properly if the user requested REFRESH_TYPE_NACL while nacl is disabled. Thanks for suggesting the test case. I think it would be easier to test TaskGroup directly. https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:263: } Nit: To keep the organization of the current code consistent, can you please move this to the anonymous namespace? Thanks for naming it ...OnIoThread(), this is clear now. I still think adding a DCHECK to assert running on IO thread is more future proof. https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/80001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:194: // Only called if NaCl debug stub ports are enabled. Can you please assert that by means of a DCHECK?
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL - have added some simple tests for sync & async cases. Overall the TaskGroup sync vs async behaviour seems fragile & confusing; callers in general can't know whether to expect "background" completion of a request. We could address that by having the TaskGroup expose the set of up-to-date flags, and having the TaskManager observer callbacks also include that, for example. https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_group.cc:188: #endif // !defined(DISABLE_NACL) On 2017/02/09 01:26:59, afakhry wrote: > On 2017/01/23 19:21:02, Wez wrote: > > nit: REFRESH_TYPE_NACL exists even in DISABLE_NACL, so should this code be > > either asserting that it's not set, or explicitly ignoring it in those builds? > > > (Same applies to REFRESH_TYPE_HANDLES, above). > > We just used to ignore it since it used to be performed synchronously and we > didn't need to wait for it in order to fire that all background calculations > were performed. > > Now that it is performed on the background, it's still should be OK (provided > that you do my comment on task_group.cc:34 on the last patch) even if the user > requested REFRESH_TYPE_NACL while DISABLE_NACL. Acknowledged. https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/ui/task_... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/40001/chrome/browser/ui/task_... chrome/browser/ui/task_manager/task_manager_table_model.cc:688: bool needs_refresh = visibility; On 2017/02/09 01:26:59, afakhry wrote: > On 2017/01/23 19:21:02, Wez wrote: > > I update this method not to bother enabling NaCl refresh if it is disabled, > even > > if the column is displayed. Also renamed this variable since it appears to > refer > > to whether or not the item should be refreshed, not whether it's visible. > > > > Is the TaskManagerTableModel unit-tested anywhere, or is that not feasible due > > to dependencies? > > Check out the users of c/b/task_manager/task_manager_tester.cc it exposes the > APIs of the TaskManagerTableModel to many browser_tests. > > It is not unit tested in isolation. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for adding the tests. I guess you were too focused on getting the tests to pass that some of my earlier comments were ignored! :D Anyways, regarding your below comment: > Overall the TaskGroup sync vs async behaviour seems fragile & confusing; callers > in general can't know whether to expect "background" completion of a request. > We could address that by having the TaskGroup expose the set of up-to-date > flags, and having the TaskManager observer callbacks also include that, for > example. We can expose that, but I used to prefer to keep this transparent to the clients, they shouldn't care. It's a decision made by the sampler and can change anytime (e.g. if we determine that some measurement has become very heavy that we need to background it). Clients shouldn't be dependent on that. I agree they're currently fragile, especially after we added the Shared sampler, things started to be a bit hairy. One idea is to make all background samplers provide a flag list of their supported capabilities, and that's the value we use to calculate the expected_on_bg_done_flags_. You don't have to do this as part of the CL. I can clean up this mess later. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL | REFRESH_TYPE_PRIORITY; You still need to wrap REFRESH_TYPE_NACL inside: #if !defined(DISABLE_NACL) #endif One of your tests should have caught this bug. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:187: expected_on_bg_done_flags_ &= ~REFRESH_TYPE_NACL; Thanks for adding this! Nit: Can you please remove the braces? https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:263: static int GetNaClDebugStubPortOnIoThread(int process_id) { No DCHECK for IO? Ideally the DCHECK should be placed inside NaClBrowser::GetProcessGdbDebugStubPort() so that no other clients of that code may repeat the same bug. What about moving this function to the anonymous namespace above? https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:62: process_id(), I think you should make the above two accessors static functions or anonymous-namespace functions. I find it weird that we're calling member functions in the constructor before the object is fully constructed. Besides they don't really access any member variable, which makes them good candidates for getting outside the class. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:89: TEST_F(TaskGroupTest, NullRefresh) { It would be really nice to add a comment above each test to simply explain what it's testing. Could you please do that? https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:118: REFRESH_TYPE_IDLE_WAKEUPS); IDLE_WAKEUPS is a background-calculated value and that what matters here. Done via the SharedSampler on Windows and the |worker_thread_sampler_| on other platforms. The name of the test as well as the comment are misleading. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:194: // Only called if NaCl debug stub ports are enabled. No DCHECK?
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #7 (id:120001) has been deleted
PTAL Re the question around the API: the TaskManager observer interface presently has separate synchronous and asynchronous completion notifications, but the latter will not be called if the request doesn't require asynchronous completion. My suggestion would be to rearrange things in terms of partial versus complete results - the observer API _may_ provide a "partial results" notification if there are both sync and async result to fetch, but will _always_ provide a "full results" notification (either immediately if only synchronous fields were requested, or after some delay if fields requiring background work were requested). The only change to the TaskGroup API would be to rename the background-tasks-are-done accessor to reflect that it actually returns whether all the fields requested were updated, regardless of whether they required background work. Finally, it struck me that the per-Task and per-TaskGroup background-updated fields can lead to a lot of thread switching, depending on how things get scheduled by the OS; would it make sense to batch that work up into a smaller number of background refresh tasks, similar to SharedSampler? https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL | REFRESH_TYPE_PRIORITY; On 2017/02/10 20:08:52, afakhry wrote: > You still need to wrap REFRESH_TYPE_NACL inside: > > #if !defined(DISABLE_NACL) > > #endif > > One of your tests should have caught this bug. Done. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:187: expected_on_bg_done_flags_ &= ~REFRESH_TYPE_NACL; On 2017/02/10 20:08:52, afakhry wrote: > Thanks for adding this! > > Nit: Can you please remove the braces? I thought there was a general preference for including braces, especially for nested if()'s, after the OpenSSL goto issue, but OK. :) Under what circumstances can a TaskGroup have zero tasks, out of interest? https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:263: static int GetNaClDebugStubPortOnIoThread(int process_id) { On 2017/02/10 20:08:52, afakhry wrote: > No DCHECK for IO? > > Ideally the DCHECK should be placed inside > NaClBrowser::GetProcessGdbDebugStubPort() so that no other clients of that code > may repeat the same bug. Yes, that's the change that prompted this patch, in fact. :) (see https://codereview.chromium.org/2630443003/). > What about moving this function to the anonymous namespace above? Done. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:62: process_id(), On 2017/02/10 20:08:52, afakhry wrote: > I think you should make the above two accessors static functions or > anonymous-namespace functions. I find it weird that we're calling member > functions in the constructor before the object is fully constructed. Besides > they don't really access any member variable, which makes them good candidates > for getting outside the class. Removed them in favour of calling the base::Process APIs directly, which seemed more readable, if anything. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:89: TEST_F(TaskGroupTest, NullRefresh) { On 2017/02/10 20:08:52, afakhry wrote: > It would be really nice to add a comment above each test to simply explain what > it's testing. Could you please do that? Done. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:118: REFRESH_TYPE_IDLE_WAKEUPS); On 2017/02/10 20:08:52, afakhry wrote: > IDLE_WAKEUPS is a background-calculated value and that what matters here. Done > via the SharedSampler on Windows and the |worker_thread_sampler_| on other > platforms. > > The name of the test as well as the comment are misleading. I've reworked the comment; I'd prefer to keep the test name as-is since the intention is specifically to test a value which will use the SharedSampler on platforms which use that. Ideally I'd like to have parameterized tests which run multiple times, each verifying a different set of flags, but this approach felt cleaner to get some basic tests running. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:194: // Only called if NaCl debug stub ports are enabled. On 2017/02/10 20:08:52, afakhry wrote: > No DCHECK? Nope; this class is really just a struct used to help generate strings; it doesn't actually know whether NaCl debug stub ports are enabled (unless it checks the command-line directly?) We could have this structure informed of whether they're enabled, but in that case we may as well move the Disabled string logic in here rather than DCHECK.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wez@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % 2 nits. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:187: expected_on_bg_done_flags_ &= ~REFRESH_TYPE_NACL; On 2017/02/13 01:22:19, Wez wrote: > On 2017/02/10 20:08:52, afakhry wrote: > > Thanks for adding this! > > > > Nit: Can you please remove the braces? > > I thought there was a general preference for including braces, especially for > nested if()'s, after the OpenSSL goto issue, but OK. :) > > Under what circumstances can a TaskGroup have zero tasks, out of interest? That shouldn't happen (See my comment on the unittest in the new patch). We can probably replace this if with a DCHECK. I think I added that as a sanity check before doing tasks_[0]. I *think* I wrote TaskGroup before I wrote TaskManagerImpl. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:263: static int GetNaClDebugStubPortOnIoThread(int process_id) { On 2017/02/13 01:22:19, Wez wrote: > On 2017/02/10 20:08:52, afakhry wrote: > > No DCHECK for IO? > > > > Ideally the DCHECK should be placed inside > > NaClBrowser::GetProcessGdbDebugStubPort() so that no other clients of that > code > > may repeat the same bug. > > Yes, that's the change that prompted this patch, in fact. :) (see > https://codereview.chromium.org/2630443003/). > > > What about moving this function to the anonymous namespace above? > > Done. Acknowledged. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:62: process_id(), On 2017/02/13 01:22:19, Wez wrote: > On 2017/02/10 20:08:52, afakhry wrote: > > I think you should make the above two accessors static functions or > > anonymous-namespace functions. I find it weird that we're calling member > > functions in the constructor before the object is fully constructed. Besides > > they don't really access any member variable, which makes them good candidates > > for getting outside the class. > > Removed them in favour of calling the base::Process APIs directly, which seemed > more readable, if anything. Acknowledged. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:118: REFRESH_TYPE_IDLE_WAKEUPS); On 2017/02/13 01:22:19, Wez wrote: > On 2017/02/10 20:08:52, afakhry wrote: > > IDLE_WAKEUPS is a background-calculated value and that what matters here. Done > > via the SharedSampler on Windows and the |worker_thread_sampler_| on other > > platforms. > > > > The name of the test as well as the comment are misleading. > > I've reworked the comment; I'd prefer to keep the test name as-is since the > intention is specifically to test a value which will use the SharedSampler on > platforms which use that. > > Ideally I'd like to have parameterized tests which run multiple times, each > verifying a different set of flags, but this approach felt cleaner to get some > basic tests running. Acknowledged. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:194: // Only called if NaCl debug stub ports are enabled. On 2017/02/13 01:22:20, Wez wrote: > On 2017/02/10 20:08:52, afakhry wrote: > > No DCHECK? > > Nope; this class is really just a struct used to help generate strings; it > doesn't actually know whether NaCl debug stub ports are enabled (unless it > checks the command-line directly?) > > We could have this structure informed of whether they're enabled, but in that > case we may as well move the Disabled string logic in here rather than DCHECK. No no, that's an overkill. I wrote this class exactly as you described, a stringifier. It shouldn't really care about whether NaCl debug is enabled or not. I was suggesting a DCHECK because the comment sounded to me like it's an error to call this function if NaCl debug is disabled, and comments really don't assert any assumptions. You can ignore this request. https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:117: // and via asynchronous refresh on others, so we just that that field always Nit: that that? https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:134: TEST_F(TaskGroupTest, NaclRefreshZeroTasks) { This shouldn't happen but it's good to have a coverage for it. TaskManagerImpl deletes the TaskGroup once it becomes empty. https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:195: Nit: Can you please remove this extra empty line?
https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:187: expected_on_bg_done_flags_ &= ~REFRESH_TYPE_NACL; On 2017/02/15 02:58:19, afakhry wrote: > On 2017/02/13 01:22:19, Wez wrote: > > On 2017/02/10 20:08:52, afakhry wrote: > > > Thanks for adding this! > > > > > > Nit: Can you please remove the braces? > > > > I thought there was a general preference for including braces, especially for > > nested if()'s, after the OpenSSL goto issue, but OK. :) > > > > Under what circumstances can a TaskGroup have zero tasks, out of interest? > > That shouldn't happen (See my comment on the unittest in the new patch). We can > probably replace this if with a DCHECK. I think I added that as a sanity check > before doing tasks_[0]. I *think* I wrote TaskGroup before I wrote > TaskManagerImpl. Looking at TaskManagerImpl, it's clear that TaskGroup is removed as soon as it is empty(), so I'm adding DCHECK(!empty()) to the top of the Refresh() call, as a sanity-check. https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/100001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:194: // Only called if NaCl debug stub ports are enabled. On 2017/02/15 02:58:19, afakhry wrote: > On 2017/02/13 01:22:20, Wez wrote: > > On 2017/02/10 20:08:52, afakhry wrote: > > > No DCHECK? > > > > Nope; this class is really just a struct used to help generate strings; it > > doesn't actually know whether NaCl debug stub ports are enabled (unless it > > checks the command-line directly?) > > > > We could have this structure informed of whether they're enabled, but in that > > case we may as well move the Disabled string logic in here rather than DCHECK. > > No no, that's an overkill. I wrote this class exactly as you described, a > stringifier. It shouldn't really care about whether NaCl debug is enabled or > not. > > I was suggesting a DCHECK because the comment sounded to me like it's an error > to call this function if NaCl debug is disabled, and comments really don't > assert any assumptions. Right, and that makes sense; it'd just be necessary for the stringified to know whether NaCl debug is enabled. > You can ignore this request. Yup, I will do that; agreed that it seems overkill. https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:117: // and via asynchronous refresh on others, so we just that that field always On 2017/02/15 02:58:19, afakhry wrote: > Nit: that that? Done. https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:134: TEST_F(TaskGroupTest, NaclRefreshZeroTasks) { On 2017/02/15 02:58:19, afakhry wrote: > This shouldn't happen but it's good to have a coverage for it. TaskManagerImpl > deletes the TaskGroup once it becomes empty. Yup, confirmed that; I'll replace this with a general EXPECT_DCHECK_DEATH test. https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/ui/task... File chrome/browser/ui/task_manager/task_manager_table_model.cc (right): https://codereview.chromium.org/2646623004/diff/150001/chrome/browser/ui/task... chrome/browser/ui/task_manager/task_manager_table_model.cc:195: On 2017/02/15 02:58:19, afakhry wrote: > Nit: Can you please remove this extra empty line? I could, but the comment applies to the function, not to the if(), so the blank line actually helps convey the scope.
The CQ bit was checked by wez@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org Link to the patchset: https://codereview.chromium.org/2646623004/#ps170001 (title: "Address review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 170001, "attempt_start_ts": 1487285069087460,
"parent_rev": "74853eec68cad708cd0b03745265df77cad6ec70", "commit_rev":
"91503efd94b63d9be85603e7d1d6edf7e837f4c9"}
Message was sent while issue was closed.
Description was changed from ========== Query NaCl processes' GDB debug port on the correct thread. TaskGroup previously called NaClBrowser on the UI thread, to refresh the GDB debug port information for NaCl processes. NaClBrowser is designed to be accessed only from the Browser's IO thread; this patch fixes that.Fix NaCl debug stub port fetching in Task Manager. This is a re-land of crrev.com/2634573003 which had a couple of issues. BUG=630861, 684042 ========== to ========== Query NaCl processes' GDB debug port on the correct thread. TaskGroup previously called NaClBrowser on the UI thread, to refresh the GDB debug port information for NaCl processes. NaClBrowser is designed to be accessed only from the Browser's IO thread; this patch fixes that.Fix NaCl debug stub port fetching in Task Manager. This is a re-land of crrev.com/2634573003 which had a couple of issues. BUG=630861, 684042 Review-Url: https://codereview.chromium.org/2646623004 Cr-Commit-Position: refs/heads/master@{#451188} Committed: https://chromium.googlesource.com/chromium/src/+/91503efd94b63d9be85603e7d1d6... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:170001) as https://chromium.googlesource.com/chromium/src/+/91503efd94b63d9be85603e7d1d6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
