Working around the page load problem on Linux, re-enable tests.

Working around the page load problem on Linux.

After experimenting on the bots, this admittedly silly workaround seems
to work without any negative side effects. This patch makes the main browser test and
the video quality test use the same helper to load pages, and then makes that helper load
the page twice on Linux. I tried that on the bot and it manages to load the js
correctly 100% of the time if I do that.

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

[Ami GONE FROM CHROMIUM, 2013-12-05 19:35:24]

drive-by

https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
chrome/browser/media/webrtc_browsertest_base.cc:116:
ui_test_utils::NavigateToURL(browser(), url);
Shot in the dark: is it possible that NavigateToURL returns "too early" because
of redirection(s)?  I.e. the initial navigate completes but the tab is still
redirecting/loading the new page while the rest of the test continues.

Might be able to tell if this is the case by observing
NavigateToURLBlockUntilNavigationsComplete(browser(), url, 2);
_doesn't_ hang?
Alternately, perhaps ui_test_utils::WaitForAppModalDialog() and add a spurious
alert("Hello!") to the end of the page's JS?

[phoglund_chromium, 2013-12-06 13:21:42]

On 2013/12/05 19:35:24, Ami Fischman wrote:
> drive-by
> 
>
https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
> File chrome/browser/media/webrtc_browsertest_base.cc (right):
> 
>
https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
> chrome/browser/media/webrtc_browsertest_base.cc:116:
> ui_test_utils::NavigateToURL(browser(), url);
> Shot in the dark: is it possible that NavigateToURL returns "too early"
because
> of redirection(s)?  I.e. the initial navigate completes but the tab is still
> redirecting/loading the new page while the rest of the test continues.
> 
> Might be able to tell if this is the case by observing
> NavigateToURLBlockUntilNavigationsComplete(browser(), url, 2);
> _doesn't_ hang?
> Alternately, perhaps ui_test_utils::WaitForAppModalDialog() and add a spurious
> alert("Hello!") to the end of the page's JS?

I have tried a number of things, like sleeping for a couple seconds after
loading the page and before making the javascript call. That didn't help, so
it's not like the page needs an additional n milliseconds to load. I also tried
listing the contents of "window" in the dom tree, and I could see all the
default stuff but none of the functions or variables that the test page defines.
It's as if some background task gets stuck indefinitely loading the page. But
the waitformodaldialog idea is interesting, I'll hack around a bit on the bot
and see if that helps as well.

[phoglund_chromium, 2013-12-06 14:30:53]

Adding the modal dialog thing forces the javascript to load, but then the test
gets stuck on accepting the getUserMedia bar. Perhaps the dialog interferes with
that or something. I think I'll go with the double-page load workaround since
that seems to work. I've already spent way too much time on this and I don't I
can make much of a dent in a complex navigation logic race. tommi, wdyt?

[phoglund_chromium, 2013-12-06 14:34:19]

Also see the bug for more descriptions of what I've tried to debug this.

[phoglund_chromium, 2013-12-06 14:45:06]

On 2013/12/06 14:34:19, phoglund wrote:
> Also see the bug for more descriptions of what I've tried to debug this.

I did try the NavigateToURLBlockUntilNavigationsComplete(browser(), url, 2); and
it hangs on the NavigateToUrl. Waiting for 1 redirect yields the same but where
the javascript isn't loaded.

[phoglund_chromium, 2013-12-06 14:45:35]

On 2013/12/06 14:45:06, phoglund wrote:
> On 2013/12/06 14:34:19, phoglund wrote:
> > Also see the bug for more descriptions of what I've tried to debug this.
> 
> I did try the NavigateToURLBlockUntilNavigationsComplete(browser(), url, 2);
and
> it hangs on the NavigateToUrl. Waiting for 1 redirect yields the same but
where
> the javascript isn't loaded.

So yeah, I'd say let's go with this workaround so that I can regain some of my
sanity :)

[phoglund_chromium, 2013-12-09 12:21:03]

> So yeah, I'd say let's go with this workaround so that I can regain some of my
> sanity :)

Tommi, WDYT?

[tommi (sloooow) - chröme, 2013-12-09 15:30:52]

https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
chrome/browser/media/webrtc_browsertest_base.cc:116:
ui_test_utils::NavigateToURL(browser(), url);
Is it possible to hook up to the onload event instead?

[phoglund_chromium, 2013-12-09 16:22:15]

On 2013/12/09 15:30:52, tommi wrote:
>
https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
> File chrome/browser/media/webrtc_browsertest_base.cc (right):
> 
>
https://codereview.chromium.org/99333010/diff/40001/chrome/browser/media/webr...
> chrome/browser/media/webrtc_browsertest_base.cc:116:
> ui_test_utils::NavigateToURL(browser(), url);
> Is it possible to hook up to the onload event instead?

I don't see how in that case; doing the alert +
ui_test_utils::WaitForAppModalDialog() trick causes the test to hang at a later
stage. "Running the test" from onload will not work either, since without the
alert trick we end up in the same situation as before (i.e. our stuff is not
loaded).

[phoglund_chromium, 2013-12-10 14:59:11]

All right, this is interesting. I tried doing this on the bot:

void WebRtcTestBase::GetUserMedia(content::WebContents* tab_contents,
                                  const std::string& constraints) const {
  // TODO(phoglund): temporary debugging measure for crbug.com/281268.
  std::string javascript =
      "if (typeof(doGetUserMedia) != typeof(Function)) {\n"
      "  console.log('hitting weird js load bug: diagnosing...');\n"
      "  for (var v in window) {\n"
      "    if (window.hasOwnProperty(v)) console.log(v);\n"
      "  }\n"
      "  window.domAutomationController.send('failed!');\n"
      "}\n"
      "else\n"
      "  doGetUserMedia(" + constraints + ");";
  // Request user media: this will launch the media stream info bar.
  std::string result;
  do {
    EXPECT_TRUE(content::ExecuteScriptAndExtractString(
        tab_contents, javascript, &result));
    base::RunLoop().RunUntilIdle();
 } while (result == "failed!");

  EXPECT_EQ("ok-requested", result);
}

Then I added a window.onload event in the test page:

window.onload = function() {
  console.log("loaded");
}

The result is that we're looping until we receive the kill timeout signal

hitting weird js load bug: diagnosing...
hitting weird js load bug: diagnosing...
hitting weird js load bug: diagnosing...
hitting weird js load bug: diagnosing...
hitting weird js load bug: diagnosing...
hitting weird js load bug: diagnosing...
// and so on
loaded
Killed: TIMED OUT

So it seems some task on the main test thread is being starved out. I think this
really captures the gist of the issue. Hmmm, I wonder if that's because the test
runner invokes the test process with --single_process?

[phoglund_chromium, 2013-12-10 15:24:37]

> So it seems some task on the main test thread is being starved out. I think
this
> really captures the gist of the issue. Hmmm, I wonder if that's because the
test
> runner invokes the test process with --single_process?

Although I don't understand why this doesn't happen on my workstation.

[Ami GONE FROM CHROMIUM, 2013-12-10 17:42:58]

On Tue, Dec 10, 2013 at 6:59 AM, <phoglund@chromium.org> wrote:

> So it seems some task on the main test thread is being starved out


I'm not sure what that means, but I wonder if it's related to
325980<https://code.google.com/p/chromium/issues/detail?id=325980>.
 Talk to tommyw?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2013-12-10 20:21:58]

OK, in that case, can you try this:

1. in your onload handler in the page, change the page title to "loaded"
2. In the cc code, after calling NavigateToUrl, do something like:
  content::TitleWatcher title_watcher(tab, "loaded");
  ASSERT_EQ("loaded", title_watcher.WaitAndGetTitle());
(no idea if this works, but something along those lines)
3. Proceed as normal.


On Tue, Dec 10, 2013 at 4:24 PM, <phoglund@chromium.org> wrote:

> So it seems some task on the main test thread is being starved out. I think
>>
> this
>
>> really captures the gist of the issue. Hmmm, I wonder if that's because
>> the
>>
> test
>
>> runner invokes the test process with --single_process?
>>
>
> Although I don't understand why this doesn't happen on my workstation.
>
>
> https://codereview.chromium.org/99333010/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[phoglund_chromium, 2013-12-11 14:17:49]

Doesn't work unfortunately: it just hangs waiting for the title.

[tommi (sloooow) - chröme, 2013-12-11 16:41:16]

ok, as discussed offline it looks like it will take some time to get to the
bottom of this.  So, lgtm for the current workaround since it does reduce the
flakiness of the test. I guess fixing the actual underlying issue is to be
continued.

[commit-bot: I haz the power, 2013-12-11 17:21:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/99333010/40001

[commit-bot: I haz the power, 2013-12-11 17:21:12]

Failed to apply patch for
chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
  Hunk #3 FAILED at 323.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc.rej

Patch:       chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
Index: chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
diff --git a/chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
b/chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
index
9b79d3ec7f34cf6f6a4c71ad9c856e98e4b8452a..ef325d125424be30abaefd852f76f6dde344f788
100644
--- a/chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
+++ b/chrome/browser/media/chrome_webrtc_video_quality_browsertest.cc
@@ -30,14 +30,6 @@
 #include "net/test/python_utils.h"
 #include "testing/perf/perf_test.h"
 
-// Temporarily disabled on Linux.
-// http://crbug.com/281268.
-#if defined(OS_LINUX)
-#define MAYBE_WebrtcVideoQualityBrowserTest
DISABLED_WebrtcVideoQualityBrowserTest
-#else
-#define MAYBE_WebrtcVideoQualityBrowserTest WebrtcVideoQualityBrowserTest
-#endif
-
 static const base::FilePath::CharType kFrameAnalyzerExecutable[] =
 #if defined(OS_WIN)
     FILE_PATH_LITERAL("frame_analyzer.exe");
@@ -100,9 +92,9 @@ static const char kPyWebSocketPortNumber[] = "12221";
 // frame_analyzer. Both tools can be found under third_party/webrtc/tools. The
 // test also runs a stand alone Python implementation of a WebSocket server
 // (pywebsocket) and a barcode_decoder script.
-class MAYBE_WebrtcVideoQualityBrowserTest : public WebRtcTestBase {
+class WebrtcVideoQualityBrowserTest : public WebRtcTestBase {
  public:
-  MAYBE_WebrtcVideoQualityBrowserTest()
+  WebrtcVideoQualityBrowserTest()
       : pywebsocket_server_(0),
         environment_(base::Environment::Create()) {}
 
@@ -331,25 +323,19 @@ class MAYBE_WebrtcVideoQualityBrowserTest : public
WebRtcTestBase {
   scoped_ptr<base::Environment> environment_;
 };
 
-IN_PROC_BROWSER_TEST_F(MAYBE_WebrtcVideoQualityBrowserTest,
+IN_PROC_BROWSER_TEST_F(WebrtcVideoQualityBrowserTest,
                        MANUAL_TestVGAVideoQuality) {
   ASSERT_TRUE(HasAllRequiredResources());
   ASSERT_TRUE(embedded_test_server()->InitializeAndWaitUntilReady());
   ASSERT_TRUE(StartPyWebSocketServer());
   ASSERT_TRUE(peerconnection_server_.Start());
 
-  ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(kMainWebrtcTestHtmlPage));
   content::WebContents* left_tab =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  GetUserMediaAndAccept(left_tab);
-
-  chrome::AddTabAt(browser(), GURL(), -1, true);
+      OpenPageAndGetUserMediaInNewTab(
+          embedded_test_server()->GetURL(kMainWebrtcTestHtmlPage));
   content::WebContents* right_tab =
-      browser()->tab_strip_model()->GetActiveWebContents();
-  ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(kCapturingWebrtcHtmlPage));
-  GetUserMediaAndAccept(right_tab);
+      OpenPageAndGetUserMediaInNewTab(
+          embedded_test_server()->GetURL(kCapturingWebrtcHtmlPage));
 
   ConnectToPeerConnectionServer("peer 1", left_tab);
   ConnectToPeerConnectionServer("peer 2", right_tab);

[commit-bot: I haz the power, 2013-12-11 17:57:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/phoglund@chromium.org/99333010/60001

[commit-bot: I haz the power, 2013-12-11 20:33:58]

Change committed as 240170