[Chromecast] Add browser test for CastWebContents

CastWebContentsImpl now has an associated browser test. This allows us
to test various content scenarios, such as net errors, content errors,
and page crashes. It also tests the standard lifecycle of a
CastWebContents, and verifies various state transitions. This new test
prompted some modifications to the CastWebContentsImpl class, including
some more descriptive comments on CWC's intended behavior.

Bug: internal b/67864604
Bug: internal b/130190264
Test: cast_shell_browsertests

Change-Id: Ibe24207cf091634cb046898276258579d9234b9e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1560492
Commit-Queue: Sean Topping <seantopping@chromium.org>
Reviewed-by: Luke Halliwell <halliwell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#649697}

chromecast/browser/BUILD.gn

@@ -469,6 +469,7 @@ cast_source_set("browsertests") {
   sources = [
     "cast_content_gesture_handler_test.cc",
     "cast_media_blocker_browsertest.cc",
+    "cast_web_contents_browsertest.cc",
     "renderer_prelauncher_test.cc",
     "test/cast_features_browsertest.cc",
     "test/cast_navigation_browsertest.cc",
@@ -478,15 +479,20 @@ cast_source_set("browsertests") {
 
   deps = [
     ":test_support",
+    "//base",
     "//chromecast:chromecast_buildflags",
     "//chromecast/base",
     "//chromecast/base:chromecast_switches",
     "//chromecast/base/metrics",
     "//components/prefs",
+    "//content/public/browser",
+    "//content/test:test_support",
     "//media:test_support",
+    "//net:test_support",
   ]
 
   data = [
+    "//chromecast/browser/test/data/",
     "//media/test/data/",
   ]
 }

chromecast/browser/cast_web_contents.h

@@ -28,6 +28,70 @@ struct RendererFeature {
 };
 
 // Simplified WebContents wrapper class for Cast platforms.
+//
+// Proper usage of content::WebContents relies on understanding the meaning
+// behind various WebContentsObserver methods, and then translating those
+// signals into some concrete state. CastWebContents does *not* own the
+// underlying WebContents (usually whatever class implements
+// content::WebContentsDelegate is the actual owner).
+//
+// =============================================================================
+// Lifetime
+// =============================================================================
+// CastWebContents *must* be created before WebContents begins loading any
+// content. Once content begins loading (via CWC::LoadUrl() or one of the
+// WebContents navigation APIs), CastWebContents will calculate its state based
+// on the status of the WebContents' *main* RenderFrame. Events from sub-frames
+// (e.g. iframes) are ignored, since we expect the web app to take care of
+// sub-frame errors.
+//
+// We consider the CastWebContents to be in a LOADED state when the content of
+// the main frame is fully loaded and running (all resources fetched, JS is
+// running). Iframes might still be loading in this case, but in general we
+// consider the page to be in a presentable state at this stage. It is
+// appropriate to display the WebContents to the user.
+//
+// During or after the page is loaded, there are multiple error conditions that
+// can occur. The following events will cause the page to enter an ERROR state:
+//
+// 1. If the main frame is served an HTTP error page (such as a 404 page), then
+//    it means the desired content wasn't loaded.
+//
+// 2. If the main frame fails to load, such as when the browser blocked the URL
+//    request, we treat this as an error.
+//
+// 3. The RenderProcess for the main frame could crash, so the page is not in a
+//    usable state.
+//
+// The CastWebContents user can respond to these errors in a few ways: The
+// content can be reloaded, or the entire page activity can be cancelled. If we
+// totally cancel the activity, we prefer to notify the user with an error
+// screen or visible/audible error message. Otherwise, a silent retry is
+// preferred.
+//
+// CastWebContents can be used to close the underlying WebContents gracefully
+// via CWC::Close(). This initiates web page tear-down logic so that the web
+// app has a chance to perform its own finalization logic in JS. Next, we call
+// WebContents::ClosePage(), which defers the page closure logic to the
+// content::WebContentsDelegate. Usually, it will run its own finalization
+// logic and then destroy the WebContents. CastWebContents will be notified of
+// the WebContents destruction and enter the DESTROYED state. In the event
+// the page isn't destroyed, the page will enter the CLOSED state automatically
+// after a timeout. CastWebContents users should not try to reload the page, as
+// page closure is intentional.
+//
+// The web app may decide to close itself (such as via "window.close()" in JS).
+// This is similar to initiating the close flow via CWC::Close(), with the end
+// result being the same. We consider this an intentional closure, and should
+// not attempt to reload the page.
+//
+// Once CastWebContents is in the DESTROYED state, it is not really usable
+// anymore; most of the methods will simply no-op, and no more observer signals
+// will be emitted.
+//
+// CastWebContents can be deleted at any time, *except* during Observer
+// notifications. If the owner wants to destroy CastWebContents as a result of
+// an Observer event, it should post a task to destroy CastWebContents.
 class CastWebContents {
  public:
   class Delegate {
@@ -39,17 +103,19 @@ class CastWebContents {
     // Called when the page has stopped. e.g.: A 404 occurred when loading the
     // page or if the render process for the main frame crashes. |error_code|
     // will return a net::Error describing the failure, or net::OK if the page
-    // closed naturally.
+    // closed intentionally.
     //
     // After this method, the page state will be one of the following:
-    // CLOSED: Page was closed as expected and the WebContents exists.
+    // CLOSED: Page was closed as expected and the WebContents exists. The page
+    //     should generally not be reloaded, since the page closure was
+    //     triggered intentionally.
+    // ERROR: Page is in an error state. It should be reloaded or deleted.
     // DESTROYED: Page was closed due to deletion of WebContents. The
     //     CastWebContents instance is no longer usable and should be deleted.
-    // ERROR: Page is in an error state. It should be reloaded or deleted.
     virtual void OnPageStopped(CastWebContents* cast_web_contents,
                                int error_code) = 0;
 
-    // Notify that a inner WebContents was created. |inner_contents| is created
+    // Notify that an inner WebContents was created. |inner_contents| is created
     // in a default-initialized state with no delegate, and can be safely
     // initialized by the delegate.
     virtual void InnerContentsCreated(CastWebContents* inner_contents,
@@ -66,6 +132,9 @@ class CastWebContents {
     virtual void RenderFrameCreated(int render_process_id,
                                     int render_frame_id) {}
 
+    // Notifies that a resource for the main frame failed to load.
+    virtual void ResourceLoadFailed(CastWebContents* cast_web_contents) {}
+
     // Adds |this| to the ObserverList in the implementation of
     // |cast_web_contents|.
     void Observe(CastWebContents* cast_web_contents);
@@ -83,10 +152,15 @@ class CastWebContents {
     CastWebContents* cast_web_contents_;
   };
 
+  // Initialization parameters for CastWebContents.
   struct InitParams {
     Delegate* delegate;
+    // Whether the underlying WebContents is exposed to the remote debugger.
     bool enabled_for_dev;
+    // Chooses a media renderer for the WebContents.
     bool use_cma_renderer;
+    // Whether the WebContents is a root native window, or if it is embedded in
+    // another WebContents (see Delegate::InnerContentsCreated()).
     bool is_root_window = false;
   };
 
@@ -142,8 +216,8 @@ class CastWebContents {
 
   // Stop the page immediately. This will automatically invoke
   // Delegate::OnPageStopped(error_code), allowing the delegate to delete or
-  // reload the page without waiting for page teardown, which may be handled
-  // independently.
+  // reload the page without waiting for the WebContents owner to tear down the
+  // page.
   virtual void Stop(int error_code) = 0;
 
   // Used to add or remove |observer| to the ObserverList in the implementation.

chromecast/browser/cast_web_contents_impl.cc

@@ -20,6 +20,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/common/bindings_policy.h"
+#include "content/public/common/resource_load_info.mojom.h"
 #include "net/base/net_errors.h"
 #include "services/service_manager/public/cpp/interface_provider.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
@@ -61,6 +62,7 @@ CastWebContentsImpl::CastWebContentsImpl(content::WebContents* web_contents,
       remote_debugging_server_(
           shell::CastBrowserProcess::GetInstance()->remote_debugging_server()),
       tab_id_(init_params.is_root_window ? 0 : next_tab_id++),
+      main_frame_loaded_(false),
       closing_(false),
       stopped_(false),
       stop_notified_(false),
@@ -127,13 +129,8 @@ void CastWebContentsImpl::LoadUrl(const GURL& url) {
     LOG(ERROR) << "Cannot load URL for WebContents while closing";
     return;
   }
-  closing_ = false;
-  stopped_ = false;
-  stop_notified_ = false;
-  last_error_ = net::OK;
-  start_loading_ticks_ = base::TimeTicks::Now();
+  OnPageLoading();
   LOG(INFO) << "Load url: " << url.possibly_invalid_spec();
-  TracePageLoadBegin(url);
   web_contents_->GetController().LoadURL(url, content::Referrer(),
                                          ui::PAGE_TRANSITION_TYPED, "");
   UpdatePageState();
@@ -299,6 +296,29 @@ void CastWebContentsImpl::RenderProcessGone(base::TerminationStatus status) {
   Stop(net::ERR_UNEXPECTED);
 }
 
+void CastWebContentsImpl::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(navigation_handle);
+  if (!web_contents_ || closing_ || stopped_)
+    return;
+  if (!navigation_handle->IsInMainFrame())
+    return;
+  // Main frame has begun navigating/loading.
+  OnPageLoading();
+  start_loading_ticks_ = base::TimeTicks::Now();
+  GURL loading_url;
+  content::NavigationEntry* nav_entry =
+      web_contents()->GetController().GetVisibleEntry();
+  if (nav_entry) {
+    loading_url = nav_entry->GetVirtualURL();
+  }
+  TracePageLoadBegin(loading_url);
+  UpdatePageState();
+  DCHECK_EQ(page_state_, PageState::LOADING);
+  NotifyObservers();
+}
+
 void CastWebContentsImpl::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
@@ -311,6 +331,10 @@ void CastWebContentsImpl::DidFinishNavigation(
     return;
   }
 
+  // Return early if we didn't navigate to an error page. Note that even if we
+  // haven't navigated to an error page, there could still be errors in loading
+  // the desired content: e.g. if the server returned HTTP 404, or if there is
+  // an error with the content itself.
   if (!navigation_handle->IsErrorPage())
     return;
 
@@ -328,48 +352,91 @@ void CastWebContentsImpl::DidFinishNavigation(
 
   LOG(ERROR) << "Got error on navigation: url=" << navigation_handle->GetURL()
              << ", error_code=" << error_code
-             << ", description= " << net::ErrorToShortString(error_code);
+             << ", description=" << net::ErrorToShortString(error_code);
 
   Stop(error_code);
   DCHECK_EQ(page_state_, PageState::ERROR);
 }
 
-void CastWebContentsImpl::DidStartLoading() {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  UpdatePageState();
-  DCHECK_EQ(page_state_, PageState::LOADING);
-  NotifyObservers();
-}
-
-void CastWebContentsImpl::DidStopLoading() {
+void CastWebContentsImpl::DidFinishLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  if (page_state_ != PageState::LOADING || !web_contents_ ||
+      render_frame_host != web_contents_->GetMainFrame()) {
+    return;
+  }
+  // The main frame finished loading. Before proceeding, we need to verify that
+  // the loaded page is the one that was requested.
+  TracePageLoadEnd(validated_url);
   int http_status_code = 0;
-  GURL final_url;
   content::NavigationEntry* nav_entry =
       web_contents()->GetController().GetVisibleEntry();
   if (nav_entry) {
     http_status_code = nav_entry->GetHttpStatusCode();
-    final_url = nav_entry->GetVirtualURL();
   }
-  TracePageLoadEnd(final_url);
 
   if (http_status_code != 0 && http_status_code / 100 != 2) {
-    // We successfully loaded an error HTML page.
-    LOG(INFO) << "Failed loading page for: " << final_url
+    // An error HTML page was loaded instead of the content we requested.
+    LOG(ERROR) << "Failed loading page for: " << validated_url
                << "; http status code: " << http_status_code;
     Stop(net::ERR_FAILED);
     DCHECK_EQ(page_state_, PageState::ERROR);
     return;
   }
-  // Main frame finished loading.
+  // Main frame finished loading properly.
   base::TimeDelta load_time = base::TimeTicks::Now() - start_loading_ticks_;
   LOG(INFO) << "Finished loading page after " << load_time.InMilliseconds()
-            << " ms, url=" << final_url;
-  PageState previous = page_state_;
+            << " ms, url=" << validated_url;
+  OnPageLoaded();
+}
+
+void CastWebContentsImpl::DidFailLoad(
+    content::RenderFrameHost* render_frame_host,
+    const GURL& validated_url,
+    int error_code,
+    const base::string16& error_description) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  // Only report an error if we are the main frame.  See b/8433611.
+  if (render_frame_host->GetParent()) {
+    LOG(ERROR) << "Got error on sub-iframe: url=" << validated_url.spec()
+               << ", error=" << error_code;
+    return;
+  }
+  if (error_code == net::ERR_ABORTED) {
+    // ERR_ABORTED means download was aborted by the app, typically this happens
+    // when flinging URL for direct playback, the initial URLRequest gets
+    // cancelled/aborted and then the same URL is requested via the buffered
+    // data source for media::Pipeline playback.
+    LOG(INFO) << "Load canceled: url=" << validated_url.spec();
+
+    // We consider the page to be fully loaded in this case, since the app has
+    // intentionally entered this state. If the app wanted to stop, it would
+    // have called window.close() instead.
+    OnPageLoaded();
+    return;
+  }
+
+  LOG(ERROR) << "Got error on load: url=" << validated_url.spec()
+             << ", error_code=" << error_code;
+
+  TracePageLoadEnd(validated_url);
+  Stop(error_code);
+  DCHECK_EQ(PageState::ERROR, page_state_);
+}
+
+void CastWebContentsImpl::OnPageLoading() {
+  closing_ = false;
+  stopped_ = false;
+  stop_notified_ = false;
+  main_frame_loaded_ = false;
+  last_error_ = net::OK;
+}
+
+void CastWebContentsImpl::OnPageLoaded() {
+  main_frame_loaded_ = true;
   UpdatePageState();
-  DCHECK((previous == PageState::ERROR && page_state_ == PageState::ERROR) ||
-         page_state_ == PageState::LOADED)
-      << "Page is in unexpected state: " << page_state_;
+  DCHECK(page_state_ == PageState::LOADED);
   NotifyObservers();
 }
 
@@ -380,10 +447,10 @@ void CastWebContentsImpl::UpdatePageState() {
     DCHECK(stopped_);
     page_state_ = PageState::DESTROYED;
   } else if (!stopped_) {
-    if (web_contents_->IsLoading()) {
-      page_state_ = PageState::LOADING;
-    } else {
+    if (main_frame_loaded_) {
       page_state_ = PageState::LOADED;
+    } else {
+      page_state_ = PageState::LOADING;
     }
   } else if (stopped_) {
     if (last_error_ != net::OK) {
@@ -413,33 +480,21 @@ void CastWebContentsImpl::NotifyObservers() {
   notifying_ = false;
 }
 
-void CastWebContentsImpl::DidFailLoad(
+void CastWebContentsImpl::ResourceLoadComplete(
     content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    int error_code,
-    const base::string16& error_description) {
-  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  // Only report an error if we are the main frame.  See b/8433611.
-  if (render_frame_host->GetParent()) {
-    LOG(ERROR) << "Got error on sub-iframe: url=" << validated_url.spec()
-               << ", error=" << error_code;
+    const content::GlobalRequestID& request_id,
+    const content::mojom::ResourceLoadInfo& resource_load_info) {
+  if (!web_contents_ || render_frame_host != web_contents_->GetMainFrame())
     return;
-  }
-  if (error_code == net::ERR_ABORTED) {
-    // ERR_ABORTED means download was aborted by the app, typically this happens
-    // when flinging URL for direct playback, the initial URLRequest gets
-    // cancelled/aborted and then the same URL is requested via the buffered
-    // data source for media::Pipeline playback.
-    LOG(INFO) << "Load canceled: url=" << validated_url.spec();
+  int net_error = resource_load_info.net_error;
+  if (net_error == net::OK)
     return;
+  LOG(ERROR) << "Resource \"" << resource_load_info.url << "\" failed to load "
+             << " with net_error=" << net_error
+             << ", description=" << net::ErrorToShortString(net_error);
+  for (auto& observer : observer_list_) {
+    observer.ResourceLoadFailed(this);
   }
-
-  LOG(ERROR) << "Got error on load: url=" << validated_url.spec()
-             << ", error_code=" << error_code;
-
-  TracePageLoadEnd(validated_url);
-  Stop(error_code);
-  DCHECK_EQ(PageState::ERROR, page_state_);
 }
 
 void CastWebContentsImpl::InnerWebContentsCreated(

chromecast/browser/cast_web_contents_impl.h

@@ -57,26 +57,34 @@ class CastWebContentsImpl : public CastWebContents,
   void AddObserver(Observer* observer) override;
   void RemoveObserver(Observer* observer) override;
 
- private:
-  // WebContentsObserver implementation:
+  // content::WebContentsObserver implementation:
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) override;
   void OnInterfaceRequestFromFrame(
       content::RenderFrameHost* /* render_frame_host */,
       const std::string& interface_name,
       mojo::ScopedMessagePipeHandle* interface_pipe) override;
   void RenderProcessGone(base::TerminationStatus status) override;
+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
-  void DidStartLoading() override;
-  void DidStopLoading() override;
+  void DidFinishLoad(content::RenderFrameHost* render_frame_host,
+                     const GURL& validated_url) override;
   void DidFailLoad(content::RenderFrameHost* render_frame_host,
                    const GURL& validated_url,
                    int error_code,
                    const base::string16& error_description) override;
+  void ResourceLoadComplete(
+      content::RenderFrameHost* render_frame_host,
+      const content::GlobalRequestID& request_id,
+      const content::mojom::ResourceLoadInfo& resource_load_info) override;
   void InnerWebContentsCreated(
       content::WebContents* inner_web_contents) override;
   void WebContentsDestroyed() override;
 
+ private:
+  void OnPageLoading();
+  void OnPageLoaded();
   void UpdatePageState();
   void NotifyObservers();
   void TracePageLoadBegin(const GURL& url);
@@ -98,6 +106,7 @@ class CastWebContentsImpl : public CastWebContents,
 
   const int tab_id_;
   base::TimeTicks start_loading_ticks_;
+  bool main_frame_loaded_;
   bool closing_;
   bool stopped_;
   bool stop_notified_;

chromecast/browser/test/data/load_cancel.html

+<!DOCTYPE html>
+<head>
+</head>
+<body>
+  <script type="text/javascript">
+    window.stop();
+  </script>
+</body>

chromecast/browser/test/data/missing_resource.html

+<!DOCTYPE html>
+<head>
+</head>
+<body>
+  <script type="text/javascript" src="this_file_does_not_exist.js"></script>
+</body>

chromecast/renderer/cast_content_renderer_client_simple.cc

@@ -7,7 +7,6 @@
 #include <memory>
 
 #include "base/memory/ptr_util.h"
-#include "ipc/message_filter.h"
 
 namespace chromecast {
 namespace shell {