Query NaCl processes' GDB debug port on the correct thread.

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.

BUG=630861
Review-Url: https://codereview.chromium.org/2634573003
Cr-Commit-Position: refs/heads/master@{#444849}
Committed: https://chromium.googlesource.com/chromium/src/+/f0bd838cce8870a95aec33be0b58151bd80f96aa

[Wez, 2017-01-13 18:59:10]

wez@chromium.org changed reviewers:
+ afakhry@chromium.org, bradnelson@chromium.org

[Wez, 2017-01-13 18:59:10]

afakhry: PTAL as TaskManager OWNER. Implementing this, I notice that each
TaskGroup seems to post its own tasks to do refresh I/O for various fields - we
should be able to improve TaskManager performance by posting a single task to
e.g. the FILE or IO thread that will update the relevant fields for all
TaskGroup fields, similarly to the way SharedSampler works. WDYT?

bradnelson: IIUC the NaCl DBG port is set at process startup, so we should be
able to optimize this case by only calling to NaClBrowser when we see a new NaCl
process for which we don't yet have a debug port number set, right?

[Wez, 2017-01-13 18:59:17]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-13 18:59:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bradnelson, 2017-01-13 19:21:43]

That's correct the port is only set at startup (though it isn't immediately
known on windows (so there might be a need to retry), because on windows the
nacl process opens the socket, and thus requires running with no-sandbox.).

Ideally we'd marshall data and have the socket managed outside the sandbox. We
had an intern looking at that a couple years ago, but he didn't quite finish and
we had to back it out.

[bradnelson, 2017-01-13 19:21:48]

lgtm

[Wez, 2017-01-13 19:46:27]

bradnelson: Assuming that we can distinguish NaCl and non-NaCl processes in
TaskGroup then how about TaskGroup refreshes only until it sees a valid
port value - I assume that if we have NaCl debugging enabled then every
process will get assigned a port?

On 13 January 2017 at 11:21, <bradnelson@chromium.org> wrote:

> lgtm
>
>
>
> https://codereview.chromium.org/2634573003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-01-13 20:52:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 20:52:24]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-01-19 20:10:37]

afahkry: Pingy!

[afakhry, 2017-01-19 20:58:45]

Oh, sorry, I thought I already reviewed this.

LGTM

[Wez, 2017-01-19 20:59:20]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2017-01-19 20:59:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 21:47:13]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484859560642540, "parent_rev":
"93587fe0a3c5f5e8051e15842f77c0935fc12627", "commit_rev":
"f0bd838cce8870a95aec33be0b58151bd80f96aa"}

[commit-bot: I haz the power, 2017-01-19 21:47:42]

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.

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.

BUG=630861

Review-Url: https://codereview.chromium.org/2634573003
Cr-Commit-Position: refs/heads/master@{#444849}
Committed:
https://chromium.googlesource.com/chromium/src/+/f0bd838cce8870a95aec33be0b58...
==========

[commit-bot: I haz the power, 2017-01-19 21:47:43]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/f0bd838cce8870a95aec33be0b58...

[Wez, 2017-01-23 19:58:02]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/2649063003/ by wez@chromium.org.

The reason for reverting is: This fix introduced a typo in the background
refresh flags mask, breaking it..

[afakhry, 2017-01-23 20:58:38]

https://codereview.chromium.org/2634573003/diff/1/chrome/browser/task_manager...
File chrome/browser/task_manager/sampling/task_group.cc (right):

https://codereview.chromium.org/2634573003/diff/1/chrome/browser/task_manager...
chrome/browser/task_manager/sampling/task_group.cc:34: REFRESH_TYPE_NACL ||
I should have caught this, sorry.