Fix tab backgrounding on Linux / ChromeOS

Fix tab backgrounding

This broke because an incompatible change was made to Process, and because there was no test that would catch it. I've fixed the underlying problem for both Linux and ChromeOS and made it testable.


BUG=102726
TEST=Added new RenderProcessHostTest.Backgrounding

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112712

[jam, 2011-11-28 21:45:13]

I'm not an owner for base, and not familiar with this code, so I defer to other
owners. One question: the code in base seems very chrome specific (i.e. the bits
about opening cros files).  It seems like it belongs in content module

[DaveMoore, 2011-11-28 22:25:10]

+brettw

[willchan no longer on Chromium, 2011-11-29 22:23:34]

I think you can avoid both the races and static initializers by using
LazyInstance.

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc
File base/process_linux.cc (right):

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:24: const char kForeground[] =
"/chrome_renderers/foreground";
I'm not particularly happy about this renderer specific code. I guess this was
already the case, so it's not any worse than it was previously.

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:28: FilePath foreground_file;
These are static initializers. Are you sure you want this?

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:37: static bool use_cgroups = false;
This code is racy, right?

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:105: static bool checked_for_permission = false;
This is racy.

http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_browsertest.cc:45:
NewRunnableFunction(&PostQuit, MessageLoop::current()));
I'd use
content::BrowserThread::PostTaskAndReply(
    content::BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
    base::Closure(), MessageLoop::QuitClosure());

Then you can ditch the boilerplate PostQuit() function.

http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_browsertest.h (right):

http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_browsertest.h:20:
base::ProcessHandle ShowSingletonTab(GURL page);
const GURL& perhaps?

[DaveMoore, 2011-11-30 19:01:18]

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc
File base/process_linux.cc (right):

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:24: const char kForeground[] =
"/chrome_renderers/foreground";
Added todo() to refactor
On 2011/11/29 22:23:35, willchan wrote:
> I'm not particularly happy about this renderer specific code. I guess this was
> already the case, so it's not any worse than it was previously.

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:28: FilePath foreground_file;
Changed to LazyInstance<>
On 2011/11/29 22:23:35, willchan wrote:
> These are static initializers. Are you sure you want this?

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:37: static bool use_cgroups = false;
Added Lock.
On 2011/11/29 22:23:35, willchan wrote:
> This code is racy, right?

http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
base/process_linux.cc:105: static bool checked_for_permission = false;
Added lock.
On 2011/11/29 22:23:35, willchan wrote:
> This is racy.

http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_browsertest.h (right):

http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_browsertest.h:20:
base::ProcessHandle ShowSingletonTab(GURL page);
On 2011/11/29 22:23:35, willchan wrote:
> const GURL& perhaps?

Done.

[DaveMoore, 2011-11-30 22:49:51]

My try jobs revealed that it's not ok to pass base::Closure() to
PostTaskAndReply(). You need to Bind an empty function instead.

On 2011/11/30 19:01:18, DaveMoore wrote:
> http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc
> File base/process_linux.cc (right):
> 
>
http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
> base/process_linux.cc:24: const char kForeground[] =
> "/chrome_renderers/foreground";
> Added todo() to refactor
> On 2011/11/29 22:23:35, willchan wrote:
> > I'm not particularly happy about this renderer specific code. I guess this
was
> > already the case, so it's not any worse than it was previously.
> 
>
http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
> base/process_linux.cc:28: FilePath foreground_file;
> Changed to LazyInstance<>
> On 2011/11/29 22:23:35, willchan wrote:
> > These are static initializers. Are you sure you want this?
> 
>
http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
> base/process_linux.cc:37: static bool use_cgroups = false;
> Added Lock.
> On 2011/11/29 22:23:35, willchan wrote:
> > This code is racy, right?
> 
>
http://codereview.chromium.org/8506036/diff/11001/base/process_linux.cc#newco...
> base/process_linux.cc:105: static bool checked_for_permission = false;
> Added lock.
> On 2011/11/29 22:23:35, willchan wrote:
> > This is racy.
> 
>
http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
> File content/browser/renderer_host/render_process_host_browsertest.h (right):
> 
>
http://codereview.chromium.org/8506036/diff/11001/content/browser/renderer_ho...
> content/browser/renderer_host/render_process_host_browsertest.h:20:
> base::ProcessHandle ShowSingletonTab(GURL page);
> On 2011/11/29 22:23:35, willchan wrote:
> > const GURL& perhaps?
> 
> Done.

[willchan no longer on Chromium, 2011-12-01 06:26:32]

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc
File base/process_linux.cc (right):

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc#newco...
base/process_linux.cc:43: base::Lock lock;
Is this lock used?

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc#newco...
base/process_linux.cc:47: cgroups_inited = true;
This cgroups_inited is still racy. I think you should just get rid of it. Create
a struct that encompasses both foreground_file and background_file. In the
struct's constructor, initialize the foreground_file and background_file.

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc#newco...
base/process_linux.cc:115: return false;
What's going on here? Always returning false? The lock is not needed, you should
use LazyInstance to wrap can_reraise_priority, and get rid of the
checked_for_permission.

[DaveMoore, 2011-12-01 22:14:17]

lgtm

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc
File base/process_linux.cc (right):

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc#newco...
base/process_linux.cc:43: base::Lock lock;
I meant AutoLock, but it's gone now
On 2011/12/01 06:26:32, willchan wrote:
> Is this lock used?

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc#newco...
base/process_linux.cc:47: cgroups_inited = true;
On 2011/12/01 06:26:32, willchan wrote:
> This cgroups_inited is still racy. I think you should just get rid of it.
Create
> a struct that encompasses both foreground_file and background_file. In the
> struct's constructor, initialize the foreground_file and background_file.

Done.

http://codereview.chromium.org/8506036/diff/22001/base/process_linux.cc#newco...
base/process_linux.cc:115: return false;
Accidentally left this in when I was testing. Changed as suggested.

On 2011/12/01 06:26:32, willchan wrote:
> What's going on here? Always returning false? The lock is not needed, you
should
> use LazyInstance to wrap can_reraise_priority, and get rid of the
> checked_for_permission.

[willchan no longer on Chromium, 2011-12-01 22:26:03]

lgtm

http://codereview.chromium.org/8506036/diff/26001/base/process_linux.cc
File base/process_linux.cc (right):

http://codereview.chromium.org/8506036/diff/26001/base/process_linux.cc#newco...
base/process_linux.cc:71: if (file_util::ReadFileToString(
Do we need to worry about this file I/O here? I assume you're calling this on
the IO thread or something. I don't have a good solution, but just wanted to see
what you thought.

[Ben Goodger (Google), 2011-12-02 00:05:04]

http://codereview.chromium.org/8506036/diff/26001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_browsertest.cc (right):

http://codereview.chromium.org/8506036/diff/26001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_browsertest.cc:41:
browser()->ShowSingletonTab(page);
Can you achieve this by using a different Browser method?

Reason is, content-team is working on breaking the dependency between content
browser tests and chrome. This will mean they eventually get rid of access to
browser() from these tests. They will have to port tests to use only types
available in src/content. For simple operations like opening a new tab, these
are more straightforward to port, since they can just create a new TabContents
instead of bouncing through Browser.

But this API, ShowSingletonTab, is very Browser-specific and it'd be good if we
could avoid it.

[Ben Goodger (Google), 2011-12-02 00:31:01]

LGTM for content.

[commit-bot: I haz the power, 2011-12-02 00:39:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davemoore@chromium.org/8506036/30001

[commit-bot: I haz the power, 2011-12-02 03:46:09]

Try job failure for 8506036-30001 (retry) (previous was lost) on win_rel for
step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...