PlzNavigate: Fix the http/tests/security/cross-frame-access-parent-isolated-world.html and http/tes…

third_party/WebKit/Source/core/loader/FrameLoader.cpp

Index: third_party/WebKit/Source/core/loader/FrameLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/FrameLoader.cpp b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
index e664eb27cae54417d51390f29bb50218095ea927..9bf2eef1d2b206a28637dc60907f120db718bf87 100644
--- a/third_party/WebKit/Source/core/loader/FrameLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/FrameLoader.cpp
@@ -1355,7 +1355,13 @@ void FrameLoader::commitProvisionalLoad() {
   }
 
   client()->transitionToCommittedForNewPage();
-  m_frame->navigationScheduler().cancel();
+
+  // PlzNavigate: We need to ensure that script initiated navigations are
+  // honored. Exceptions being navigations initiated by the user.
+  if (!m_isNavigationHandledByClient ||
+      UserGestureIndicator::processingUserGesture()) {

[Nate Chapin, 2017/01/19 21:02:59]

Is this user gesture check necessary to get the tests to pass? It looks
different than what blink does for internal navigations, which seems less than
ideal.

[ananta, 2017/01/19 21:38:08]

The user gesture flag is passed via the FrameHostMsg_BeginNavigation IPC to the
browser and is sent back via the FrameMsg_CommitNavigation IPC back to the
renderer. This is then set in the form of a scoped user gesture for the duration
of the NavigateInternal() function.

The UserGestureIndicator::processingUserGesture() checks the same flag. Added a
comment explaining this.

[Nate Chapin, 2017/01/19 23:33:50]

Right, I got that. I just don't immediately see why you need to check
processingUserGesture() here. There's no similar special case for
processingUserGesture() when starting navigations without PlzNaviagate, is
there?

[ananta, 2017/01/20 00:00:33]

Based on our discussion, removing this check

+    m_frame->navigationScheduler().cancel();
+  }
   m_frame->editor().clearLastEditCommand();
 
   // If we are still in the process of initializing an empty document then its
@@ -1684,6 +1690,9 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
   // request.
   recordLatestRequiredCSP();
   modifyRequestForCSP(resourceRequest, nullptr);
+
+  bool navigationHandledByClient = m_isNavigationHandledByClient;

[Nate Chapin, 2017/01/19 21:02:59]

This should be probably have a "was" in the name, and note that
shouldContinueForNavigationPolicy may change its value?

[ananta, 2017/01/19 21:38:08]

Done.

+
   if (!shouldContinueForNavigationPolicy(
           resourceRequest, frameLoadRequest.substituteData(), nullptr,
           frameLoadRequest.shouldCheckMainWorldContentSecurityPolicy(),
@@ -1711,8 +1720,14 @@ void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
   m_provisionalDocumentLoader->setNavigationType(navigationType);
   m_provisionalDocumentLoader->setReplacesCurrentHistoryItem(
       type == FrameLoadTypeReplaceCurrentItem);
-  m_frame->navigationScheduler().cancel();
-  m_checkTimer.stop();
+
+  // PlzNavigate: We need to ensure that script initiated navigations are
+  // honored. Exceptions being navigations initiated by the user.
+  if (!navigationHandledByClient ||
+      UserGestureIndicator::processingUserGesture()) {
+    m_frame->navigationScheduler().cancel();
+    m_checkTimer.stop();
+  }
 
   m_loadType = type;