Fixed the audio backgrounding bug

chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc

Index: chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
diff --git a/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc b/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
index 24c58ef9a6acd950d566e73f5f62e0f5225ad8ed..9fff52c3649f5192278d70ec852e82af11b9143b 100644
--- a/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
+++ b/chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc
@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/path_service.h"
 #include "base/process/process.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/devtools/devtools_window.h"
@@ -23,6 +24,8 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test_utils.h"
+#include "net/base/filename_util.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
 
 using content::RenderViewHost;
 using content::RenderWidgetHost;
@@ -75,6 +78,39 @@ base::Process ProcessFromHandle(base::ProcessHandle handle) {
   return base::Process(handle);
 }
 
+GURL GetSimplePageUrl() {
+  base::FilePath no_audio_path;
+  EXPECT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &no_audio_path));
+  no_audio_path = no_audio_path.Append(FILE_PATH_LITERAL("content"));
+  no_audio_path = no_audio_path.Append(FILE_PATH_LITERAL("test"));
+  no_audio_path = no_audio_path.Append(FILE_PATH_LITERAL("data"));
+  no_audio_path = no_audio_path.Append(FILE_PATH_LITERAL("simple_page.html"));

[gab, 2015/08/07 19:55:06]

GetTestUrl("simple_page.html") from content_browser_test_utils.h gives you
exactly this in one line :-)

[sebsg, 2015/08/09 21:07:34]

Good to know! But, as Nick recommended I now use the embedded_test_server
instead :)

+  EXPECT_TRUE(base::PathExists(no_audio_path));
+  return net::FilePathToFileURL(no_audio_path);
+}
+
+GURL GetAudioPlayingPage() {
+  base::FilePath audio_path;
+  EXPECT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &audio_path));
+  audio_path = audio_path.Append(FILE_PATH_LITERAL("chrome"));
+  audio_path = audio_path.Append(FILE_PATH_LITERAL("test"));
+  audio_path = audio_path.Append(FILE_PATH_LITERAL("data"));
+  audio_path = audio_path.Append(FILE_PATH_LITERAL("extensions"));
+  audio_path = audio_path.Append(FILE_PATH_LITERAL("loop_audio.html"));
+  EXPECT_TRUE(base::PathExists(audio_path));
+  return net::FilePathToFileURL(audio_path);
+}
+
+bool IsProcessBackgroundedHelper(base::Process* process) {

[gab, 2015/08/07 19:55:06]

const base::Process& process

(i.e. always use const& for params unless you need non-const -- (non-const &
isn't allowed by style guide)

[sebsg, 2015/08/09 21:07:34]

Done.

+#if defined(OS_MACOSX)
+  MachBroker* broker = MachBroker::GetInstance();
+  mach_port_t task_port = broker->TaskForPid(process.Pid());

[gab, 2015/08/07 19:55:06]

I predict this won't compile because you're invoking the dot-operator on a
pointer ;-).

[sebsg, 2015/08/09 21:07:34]

Done.

+  return process->IsProcessBackgrounded(task_port);
+#else
+  return process->IsProcessBackgrounded();
+#endif  // defined(OS_MACOSX)
+}
+
 }  // namespace
 
 class ChromeRenderProcessHostTest : public InProcessBrowserTest {
@@ -531,3 +567,168 @@ IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest,
 
   observer.Wait();
 }
+
+// Test to make sure that a process is backgrounded when the audio stops playing
+// from it playing and there is an immediate tab switch.

[gab, 2015/08/07 19:55:06]

Remove "from it playing" it makes the sentence confusing and it's implicit (i.e.
if you're stopping it it's because it was previously playing).

[sebsg, 2015/08/09 21:07:34]

Done.

+IN_PROC_BROWSER_TEST_F(
+    ChromeRenderProcessHostTest,
+    BackgroundingLogicOfStoppedAudioAndImmediateTabSwitching) {

[ncarter (slow), 2015/08/06 22:15:58]

ProcessPriorityAfterStoppedAudio

[sebsg, 2015/08/09 21:07:34]

Done.

+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());

[ncarter (slow), 2015/08/06 22:15:58]

Seems like you launch the embedded_test_server, but then load your content via
the file URLs, instead of getting URLs via
embedded_test_server()->GetURL("simple_page.html"). You only need one or the
other. I'd prefer the embedded testserver here, since it's fewer lines of code
and the embedded testserver is pretty fast and reliable (unlike the spawned
testserver, which is a different beast).

From a chrome/ browsertest, you could force the embedded_test_server to also
serve files from content/test/data/ via the following code:

base::FilePath test_data_dir;
ASSERT_TRUE(PathService::Get(base::DIR_SOURCE_ROOT, &test_data_dir));
embedded_test_server()->ServeFilesFromDirectory(
test_data_dir.AppendASCII("content/test/data/"));

That's allowed, and encouraged even for complex test data -- see this thread:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/Go0F5PP2Vl8...

However, I probably wouldn't bother with that just for simple_page.html -- you
can use some other dummy page from chrome/test/data, maybe
chrome/test/data/title1.html

[sebsg, 2015/08/09 21:07:34]

Done.

+
+  if (!base::Process::CanBackgroundProcesses()) {

[gab, 2015/08/07 19:55:06]

Make this the first line of the test with a comment that this test is invalid on
platforms that can't background.

[sebsg, 2015/08/09 21:07:34]

Done.

+    LOG(ERROR) << "Can't background processes";

[gab, 2015/08/07 19:55:06]

Avoid all types of logging in tests (and in general) if not on true errors (in
which case in tests you should use asserts).

Having everybody logging their thing is just as good as having no logs but
noisier and there was a massive effort some years ago to get rid of all logs
that happen in regular flow.

[sebsg, 2015/08/09 21:07:34]

Done.

+    return;
+  }
+  base::CommandLine& parsed_command_line =

[gab, 2015/08/07 19:55:06]

non-const & is banned by the style-guide.

[sebsg, 2015/08/09 21:07:34]

Done.

+      *base::CommandLine::ForCurrentProcess();
+  parsed_command_line.AppendSwitch(switches::kProcessPerTab);

[gab, 2015/08/07 19:55:06]

Instead of doing this here, override SetUpCommandLine() in the test fixture.

[sebsg, 2015/08/09 21:07:34]

Done.

+
+  GURL no_audio_url = GetSimplePageUrl();
+  GURL audio_url = GetAudioPlayingPage();

[gab, 2015/08/07 19:55:06]

Make both of these const

[sebsg, 2015/08/09 21:07:34]

Done.

+
+  ui_test_utils::NavigateToURL(browser(), audio_url);
+
+  content::WebContents* audio_tab_web_content =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Add a new tab to the no audio page and make the tab with the audio page
+  // be the one that is visible. Get the process corresponding to the pages.
+  base::Process no_audio_process = ShowSingletonTab(no_audio_url);
+  base::Process audio_process = ShowSingletonTab(audio_url);

[gab, 2015/08/07 19:55:06]

const

[sebsg, 2015/08/09 21:07:34]

Done.

+
+  // Wait until the no audio page is backgrounded and the audio page is not
+  // backgrounded.
+  while (!IsProcessBackgroundedHelper(&no_audio_process) ||
+         IsProcessBackgroundedHelper(&audio_process)) {
+    base::RunLoop().RunUntilIdle();
+  }
+  EXPECT_TRUE(IsProcessBackgroundedHelper(&no_audio_process));
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&audio_process));

[ncarter (slow), 2015/08/06 22:15:58]

With the while loop above, these EXPECT's are kind of lame -- they can't
possibly fail.

[sebsg, 2015/08/09 21:07:34]

Done.

+
+  // Pause the audio and immediately switch to the no audio tab.
+  content::ExecuteScript(audio_tab_web_content,
+                         "document.getElementById('audioPlayer').pause();");
+  ShowSingletonTab(no_audio_url);
+
+  // Wait until the no audio page is not backgrounded and the audio page is
+  // backgrounded.
+  while (IsProcessBackgroundedHelper(&no_audio_process) ||
+         !IsProcessBackgroundedHelper(&audio_process)) {
+    base::RunLoop().RunUntilIdle();

[ncarter (slow), 2015/08/06 22:15:58]

What'll happen in practice here, is that RunUntilIdle will return immediately
(since there's no work to do) and this functions as a spinloop, likely pegging a
core and possibly stealing scheduler time from the threads we're actually
waiting for. Also, it means that the test's only failure mode is a timeout,
which becomes much harder to debug and more expensive to run.

There are only see a handful of cases of this pattern in existing code:
https://code.google.com/p/chromium/codesearch#search/&q=while(.*%5Cn)?(.*%5Cn...
and I'm pretty sure it's generally discouraged.

So, spins should be an absolute last resort.  Is there any
event/callback/observer method we can wait on (maybe from the audio code), to
catch the point in time where the RPHImpl is told that the transition is done?

Sometimes even just a single content::RunAllPendingInMessageLoop() (without a
loop) does the trick.

[gab, 2015/08/07 19:55:06]

There is no event we can wait in the browser because this is waiting on the
reaction of the OS/kernel for it to truly background the process (well,
actually, I'm not exactly sure how the kernel handles it but I'd assume the OS's
GetProcessPriority() immediately returns the last priority set by
SetProcessPriority() even if it's not done handling it -- so maybe we don't even
need to loop?! @seb, try that?).

But I agree otherwise that this would result in an overly busy loop.

If we have to loop, I'd suggest we add a

  base::PlatformThread::Sleep(TestTimeouts::tiny_timeout());

call here between each iteration (while also running the loop just in case it
has something else to do I guess?).

[sebsg, 2015/08/09 21:07:34]

Unfortunately, the results from using the OS' GetProcessPriority seems to give
flaky results if I dont' do a loop.

+  }
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&no_audio_process));
+  EXPECT_TRUE(IsProcessBackgroundedHelper(&audio_process));
+}
+
+// Test to make sure that a process is backgrounded automatically when audio
+// stops playing from it playing if it is not the active tab.
+IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest,
+                       BackgroundingLogicAudioStopsFromNotVisibleTab) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  if (!base::Process::CanBackgroundProcesses()) {
+    LOG(ERROR) << "Can't background processes";
+    return;
+  }
+  base::CommandLine& parsed_command_line =
+      *base::CommandLine::ForCurrentProcess();
+  parsed_command_line.AppendSwitch(switches::kProcessPerTab);
+
+  GURL no_audio_url = GetSimplePageUrl();
+  GURL audio_url = GetAudioPlayingPage();
+
+  ui_test_utils::NavigateToURL(browser(), audio_url);
+
+  content::WebContents* audio_tab_web_content =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Get the process corresponding to the audio playing page.
+  base::Process audio_process = ProcessFromHandle(
+      audio_tab_web_content->GetRenderProcessHost()->GetHandle());
+
+  // Add a new tab to the no audio page and get its process.
+  base::Process no_audio_process = ShowSingletonTab(no_audio_url);
+
+  ASSERT_TRUE(no_audio_process.IsValid());
+  ASSERT_TRUE(audio_process.IsValid());
+  EXPECT_NE(audio_process.Pid(), no_audio_process.Pid());

[gab, 2015/08/07 19:55:06]

A lot of the test setup is the same for all 3 of these tests.

I think it makes sense to extract the common code to a test fixture.

That test fixture would be a subclass of ChromeRenderProcessHostTest and would
override SetUpOnMainThread() to do the common work (something like starting up
both tabs and storing their Process pointers in member variables).

(it would also make sense for the helper methods specific to backgrounding to
move to this backgrounding specific test fixture)

[sebsg, 2015/08/09 21:07:34]

Done.

+
+  // Wait until the two pages are not backgrounded.
+  while (IsProcessBackgroundedHelper(&no_audio_process) ||
+         IsProcessBackgroundedHelper(&audio_process)) {
+    base::RunLoop().RunUntilIdle();
+  }
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&no_audio_process));
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&audio_process));
+
+  // Stop the audio.
+  content::ExecuteScript(audio_tab_web_content,
+                         "document.getElementById('audioPlayer').pause();");
+
+  // Wait until the no audio page is not backgrounded and the audio page is
+  // backgrounded.
+  while (IsProcessBackgroundedHelper(&no_audio_process) ||
+         !IsProcessBackgroundedHelper(&audio_process)) {
+    base::RunLoop().RunUntilIdle();
+  }
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&no_audio_process));
+  EXPECT_TRUE(IsProcessBackgroundedHelper(&audio_process));
+}
+
+// Test to make sure that a process is un-backgrounded automatically when audio
+// starts playing from it if it is not the active tab.
+IN_PROC_BROWSER_TEST_F(ChromeRenderProcessHostTest,
+                       BackgroundingLogicAudioStartsFromBackgroundTab) {
+  ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
+
+  if (!base::Process::CanBackgroundProcesses()) {
+    LOG(ERROR) << "Can't background processes";
+    return;
+  }
+  base::CommandLine& parsed_command_line =
+      *base::CommandLine::ForCurrentProcess();
+  parsed_command_line.AppendSwitch(switches::kProcessPerTab);
+
+  GURL no_audio_url = GetSimplePageUrl();
+  GURL audio_url = GetAudioPlayingPage();
+
+  ui_test_utils::NavigateToURL(browser(), audio_url);
+
+  content::WebContents* audio_tab_web_content =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  // Stop the audio.
+  content::ExecuteScript(audio_tab_web_content,
+                         "document.getElementById('audioPlayer').pause();");
+
+  // Get the process corresponding to the pages and creates a new tab to for
+  // the no audio page.
+  base::Process audio_process = ProcessFromHandle(
+      audio_tab_web_content->GetRenderProcessHost()->GetHandle());
+  base::Process no_audio_process = ShowSingletonTab(no_audio_url);
+
+  ASSERT_TRUE(no_audio_process.IsValid());
+  ASSERT_TRUE(audio_process.IsValid());
+  EXPECT_NE(audio_process.Pid(), no_audio_process.Pid());
+
+  // Wait until the no audio page is not backgrounded and the audio page is
+  // backgrounded.
+  while (IsProcessBackgroundedHelper(&no_audio_process) ||
+         !IsProcessBackgroundedHelper(&audio_process)) {
+    base::RunLoop().RunUntilIdle();
+  }
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&no_audio_process));
+  EXPECT_TRUE(IsProcessBackgroundedHelper(&audio_process));
+
+  // Start the audio from the backgrounded tab.
+  content::ExecuteScript(audio_tab_web_content,
+                         "document.getElementById('audioPlayer').play();");
+
+  // Wait until the two pages are not backgrounded.
+  while (IsProcessBackgroundedHelper(&no_audio_process) ||
+         IsProcessBackgroundedHelper(&audio_process)) {
+    base::RunLoop().RunUntilIdle();
+  }
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&no_audio_process));
+  EXPECT_FALSE(IsProcessBackgroundedHelper(&audio_process));
+}