Linux Sandbox: Stop GPU watchdog in accountable way.

Linux Sandbox: Stop GPU watchdog in accountable way.

The Linux sandbox can sometimes detect a spurious running thread if it
has just been stopped.

We add a new LinuxSandbox::StopThread() method to safely stop threads
and make sure they won't be counted as still running.

BUG=328620
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249937

[jln (very slow on Chromium), 2014-02-07 21:57:36]

Jorge, can you please take a look?

[Jorge Lucangeli Obes, 2014-02-07 22:30:34]

https://codereview.chromium.org/147203005/diff/70001/content/common/sandbox_l...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://codereview.chromium.org/147203005/diff/70001/content/common/sandbox_l...
content/common/sandbox_linux/sandbox_linux.cc:197: // Using DCHECK here would be
incorrect. DCHECK can be enabled in non
Are you trying to always keep the check?

https://codereview.chromium.org/147203005/diff/70001/content/common/sandbox_l...
content/common/sandbox_linux/sandbox_linux.cc:198: // official release mode.
nit: non-official.

https://codereview.chromium.org/147203005/diff/70001/content/common/sandbox_l...
File content/common/sandbox_linux/sandbox_linux.h (right):

https://codereview.chromium.org/147203005/diff/70001/content/common/sandbox_l...
content/common/sandbox_linux/sandbox_linux.h:51: // This function should only be
called without any thread running.
nit: threads.

https://codereview.chromium.org/147203005/diff/70001/sandbox/linux/services/t...
File sandbox/linux/services/thread_helpers.cc (right):

https://codereview.chromium.org/147203005/diff/70001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.cc:72: // Crash after 30 iterations, which
means having spent roughly 2s in
This seems high, but maybe it doesn't matter in practice.

https://codereview.chromium.org/147203005/diff/70001/sandbox/linux/services/t...
File sandbox/linux/services/thread_helpers.h (right):

https://codereview.chromium.org/147203005/diff/70001/sandbox/linux/services/t...
sandbox/linux/services/thread_helpers.h:23: // way to stop threads before
calling IsSingleThreaded().
"This is way"?

[jln (very slow on Chromium), 2014-02-07 22:53:06]

Thanks, PTAL!

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.cc:197: // Using DCHECK here would be
incorrect. DCHECK can be enabled in non
On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> Are you trying to always keep the check?

The check needs to match exactly the cases where we do always have proc_fd_
available, which is only !NDEBUG (for security reasons). We may update this soon
for ASAN.

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.cc:198: // official release mode.
On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> nit: non-official.

Done.

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.h:51: // This function should only be
called without any thread running.
On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> nit: threads.

I think this is correct (we had that discussion before IIRC).

Example: Google fight between "without any cars" and "without any car".

https://chromiumcodereview.appspot.com/147203005/diff/70001/sandbox/linux/ser...
File sandbox/linux/services/thread_helpers.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.cc:72: // Crash after 30 iterations, which
means having spent roughly 2s in
On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> This seems high, but maybe it doesn't matter in practice.

Yeah, I wanted something ridiculously high before crashing. The DCHECK below is
here to check that it's very fast, as expected.

https://chromiumcodereview.appspot.com/147203005/diff/70001/sandbox/linux/ser...
File sandbox/linux/services/thread_helpers.h (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/sandbox/linux/ser...
sandbox/linux/services/thread_helpers.h:23: // way to stop threads before
calling IsSingleThreaded().
On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> "This is way"?

Done.

[Jorge Lucangeli Obes, 2014-02-08 00:07:11]

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.cc:197: // Using DCHECK here would be
incorrect. DCHECK can be enabled in non
On 2014/02/07 22:53:07, jln wrote:
> On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> > Are you trying to always keep the check?
> 
> The check needs to match exactly the cases where we do always have proc_fd_
> available, which is only !NDEBUG (for security reasons). We may update this
soon
> for ASAN.

I ask because
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=468
suggests !NDEBUG is enough to guarantee DCHECK will be there.

Anyhow, I found the CHECK confusing because the comment suggests you are not
using DCHECK because DCHECK could be enabled in situations where you don't want
it enabled, but CHECK is gonna be enabled in all situations anyways.

I.e. IIUC the only reason to use CHECK over DCHECK is DCHECK being disabled, not
DCHECK being enabled -- the set "builds where DCHECK is enabled" is strictly
included in the set "builds where CHECK is enabled".

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.h (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.h:51: // This function should only be
called without any thread running.
On 2014/02/07 22:53:07, jln wrote:
> On 2014/02/07 22:30:34, Jorge Lucangeli Obes wrote:
> > nit: threads.
> 
> I think this is correct (we had that discussion before IIRC).
> 
> Example: Google fight between "without any cars" and "without any car".

Heh we might have. No preference, first one sounds better to me but it's a nit.

https://chromiumcodereview.appspot.com/147203005/diff/280001/sandbox/linux/se...
File sandbox/linux/services/thread_helpers.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/280001/sandbox/linux/se...
sandbox/linux/services/thread_helpers.cc:78: DCHECK_GT(25U, iterations);
I'm sorry, I wasn't clear. I'm OK with leaving this as 10 iterations -- I
actually think it's better. I was asking about the 30 iterations for CHECK.

[jln (very slow on Chromium), 2014-02-08 00:13:41]

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.cc:197: // Using DCHECK here would be
incorrect. DCHECK can be enabled in non

> I.e. IIUC the only reason to use CHECK over DCHECK is DCHECK being disabled,
not
> DCHECK being enabled -- the set "builds where DCHECK is enabled" is strictly
> included in the set "builds where CHECK is enabled".

The comment pertains to using if "!defined(NDEBUG)" + CHECK rather than a
DCHECK.

This define has to match line 136 exactly. I want to check in exactly all cases
where line 136 is built.

https://chromiumcodereview.appspot.com/147203005/diff/280001/sandbox/linux/se...
File sandbox/linux/services/thread_helpers.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/280001/sandbox/linux/se...
sandbox/linux/services/thread_helpers.cc:78: DCHECK_GT(25U, iterations);
On 2014/02/08 00:07:12, Jorge Lucangeli Obes wrote:
> I'm sorry, I wasn't clear. I'm OK with leaving this as 10 iterations -- I
> actually think it's better. I was asking about the 30 iterations for CHECK.

Ohh you were clear, I changed it just to be safe. The microsecond metric was a
bit misleading anyway because of the expected clock granularity.

[Jorge Lucangeli Obes, 2014-02-08 00:24:21]

lgtm % comment nit.

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.cc:197: // Using DCHECK here would be
incorrect. DCHECK can be enabled in non
On 2014/02/08 00:13:41, jln wrote:
> 
> > I.e. IIUC the only reason to use CHECK over DCHECK is DCHECK being disabled,
> not
> > DCHECK being enabled -- the set "builds where DCHECK is enabled" is strictly
> > included in the set "builds where CHECK is enabled".
> 
> The comment pertains to using if "!defined(NDEBUG)" + CHECK rather than a
> DCHECK.
> 
> This define has to match line 136 exactly. I want to check in exactly all
cases
> where line 136 is built.

Understood. Then I would suggest that as a comment.

"Using CHECK here since we want to check all the cases where !defined(NDEBUG)
gets built."

https://chromiumcodereview.appspot.com/147203005/diff/280001/sandbox/linux/se...
File sandbox/linux/services/thread_helpers.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/280001/sandbox/linux/se...
sandbox/linux/services/thread_helpers.cc:78: DCHECK_GT(25U, iterations);
On 2014/02/08 00:13:42, jln wrote:
> On 2014/02/08 00:07:12, Jorge Lucangeli Obes wrote:
> > I'm sorry, I wasn't clear. I'm OK with leaving this as 10 iterations -- I
> > actually think it's better. I was asking about the 30 iterations for CHECK.
> 
> Ohh you were clear, I changed it just to be safe. The microsecond metric was a
> bit misleading anyway because of the expected clock granularity.

Done.

[jln (very slow on Chromium), 2014-02-08 00:36:34]

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
File content/common/sandbox_linux/sandbox_linux.cc (right):

https://chromiumcodereview.appspot.com/147203005/diff/70001/content/common/sa...
content/common/sandbox_linux/sandbox_linux.cc:197: // Using DCHECK here would be
incorrect. DCHECK can be enabled in non
On 2014/02/08 00:24:22, Jorge Lucangeli Obes wrote:
> On 2014/02/08 00:13:41, jln wrote:
> > 
> > > I.e. IIUC the only reason to use CHECK over DCHECK is DCHECK being
disabled,
> > not
> > > DCHECK being enabled -- the set "builds where DCHECK is enabled" is
strictly
> > > included in the set "builds where CHECK is enabled".
> > 
> > The comment pertains to using if "!defined(NDEBUG)" + CHECK rather than a
> > DCHECK.
> > 
> > This define has to match line 136 exactly. I want to check in exactly all
> cases
> > where line 136 is built.
> 
> Understood. Then I would suggest that as a comment.
> 
> "Using CHECK here since we want to check all the cases where !defined(NDEBUG)
> gets built."

Done.

[jln (very slow on Chromium), 2014-02-08 00:37:09]

Ken, could you please take a look as content/gpu/gpu_main.cc owner?

[piman, 2014-02-08 00:50:57]

LGTM for gpu_main.

[jln (very slow on Chromium), 2014-02-08 01:00:00]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-02-08 01:01:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/147203005/350002

[commit-bot: I haz the power, 2014-02-08 04:48:27]

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

[commit-bot: I haz the power, 2014-02-08 04:48:27]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[jln (very slow on Chromium), 2014-02-08 04:52:22]

The CQ bit was checked by jln@chromium.org

[commit-bot: I haz the power, 2014-02-08 04:52:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/147203005/350002

[commit-bot: I haz the power, 2014-02-08 04:52:49]

Change committed as 249937

[Ken Russell (switch to Gerrit), 2014-02-10 19:45:53]

LGTM too -- sorry for the delay.