Fix NavigationControllerBrowserTest. EnsureSamePageNavigationUpdatesFrameNavigationEntry browser nav

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 2d90ebb2f9db861d3fd425e46ce60d45039c25a8..218446898993733af3d660b34c0f4c58f3654d4f 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -1782,6 +1782,25 @@ void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
         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);