Regression tests for two recently-fixed audio-related crashers.

chrome/browser/media/media_browsertest.cc

Index: chrome/browser/media/media_browsertest.cc
diff --git a/chrome/browser/media/media_browsertest.cc b/chrome/browser/media/media_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..76290823d6e2e18cf02420135f5b32432110abf2
--- /dev/null
+++ b/chrome/browser/media/media_browsertest.cc
@@ -0,0 +1,90 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/test/test_timeouts.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/browser/tab_contents/tab_contents.h"
+
+class MediaBrowserTest : public InProcessBrowserTest {
+ public:
+  MediaBrowserTest()
+      : seek_jumper_url_(ui_test_utils::GetTestUrl(
+          FilePath(FILE_PATH_LITERAL("media")),
+          FilePath(FILE_PATH_LITERAL("seek-jumper.html")))) {
+    EnableDOMAutomation();
+  }
+
+ protected:
+  // Loop until |closure| returns true, CHECK-failing if
+  // this fails to happen within a reasonable amount of time.
+  void WaitForSuccess(const base::Callback<bool(void)>& closure) {

[Paweł Hajdan Jr., 2011/11/02 08:43:27]

Sorry, no way. Please wait for an event, this kind of code is a known
anti-pattern.

[Ami GONE FROM CHROMIUM, 2011/11/02 16:32:48]

The events I need to await for are "JS expression becomes true in the page" (or,
equivalently, JS method is called in the page) and "browser tab count becomes
N".  Can you tell me how to listen for them without these effective sleeps?

[Paweł Hajdan Jr., 2011/11/02 18:18:15]

This is non-trivial, but I think extensions and WebUI teams do something very
similar. Unfortunately I don't know much more. However, this C++ <-> JS
interaction is a very common cause of flakiness if not done with extreme care.
That's easy! See TabCountChangeObserver and
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/chrome...

It may even be useful for you in the future, so I'm glad you asked.

+    const int kLoopIntervalMs = 250;
+    const int kNumIntervals =
+        TestTimeouts::action_timeout_ms() / kLoopIntervalMs;
+    for (int i = 0; i < kNumIntervals; ++i) {
+      if (closure.Run())
+        return;
+      MessageLoop::current()->PostDelayedTask(
+          FROM_HERE, new MessageLoop::QuitTask(), kLoopIntervalMs);
+      ui_test_utils::RunMessageLoop();
+    }
+    LOG(FATAL) << "Timed out.";
+  }
+
+  const GURL seek_jumper_url_;
+};
+
+// Helper that executes |js_expr| in |tab| and returns its evaluated value as a
+// bool.
+static bool ExecuteJavaScriptAsBool(TabContents* tab,
+                                    const std::wstring& js_expr) {
+  bool result = false;
+  CHECK(ui_test_utils::ExecuteJavaScriptAndExtractBool(

[Paweł Hajdan Jr., 2011/11/02 08:43:27]

Please avoid CHECK, on any failures the test would have to be DISABLED. I can
help you come up with a better design.

[Ami GONE FROM CHROMIUM, 2011/11/02 16:32:48]

1) Why does a CHECK (as opposed to propagating a gtest failure and ASSERTing
it's absence in the TEST method) subject the test to a higher risk of
DISABLEment?  I thought InProcessBrowserTest's were launched in individual
processes anyway, so a CHECK failure here results in just the calling test
crashing, and not affecting other tests.

2) The condition being CHECKd for indicates a programming error, so it seems
reasonable to me to use CHECK (as opposed to missing an expectation of the test,
for which I use ASSERT/EXPECT).

[Paweł Hajdan Jr., 2011/11/02 18:18:15]

That's true. Now here's the catch: we still want to distinguish between flaky
failures and crashes, so FLAKY and FAILS do not suppress those crashes. Other
tests are indeed not affected, but the only way to green the tree is to DISABLE
the test. Please see also
http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/han...
For anything in ui_test_utils failing, you can't say for sure it's a programming
error. There are many different reasons those calls can fail, especially those
related to JS and the renderers.

+      tab->render_view_host(), L"", js_expr, &result));
+  return result;
+}
+
+// Helper that returns true iff |browser| has |expected_tab_count| tabs.
+static bool IsTabCount(Browser* browser, int expected_tab_count) {
+  return browser->tab_count() == expected_tab_count;
+}
+
+// Regression test: pending seeks shouldn't crash the browser when the tab is
+// closed.
+IN_PROC_BROWSER_TEST_F(MediaBrowserTest, SeekJumper_Alone) {
+  ui_test_utils::NavigateToURL(browser(), seek_jumper_url_);
+  WaitForSuccess(base::Bind(
+      &ExecuteJavaScriptAsBool,
+      browser()->GetTabContentsAt(0),
+      L"window.domAutomationController.send(seekCount > 100)"));
+  CHECK(ui_test_utils::SendKeyPressSync(

[Paweł Hajdan Jr., 2011/11/02 08:43:27]

Same here.

+      browser(), ui::VKEY_W, true, false, false, false));
+  // Lack of crash is our SUCCESS.
+}
+
+// Regression test: pending seeks shouldn't crash a shared renderer when the tab
+// containing the audio element is closed.
+IN_PROC_BROWSER_TEST_F(MediaBrowserTest, SeekJumper_SharedRenderer) {
+  ui_test_utils::NavigateToURL(browser(), seek_jumper_url_);
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), seek_jumper_url_, NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  WaitForSuccess(base::Bind(&IsTabCount, browser(), 2));
+  WaitForSuccess(base::Bind(
+      &ExecuteJavaScriptAsBool,
+      browser()->GetTabContentsAt(0),
+      L"window.domAutomationController.send(seekCount > 100)"));
+  CHECK(ui_test_utils::SendKeyPressSync(
+      browser(), ui::VKEY_W, true, false, false, false));
+  WaitForSuccess(base::Bind(&IsTabCount, browser(), 1));
+  // Give the renderer a bit of time to crash.  Sad but necessary.

[Paweł Hajdan Jr., 2011/11/02 08:43:27]

Not true, at least on POSIX. On Windows it's a known problem that should be
fixed. No workarounds please.

[Ami GONE FROM CHROMIUM, 2011/11/02 16:32:48]

I feel like you have a magic cure but you're holding it behind your back,
waiting for me to guess :)

How would you implement this on POSIX?
How would you fix this on Windows?

[Paweł Hajdan Jr., 2011/11/02 18:18:15]

Not exactly, but yeah, I was working on this issue and just wanted to keep my
initial comment short. I'm glad you're interested in the real answer.
Take a look at InitialLoadObserver and
content::NOTIFICATION_RENDERER_PROCESS_CLOSED. One note: you should then
probably check the details to see if it was a crash. You can then use
WindowedNotificationObserver to wait for the notification.
Same way, but I think there is a bug that results in
NOTIFICATION_RENDERER_PROCESS_CLOSED not being called in some circumstances
(when the renderer crashes before connecting to the IPC channel). Actually this
test doesn't seem to be affected by that bug.

+  MessageLoop::current()->PostDelayedTask(
+      FROM_HERE, new MessageLoop::QuitTask(), 2000);

[Paweł Hajdan Jr., 2011/11/02 08:43:27]

No hardcoded timeouts please.

[Ami GONE FROM CHROMIUM, 2011/11/02 16:32:48]

Have you looked at the bugs that these tests are regression-testing?  I'm happy
to hear concrete suggestions you have for improvements.

[Paweł Hajdan Jr., 2011/11/02 18:18:15]

I've read the bugs now. I think my comment about this still makes sense. On
Valgrind you'll execute much less iterations in 2 s than without it. With
TestTimeouts this time would be automatically adjusted for you when executing in
a slower environment.

+  ui_test_utils::RunMessageLoop();
+  ASSERT_FALSE(browser()->GetTabContentsAt(0)->is_crashed());
+}