Fix NavigationControllerBrowserTest. EnsureSamePageNavigationUpdatesFrameNavigationEntry browser nav

content/browser/frame_host/navigation_controller_impl.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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 1764 matching lines @@
 
   // If an interstitial page is showing, the previous renderer is blocked and
   // cannot make new requests.  Unblock (and disable) it to allow this
   // navigation to succeed.  The interstitial will stay visible until the
   // resulting DidNavigate.
   if (delegate_->GetInterstitialPage()) {
     static_cast<InterstitialPageImpl*>(delegate_->GetInterstitialPage())->
         CancelForNavigation();
   }
 
+  // Convert Enter-in-omnibox to a reload. This is what Blink does in
+  // FrameLoader, but we want to handle it here so that if the navigation is
+  // redirected or handled purely on the browser side in PlzNavigate we have the
+  // same behaviour as Blink would. Note that we don't want to convert to a
+  // reload for history navigations, so this must be above the retrieval of the
+  // pending_entry_ below when pending_entry_index_ is used.
+  if (reload_type == ReloadType::NONE && GetLastCommittedEntry() &&
+      pending_entry_ && pending_entry_->frame_tree_node_id() == -1 &&
+      pending_entry_->GetURL() == GetLastCommittedEntry()->GetURL() &&
+      !pending_entry_->GetHasPostData() &&
+      // Android uses data urls in WebViews. Skipping them here to see if
+      // it fixes http://crbug.com/664319. Should we check for OS_ANDROID here?
+      !pending_entry_->GetURL().SchemeIs(url::kDataScheme) &&

[Charlie Reis, 2016/12/15 22:28:09]

I like the idea of keeping essentially the same fix as before, but we should be
careful about just excluding all data URLs.

I note that the test this CL is enabling
(EnsureSamePageNavigationUpdatesFrameNavigationEntry) was added to prevent the
document's URL and origin from getting out of sync, which is bad for security. 
(See https://crbug.com/630103#c5.)  Would skipping this check mean PlzNavigate
is vulnerable to that bug for data URLs?

We could limit it further to the LoadDataWithBaseURL case, but I'd have the same
question there (about making that case vulnerable to the URL/origin mismatch).

I note that the lines below already tried to address Android WebView's
loadDataWithBaseURL.  Maybe they did so in the wrong way, and a different check
would work better?

Maybe it's worth talking with some Android Webview folks (e.g., sgurun or boliu)
to get a sense for why the original patch caused the Inbox bug
(https://crbug.com/664319).

[ananta, 2016/12/16 00:42:28]

Thanks. I checked with boliu. He mentioned that the virtual URL is what is
dislayed to the user, the URL contains the whole URL. In this case the data:,
etc. The base URL could contain whatever is passed by the app. I added a check
for the base URL as well based on our discussion.

Thanks

+      // This check is required for Android WebView loadDataWithBaseURL.
+      GetLastCommittedEntry()->GetVirtualURL() ==
+          pending_entry_->GetVirtualURL()) {
+    reload_type = ReloadType::NORMAL;
+  }
+
   // For session history navigations only the pending_entry_index_ is set.
   if (!pending_entry_) {
     CHECK_NE(pending_entry_index_, -1);
     pending_entry_ = entries_[pending_entry_index_].get();
   }
 
   // Any renderer-side debug URLs or javascript: URLs should be ignored if the
   // renderer process is not live, unless it is the initial navigation of the
   // tab.
   if (IsRendererDebugURL(pending_entry_->GetURL())) {
@@ skipping 282 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content