Change WebAuthFlow to use the new WebContentsObserver navigation notifications.

chrome/browser/extensions/api/identity/web_auth_flow.cc

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/identity/web_auth_flow.h"
 
 #include <utility>
 
 #include "base/base64.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/identity_private.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/grit/browser_resources.h"
 #include "components/guest_view/browser/guest_view_base.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/resource_request_details.h"
 #include "content/public/browser/web_contents.h"
 #include "crypto/random.h"
 #include "extensions/browser/app_window/app_window.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_system.h"
+#include "net/http/http_response_headers.h"
 #include "url/gurl.h"
 
 using content::RenderViewHost;
 using content::ResourceRedirectDetails;
 using content::WebContents;
 using content::WebContentsObserver;
 using guest_view::GuestViewBase;
 
 namespace extensions {
 
@@ skipping 124 matching lines @@
   WebContentsObserver::Observe(web_contents);
 
   registrar_.RemoveAll();
 }
 
 void WebAuthFlow::RenderProcessGone(base::TerminationStatus status) {
   if (delegate_)
     delegate_->OnAuthFlowFailure(WebAuthFlow::WINDOW_CLOSED);
 }
 
-void WebAuthFlow::DidStartProvisionalLoadForFrame(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    bool is_error_page,
-    bool is_iframe_srcdoc) {
-  if (!render_frame_host->GetParent())
-    BeforeUrlLoaded(validated_url);
-}
-
-void WebAuthFlow::DidFailProvisionalLoad(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    int error_code,
-    const base::string16& error_description,
-    bool was_ignored_by_handler) {
-  TRACE_EVENT_ASYNC_STEP_PAST1("identity",

[Michael Courage, 2016/09/22 16:25:36]

would be nice to keep the error tracing in the new code for future debugging. I
don't know if this has changed, but at the time I made this it was very
difficult for most people to figure out how to inspect Chrome Apps, and
relatively easy to turn on tracing.

[yzshen1, 2016/09/22 18:07:52]

I removed it because this doesn't have a corresponding TRACE_EVENT_ASYNC_BEGIN.
I thought it was outdated.

I added it back and also TRACE_EVENT_ASYNC_BEGIN/END. Thanks!

-                               "WebAuthFlow",
-                               this,
-                               "DidFailProvisionalLoad",
-                               "error_code",
-                               error_code);
-  if (delegate_)
-    delegate_->OnAuthFlowFailure(LOAD_FAILED);
-}
-
 void WebAuthFlow::DidGetRedirectForResourceRequest(
     const content::ResourceRedirectDetails& details) {
   BeforeUrlLoaded(details.new_url);
 }
 
 void WebAuthFlow::TitleWasSet(content::NavigationEntry* entry,
                               bool explicit_set) {
   if (delegate_)
     delegate_->OnAuthFlowTitleChange(base::UTF16ToUTF8(entry->GetTitle()));
 }
 
 void WebAuthFlow::DidStopLoading() {
   AfterUrlLoaded();
 }
 
-void WebAuthFlow::DidNavigateMainFrame(
-    const content::LoadCommittedDetails& details,
-    const content::FrameNavigateParams& params) {
-  if (delegate_ && details.http_status_code >= 400)
+void WebAuthFlow::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsInMainFrame())
+    BeforeUrlLoaded(navigation_handle->GetURL());

[yzshen1, 2016/09/16 22:32:44]

One question:
It seems the original code doesn't care whether there are multiple navigations
going on at the same time. So I didn't try to handle it either.
For example, is it possible that a new navigation N2 is started and later a
failure for a previous navigation N1 is received? It seems in this case we may
notify the delegate that the auth flow has failed. The delegate doesn't have a
way to tell whether that failure is related to N2 or N1. Is this a concern?

Thanks!

[clamy, 2016/09/19 10:02:54]

It is possible to have 2 navigation sin the main frame at the same time, and it
seems this code doesn't handle it. CaptivePortalTabHelper had a similar issue
(trying to detect when a navigation fails due to a captive portal). We solved it
by keeping track of the latest handle seen, and simulating starts again if the
other one commits first.

[yzshen1, 2016/09/19 15:32:56]

Thanks for the info! I will also wait for the WebAuthFlow owner's input and see
what they think about this issue.

[Michael Courage, 2016/09/19 19:11:43]

At the time I wrote this code I didn't have a great understanding of the precise
semantics of navigation callbacks, and in fact this is still true. I don't
remember for sure, but chances are reasonably good that I was not considering
multiple simultaneous navigations at all.

I don't have a strong opinion about how each case should be handled. The main
thing to keep in mind is that these windows don't have normal browser chrome, so
the users ability to understand and correct problems is diminished. I therefore
tried to be fairly aggressive about programmatic detection of these states.

+}
+
+void WebAuthFlow::DidFinishNavigation(
+    content::NavigationHandle* navigation_handle) {
+  bool failed =
+      navigation_handle->GetNetErrorCode() != net::OK ||

[clamy, 2016/09/19 10:02:54]

This seems like it would trigger for a subframe navigation as well. If we have
the following case:
- We have a main frame A and a subframe B. B is navigating.
- We start a navigation in A but it is slow.
- In the meantime the navigation in B fails.
We will trigger the auth flow failure.

[yzshen1, 2016/09/19 15:32:56]

It seems this is what the original code did. But what you said makes sense to
me.

+      (navigation_handle->IsInMainFrame() &&
+       navigation_handle->GetResponseHeaders()->response_code() >= 400);
+
+  if (failed && delegate_)
     delegate_->OnAuthFlowFailure(LOAD_FAILED);
 }
 
 }  // namespace extensions