Creates a basic Firefox - Chrome WebRTC interop test.

chrome/browser/media/chrome_webrtc_apprtc_browsertest.cc

 // Copyright 2013 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/command_line.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/rand_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/win/windows_version.h"
@@ skipping 15 matching lines @@
 const char kAdviseOnGclientSolution[] =
     "You need to add this solution to your .gclient to run this test:\n"
     "{\n"
     "  \"name\"        : \"webrtc.DEPS\",\n"
     "  \"url\"         : \"svn://svn.chromium.org/chrome/trunk/deps/"
     "third_party/webrtc/webrtc.DEPS\",\n"
     "}";
 const char kTitlePageOfAppEngineAdminPage[] = "Instances";
 
 
 // WebRTC-AppRTC integration test. Requires a real webcam and microphone

[kjellander_chromium, 2013/12/13 10:19:11]

Please clarify that for the Firefox-Chrome test, the real webcam will be used by
Firefox and Chrome will fall back on using its fake device, in order to have a
webcam available for Firefox.

[phoglund_chromium, 2013/12/13 14:35:10]

Done.

 // on the running system. This test is not meant to run in the main browser
 // test suite since normal tester machines do not have webcams.
 //
 // This test will bring up a AppRTC instance on localhost and verify that the
 // call gets up when connecting to the same room from two tabs in a browser.
 class WebrtcApprtcBrowserTest : public WebRtcTestBase {
  public:
   virtual void SetUpCommandLine(CommandLine* command_line) OVERRIDE {
-    EXPECT_FALSE(command_line->HasSwitch(
-        switches::kUseFakeDeviceForMediaStream));
-    EXPECT_FALSE(command_line->HasSwitch(
-        switches::kUseFakeUIForMediaStream));
+    EXPECT_FALSE(command_line->HasSwitch(switches::kUseFakeUIForMediaStream));
 
     // The video playback will not work without a GPU, so force its use here.
     command_line->AppendSwitch(switches::kUseGpuInTests);
   }
 
  protected:
   bool LaunchApprtcInstanceOnLocalhost() {
     base::FilePath appengine_dev_appserver =
         GetSourceDir().Append(
             FILE_PATH_LITERAL("../google_appengine/dev_appserver.py"));
@@ skipping 50 matching lines @@
         "window.domAutomationController.send(remoteVideo.style.opacity)";
     return PollingWaitUntil(javascript, "1", tab_contents);
   }
 
   base::FilePath GetSourceDir() {
     base::FilePath source_dir;
     PathService::Get(base::DIR_SOURCE_ROOT, &source_dir);
     return source_dir;
   }
 
+  bool LaunchFirefoxWithUrl(const GURL& url) {
+    base::FilePath firefox_binary =
+        GetSourceDir().Append(
+            FILE_PATH_LITERAL("../firefox-nightly/firefox/firefox"));

[tommi (sloooow) - chröme, 2013/12/12 18:41:33]

Add each path component separately for cross platform compatibility.

Same below. 

[phoglund_chromium, 2013/12/13 14:35:10]

I can do that, but it will _work_ like this. For instance we're doing that in
the other browser tests which run on all platforms (e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/med...).
I think this is the more readable alternative than adding a ton of .Append and
FILE_PATH_LITERAL calls.

[tommi (sloooow) - chröme, 2013/12/13 14:51:26]

OK, if Append takes care of this then it's fine as is.

+    if (!base::PathExists(firefox_binary)) {
+      LOG(ERROR) << "Missing firefox binary at " <<
+          firefox_binary.value() << ". " << kAdviseOnGclientSolution;
+      return false;
+    }
+    base::FilePath firefox_launcher =
+        GetSourceDir().Append(
+            FILE_PATH_LITERAL("../webrtc.DEPS/run_firefox_webrtc.py"));
+    if (!base::PathExists(firefox_launcher)) {
+      LOG(ERROR) << "Missing firefox launcher at " <<
+          firefox_launcher.value() << ". " << kAdviseOnGclientSolution;
+      return false;
+    }
+
+    CommandLine command_line(firefox_launcher);
+    command_line.AppendSwitchPath("--binary", firefox_binary);
+    command_line.AppendSwitchASCII("--webpage", url.spec());
+
+    VLOG(0) << "Running " << command_line.GetCommandLineString();

[tommi (sloooow) - chröme, 2013/12/12 19:52:05]

forgot one thing - VLOG(0) is apparently frowned upon (see chromium-dev). Use
VLOG(1) for truly 'verbose only' logging.

[phoglund_chromium, 2013/12/13 14:35:10]

Done.

+    return base::LaunchProcess(command_line, base::LaunchOptions(),

[kjellander_chromium, 2013/12/13 10:19:11]

What happens if there's no webcam available for Firefox? It would be nice if
it's easy to find out from logs but since we have a webcam check it's unlikely
to happen unless we make mistakes in additional tests involving Firefox in the
future.

[phoglund_chromium, 2013/12/13 14:35:10]

Don't know, I guess gUM will fail there and the call will not get up. I can see
if I can enable a suitable level of logging for FF for now so that we can see
something from the test logs when the call doesn't get up.

+                               &firefox_);
+
+    return true;
+  }
+
+  bool StopFirefox() {
+    return base::KillProcess(firefox_, 0, false);

[kjellander_chromium, 2013/12/13 10:19:11]

Is there some way we can check if firefox crashed and fail the test in such a
case? I.e. the process no longer exists at this step.

[tommi (sloooow) - chröme, 2013/12/13 12:21:31]

Can we find a better way to close Firefox?
Killing the process like this is a good way to end up with a corrupt profile.  A
corrupt profile will break FF on the next startup, cause the test to fail and
someone (sheriff/trooper) will have to manually go in and fix the profile on the
bot.

[phoglund_chromium, 2013/12/13 14:35:10]

I can't see any easy portable way of doing that unfortunately. Perhaps if we
manage to redesign the firefox run script to terminate itself cleanly somehow.

[phoglund_chromium, 2013/12/13 14:35:10]

It will shut down cleanly on Linux, because I hooked the runner script up to
listen to SIGTERM (which is what kill sends on POSIX). That signal handler will
ask Firefox to shut itself down cleanly. See line 57 of the runner script in the
sibling patch:
https://codereview.chromium.org/114293002/diff/60001/run_firefox_webrtc.py

We probably need to do something else on Windows. Kill does TerminateProcess
(http://msdn.microsoft.com/en-us/library/windows/desktop/ms686714(v=vs.85).aspx)
on Windows, which seems to be a pretty hard kill. The docs for the python signal
API (http://docs.python.org/2/library/signal.html) hints that it maps the
signals on Windows somehow - question is just if it maps TerminateProcess to
SIGTERM or SIGKILL, or none of them.

If we can catch SIGTERM or its equivalent on Windows, I suggest we go with this
KillProcess strategy. I kind of like the idea that a process cleans itself up
nicely when sent the TERM signal. If not, we could redesign the runner script so
that it figures out by itself when it can die. Then we just wait here for it to
die. If it doesn't die, something presumably went wrong.

[tommi (sloooow) - chröme, 2013/12/13 14:51:26]

Signals like that don't exist on Windows unfortunately.  Instead WM_CLOSE or
WM_SYSCOMMAND(SC_CLOSE) can be posted to the main window of the application. 
Then wait for the application to terminate and only if it doesn't quit within a
specific timeout, use KillProcess.

In the case of the launcher, maybe there isn't a window?  In that case you could
try posting a WM_QUIT message to the main thread (PostThreadMessage) and do the
same.

In any case, on Windows, what will happen with the code as is, is that the
launcher will be killed and Firefox will actually be left running.

[phoglund_chromium, 2013/12/13 15:13:35]

Nope, you are right, I actually tried it and registering a signal handler and
TerminateProcess'ing the python instance and it doesn't work on Windows.

There is no window in the launcher itself, so we need to redesign the launcher
so it can figure out when to die and clean up by itself. I suggest we punt that
to a later patch and get this Linux-only patch running first.

[tommi (sloooow) - chröme, 2013/12/13 15:22:30]

Sounds good.  Can you add a note explaining that and perhaps a check that makes
sure this doesn't run on windows until we've fixed it?

+  }
+
  private:
   base::ProcessHandle dev_appserver_;
+  base::ProcessHandle firefox_;
 };
 
 IN_PROC_BROWSER_TEST_F(WebrtcApprtcBrowserTest, MANUAL_WorksOnApprtc) {
   // TODO(mcasas): Remove Win version filtering when this bug gets fixed:
   // http://code.google.com/p/webrtc/issues/detail?id=2703
 #if defined(OS_WIN)
   if (base::win::GetVersion() < base::win::VERSION_VISTA)
     return;
 #endif
   DetectErrorsInJavaScript();
@@ skipping 10 matching lines @@
   // http://code.google.com/p/webrtc/issues/detail?id=1742
   SleepInJavascript(left_tab, 5000);
   chrome::AddTabAt(browser(), GURL(), -1, true);
   content::WebContents* right_tab = OpenPageAndAcceptUserMedia(room_url);
 
   ASSERT_TRUE(WaitForCallToComeUp(left_tab));
   ASSERT_TRUE(WaitForCallToComeUp(right_tab));
 
   ASSERT_TRUE(StopApprtcInstance());
 }
+
+IN_PROC_BROWSER_TEST_F(WebrtcApprtcBrowserTest,
+                       MANUAL_FirefoxApprtcInteropTest) {
+  ASSERT_TRUE(LaunchApprtcInstanceOnLocalhost());

[kjellander_chromium, 2013/12/13 10:19:11]

I think we want to skip this on Vista as well, as in the WorksOnApprtc test
above.

[kjellander_chromium, 2013/12/13 10:19:11]

Add DetectErrorsInJavaScript(); since communicating with firefox might trigger
other JS code paths than the Chrome-Chrome AppRTC test.

[phoglund_chromium, 2013/12/13 14:35:10]

You mean skip it on XP? Yeah, probably. Actually I probably need to realign this
patch to just be for Linux for now anyway. It needs more work for Win and Mac.

[phoglund_chromium, 2013/12/13 14:35:10]

Done.

+  while (!LocalApprtcInstanceIsUp())
+    VLOG(0) << "Waiting for AppRTC to come up...";

[tommi (sloooow) - chröme, 2013/12/12 19:52:05]

would be nice to change all the VLOG(0)'s to VLOG(1)'s in this file.

[phoglund_chromium, 2013/12/13 14:35:10]

Done.

+
+  // Run Chrome with a fake device to avoid having the browsers fight over the
+  // camera (we'll just give that to firefox here).
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kUseFakeDeviceForMediaStream);
+
+  GURL room_url = GURL(base::StringPrintf("http://localhost:9999?r=room_%d",
+                                          base::RandInt(0, 65536)));

[kjellander_chromium, 2013/12/13 10:19:11]

Do we really need to randomize this? I would be surprised if the room i already
taken as I assume the state of the local AppRTC instance is reset between each
run?
For eventual parallel test execution, I guess we would run into a port conflict
on 9999 before hitting an occupied room due to multiple AppRTC instances.

[phoglund_chromium, 2013/12/13 14:35:10]

Yeah, this test can't execute in parallel, but it presumably could if we
randomized the ports.

And no, randomizing the ports seems unnecessary since we restart the server
instance each time.

[tommi (sloooow) - chröme, 2013/12/13 14:51:26]

The room number could also be the thread id.  As is the test can still run into
the occasional flakiness.

[kjellander_chromium, 2013/12/13 14:53:21]

Fair enough, as we don't run these tests in parallel. But please remove the
unnecessary randomization of the room name, since it doesn't seem to be needed.

+  content::WebContents* chrome_tab = OpenPageAndAcceptUserMedia(room_url);
+
+  // TODO(phoglund): Remove when this bug gets fixed:
+  // http://code.google.com/p/webrtc/issues/detail?id=1742
+  SleepInJavascript(chrome_tab, 5000);
+
+  ASSERT_TRUE(LaunchFirefoxWithUrl(room_url));
+
+  ASSERT_TRUE(WaitForCallToComeUp(chrome_tab));

[tommi (sloooow) - chröme, 2013/12/12 18:41:33]

EXPECT_TRUE so that the next line will be run.

Fix this elsewhere too? 

[phoglund_chromium, 2013/12/13 14:35:10]

I think it's good to return early if we have already failed, in general, but
perhaps it would be nice to ensure we clean up Firefox.
I guess I could add a TearDown that checks "if we opened firefox, make sure it
dies".

+  ASSERT_TRUE(StopFirefox());

[kjellander_chromium, 2013/12/13 10:19:11]

Add 
ASSERT_TRUE(StopApprtcInstance());
after stopping firefox? (changing that one to EXPECT_TRUE too).

[phoglund_chromium, 2013/12/13 14:35:10]

Done.

+}