Fix headless_browsertests failures with PlzNavigate.

headless/test/headless_browser_test.cc

 // Copyright 2016 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 "headless/test/headless_browser_test.h"
 
 #include "base/files/file_path.h"
 #include "base/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/common/url_constants.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "headless/lib/browser/headless_browser_impl.h"
+#include "headless/lib/browser/headless_web_contents_impl.h"
 #include "headless/lib/headless_content_main_delegate.h"
 #include "headless/public/devtools/domains/runtime.h"
 #include "headless/public/headless_devtools_client.h"
 #include "headless/public/headless_devtools_target.h"
 #include "headless/public/headless_web_contents.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/geometry/size.h"
 #include "url/gurl.h"
 
 namespace headless {
@@ skipping 125 matching lines @@
 
 HeadlessBrowser* HeadlessBrowserTest::browser() const {
   return HeadlessContentMainDelegate::GetInstance()->browser();
 }
 
 HeadlessBrowser::Options* HeadlessBrowserTest::options() const {
   return HeadlessContentMainDelegate::GetInstance()->browser()->options();
 }
 
 bool HeadlessBrowserTest::WaitForLoad(HeadlessWebContents* web_contents) {
-  SynchronousLoadObserver load_observer(this, web_contents);
-  RunAsynchronousTest();
-  return load_observer.navigation_succeeded();
+  HeadlessWebContentsImpl* web_contents_impl =
+      HeadlessWebContentsImpl::From(web_contents);
+  content::TestNavigationObserver observer(web_contents_impl->web_contents(),

[Sami, 2017/02/27 11:46:19]

This part makes me a little nervous: if we can't rely on waiting for loads to
complete just by looking at DevTools events, does that mean that headless users
(who can only used DevTools) can't do this reliably either? I wonder if there's
a way to make the messages related to this get deterministically delivered
instead?

[jam, 2017/02/27 15:18:18]

The main difference with PlzNavigate, due to the buffering of devtools IPCs in
RenderFrameDevToolsAgentHost with PlzNavigate, is that loads triggered before
enabling network module won't generate events (and similarly for other devtools
messages). The solution for headless consumers is that they have to wait
synchronously for module initialization like I did in the other devtools tests,
if they care about devtools messages. Per Dmitry, this buffering could go away
once the speculative RenderFrameHost in PlzNavigate is removed, but that is not
happening before PlzNavigate launches, which is why I don't see any other
solution.

[Sami, 2017/02/27 16:22:53]

I see, thanks for the explanation. I wonder if there's still a fundamental race
condition here since these tests are mostly trying to catch the initial
navigation of the WebContents (i.e., not an explicit Page.navigate) which could
in theory happen way before we get around to attaching the DevTools client (in
order to enable the network domain). Is there's any way around that -- or do we
need to do something like specifying the list of domains to enable when creating
the WebContents?

[jam, 2017/02/27 16:29:43]

I'm not really sure I understand this comment, sorry do you have specific steps?


Before the change in this function, there was a lot of flakiness with
PlzNavigate enabled. Afterwards I didn't see any after a lot of runs. So I'm
inclined to believe there are no race conditions because this change was made
after a lot of tracing to ensure that IPCs are received in the right order.

[Sami, 2017/02/27 16:36:39]

Sorry for being vague: I fully trust this specific change works. I'm just
wondering what to tell headless folks who can't use
content::TestNavigationObserver. Here's the scenario I'm thinking of:

1. Create a headless WebContents, giving it an initial URL to navigate to.
2. Attach a DevTools client to the WebContents.
3. Enable the Page domain.
4. Wait for a load event that never comes, because the navigation already
happened before step 3 (or really, step 2)?

The chicken/egg-situation being that you can't enable the Page domain before you
have a WebContents, which navigates as soon as it's constructed. Please tell me
if I'm missing something :)

[jam, 2017/02/27 16:52:57]

ah, got it, thanks.

An example of this scenario is my changes to embedder_mojo_browsertest.cc in
this cl. It's not possible, with the current devtools code, to navigate a page
and then wait for devtools messages. The solution is to have a WebContents start
with the default about:blank, and then after enabling a devtools module that
they're interested in, navigate.

[jam, 2017/02/27 16:53:45]

oh, and if course, if all a user of headless cares about is waiting for
navigation to complete, you can always expose a version of WaitForLoad to
non-test code.

[Sami, 2017/02/27 17:33:02]

Alright, thanks for the explanation. The intermediate hop to about:blank is a
little unfortunate, but maybe we can't avoid it without exposing something new.

+                                           1);
+  observer.Wait();
+  return observer.last_navigation_succeeded();
 }
 
 std::unique_ptr<runtime::EvaluateResult> HeadlessBrowserTest::EvaluateScript(
     HeadlessWebContents* web_contents,
     const std::string& script) {
   EvaluateHelper helper(this, web_contents, script);
   RunAsynchronousTest();
   return helper.TakeResult();
 }
 
@@ skipping 47 matching lines @@
   if (!render_process_exited_)
     web_contents_->GetDevToolsTarget()->DetachClient(devtools_client_.get());
   web_contents_->RemoveObserver(this);
   web_contents_->Close();
   web_contents_ = nullptr;
   browser_context_->Close();
   browser_context_ = nullptr;
 }
 
 }  // namespace headless