Cleanup blink-side PlzNavigate logic

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2009, 2010, 2011 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/)
  * Copyright (C) 2008 Alp Toker <alp@atoker.com>
  * Copyright (C) Research In Motion Limited 2009. All rights reserved.
  * Copyright (C) 2011 Kris Jordan <krisjordan@gmail.com>
  * Copyright (C) 2011 Google Inc. All rights reserved.
@@ skipping 171 matching lines @@
 
 FrameLoader::FrameLoader(LocalFrame* frame)
     : m_frame(frame),
       m_progressTracker(ProgressTracker::create(frame)),
       m_inStopAllLoaders(false),
       m_checkTimer(TaskRunnerHelper::get(TaskType::Networking, frame),
                    this,
                    &FrameLoader::checkTimerFired),
       m_forcedSandboxFlags(SandboxNone),
       m_dispatchingDidClearWindowObjectInMainWorld(false),
-      m_protectProvisionalLoader(false),
-      m_isNavigationHandledByClient(false) {
+      m_protectProvisionalLoader(false) {
   DCHECK(m_frame);
   TRACE_EVENT_OBJECT_CREATED_WITH_ID("loading", "FrameLoader", this);
   takeObjectSnapshot();
 }
 
 FrameLoader::~FrameLoader() {
   // Verify that this FrameLoader has been detached.
   DCHECK(!m_progressTracker);
 }
 
@@ skipping 492 matching lines @@
   if (!allDescendantsAreComplete(frame))
     return false;
 
   // Don't notify if the frame is being detached.
   if (frame->isDetaching())
     return false;
 
   return true;
 }
 
-static bool shouldSendCompleteNotification(LocalFrame* frame,
-                                           bool isNavigationHandledByClient) {
+static bool shouldSendCompleteNotification(LocalFrame* frame) {
   // FIXME: We might have already sent stop notifications and be re-completing.
   if (!frame->isLoading())
     return false;
   // Only send didStopLoading() if there are no navigations in progress at all,
   // whether committed, provisional, or pending.
   return frame->loader().documentLoader()->sentDidFinishLoad() &&
          !frame->loader().hasProvisionalNavigation();
 }
 
 void FrameLoader::checkCompleted() {
@@ skipping 19 matching lines @@
     // Android.
     if (m_frame->isMainFrame())
       m_frame->document()->viewportDescription().reportMobilePageStats(m_frame);
     m_documentLoader->setSentDidFinishLoad();
     client()->dispatchDidFinishLoad();
     // Finishing the load can detach the frame when running layout tests.
     if (!m_frame->client())
       return;
   }
 
-  if (shouldSendCompleteNotification(m_frame, m_isNavigationHandledByClient)) {
+  if (shouldSendCompleteNotification(m_frame)) {
     m_progressTracker->progressCompleted();
     // Retry restoring scroll offset since finishing loading disables content
     // size clamping.
     restoreScrollPositionAndViewState();
     if (m_documentLoader)
       m_documentLoader->setLoadType(FrameLoadTypeStandard);
     m_frame->domWindow()->finishedLoading();
   }
 
   Frame* parent = m_frame->tree().parent();
@@ skipping 100 matching lines @@
     HistoryLoadType historyLoadType,
     ClientRedirectPolicy clientRedirect,
     Document* initiatingDocument) {
   // If we have a state object, we cannot also be a new navigation.
   DCHECK(!stateObject || frameLoadType == FrameLoadTypeBackForward);
 
   // If we have a provisional request for a different document, a fragment
   // scroll should cancel it.
   detachDocumentLoader(m_provisionalDocumentLoader);
 
-  // PlzNavigate: A fragment scroll should clear ongoing client navigations.
-  clearNavigationHandledByClient();
-
   if (!m_frame->host())
     return;
   saveScrollState();
 
   KURL oldURL = m_frame->document()->url();
   bool hashChange = equalIgnoringFragmentIdentifier(url, oldURL) &&
                     url.fragmentIdentifier() != oldURL.fragmentIdentifier();
   if (hashChange) {
     // If we were in the autoscroll/middleClickAutoscroll mode we want to stop
     // it before following the link to the anchor
@@ skipping 357 matching lines @@
       Document::NoDismissal)
     return;
 
   // If this method is called from within this method, infinite recursion can
   // occur (3442218). Avoid this.
   if (m_inStopAllLoaders)
     return;
 
   m_inStopAllLoaders = true;
 
-  if (m_isNavigationHandledByClient) {
-    client()->dispatchDidFailProvisionalLoad(
-        ResourceError::cancelledError(String()), StandardCommit);
-  }
-
-  clearNavigationHandledByClient();
-
   for (Frame* child = m_frame->tree().firstChild(); child;
        child = child->tree().nextSibling()) {
     if (child->isLocalFrame())
       toLocalFrame(child)->loader().stopAllLoaders();
   }
 
   m_frame->document()->suppressLoadEvent();
   if (m_documentLoader)
     m_documentLoader->fetcher()->stopFetching();
   m_frame->document()->cancelParsing();
@@ skipping 111 matching lines @@
     m_frame->page()->chromeClient().setEventListenerProperties(
         m_frame, WebEventListenerClass::MouseWheel,
         WebEventListenerProperties::Nothing);
     m_frame->page()->chromeClient().setEventListenerProperties(
         m_frame, WebEventListenerClass::TouchEndOrCancel,
         WebEventListenerProperties::Nothing);
   }
 
   client()->transitionToCommittedForNewPage();
 
-  // PlzNavigate: We need to ensure that script initiated navigations are
-  // honored.
-  if (!m_isNavigationHandledByClient)
-    m_frame->navigationScheduler().cancel();
+  m_frame->navigationScheduler().cancel();
 
   m_frame->editor().clearLastEditCommand();
 
   // If we are still in the process of initializing an empty document then its
   // frame is not in a consistent state for rendering, so avoid
   // setJSStatusBarText since it may cause clients to attempt to render the
   // frame.
   if (!m_stateMachine.creatingInitialEmptyDocument()) {
     LocalDOMWindow* window = m_frame->domWindow();
     window->setStatus(String());
@@ skipping 108 matching lines @@
                              const ResourceError& error) {
   if (!error.isCancellation() && m_frame->owner()) {
     // FIXME: For now, fallback content doesn't work cross process.
     if (m_frame->owner()->isLocal())
       m_frame->deprecatedLocalOwner()->renderFallbackContent();
   }
 
   HistoryCommitType historyCommitType =
       loadTypeToCommitType(loader->loadType());
   if (loader == m_provisionalDocumentLoader) {
+    if (!m_provisionalDocumentLoader->didStart())
+      InspectorInstrumentation::frameClearedScheduledClientNavigation(m_frame);
     m_provisionalDocumentLoader->setSentDidFinishLoad();
     client()->dispatchDidFailProvisionalLoad(error, historyCommitType);
     if (loader != m_provisionalDocumentLoader)
       return;
     detachDocumentLoader(m_provisionalDocumentLoader);
   } else {
     DCHECK_EQ(loader, m_documentLoader);
     if (m_frame->document()->parser())
       m_frame->document()->parser()->stopParsing();
     if (!m_documentLoader->sentDidFinishLoad()) {
@@ skipping 91 matching lines @@
         break;
     }
 
     if (i == targetFrames.size())
       shouldClose = true;
   }
 
   return shouldClose;
 }
 
-bool FrameLoader::shouldContinueForNavigationPolicy(
+NavigationPolicy FrameLoader::shouldContinueForNavigationPolicy(
     const ResourceRequest& request,
     const SubstituteData& substituteData,
     DocumentLoader* loader,
     ContentSecurityPolicyDisposition shouldCheckMainWorldContentSecurityPolicy,
     NavigationType type,
     NavigationPolicy policy,
     FrameLoadType frameLoadType,
     bool isClientRedirect,
     HTMLFormElement* form) {
   // Don't ask if we are loading an empty URL.
   if (request.url().isEmpty() || substituteData.isValid())
-    return true;
+    return NavigationPolicyCurrentTab;
 
   // If we're loading content into |m_frame| (NavigationPolicyCurrentTab), check
   // against the parent's Content Security Policy and kill the load if that
   // check fails, unless we should bypass the main world's CSP.
   if (policy == NavigationPolicyCurrentTab &&
       shouldCheckMainWorldContentSecurityPolicy == CheckContentSecurityPolicy) {
     Frame* parentFrame = m_frame->tree().parent();
     if (parentFrame) {
       ContentSecurityPolicy* parentPolicy =
           parentFrame->securityContext()->contentSecurityPolicy();
       if (!parentPolicy->allowFrameFromSource(request.url(),
                                               request.redirectStatus())) {
         // Fire a load event, as timing attacks would otherwise reveal that the
         // frame was blocked. This way, it looks like every other cross-origin
         // page load.
         m_frame->document()->enforceSandboxFlags(SandboxOrigin);
         m_frame->owner()->dispatchLoad();
-        return false;
+        return NavigationPolicyIgnore;
       }
     }
   }
 
   bool isFormSubmission = type == NavigationTypeFormSubmitted ||
                           type == NavigationTypeFormResubmitted;
   if (isFormSubmission &&
       !m_frame->document()->contentSecurityPolicy()->allowFormAction(
           request.url()))
-    return false;
+    return NavigationPolicyIgnore;
 
   bool replacesCurrentHistoryItem =
       frameLoadType == FrameLoadTypeReplaceCurrentItem;
   policy = client()->decidePolicyForNavigation(request, loader, type, policy,
                                                replacesCurrentHistoryItem,
                                                isClientRedirect, form);
-  if (policy == NavigationPolicyCurrentTab)
-    return true;
-  if (policy == NavigationPolicyIgnore)
-    return false;
-  if (policy == NavigationPolicyHandledByClient) {
-    setNavigationHandledByClient();
-    // Mark the frame as loading since the embedder is handling the navigation.
-    m_progressTracker->progressStarted(frameLoadType);
+  if (policy == NavigationPolicyCurrentTab ||
+      policy == NavigationPolicyIgnore ||
+      policy == NavigationPolicyHandledByClient ||
+      policy == NavigationPolicyHandledByClientForInitialHistory) {
+    return policy;
+  }
 
-    m_frame->navigationScheduler().cancel();
-
-    // If this is a form submit, dispatch that a form is being submitted
-    // since the embedder is handling the navigation.
-    if (form)
-      client()->dispatchWillSubmitForm(form);
-
-    m_frame->document()->cancelParsing();
-
-    return false;
-  }
   if (!LocalDOMWindow::allowPopUp(*m_frame) &&
       !UserGestureIndicator::utilizeUserGesture())
-    return false;
+    return NavigationPolicyIgnore;
   client()->loadURLExternally(request, policy, String(),
                               replacesCurrentHistoryItem);
-  return false;
+  return NavigationPolicyIgnore;
 }
 
-bool FrameLoader::checkLoadCanStart(FrameLoadRequest& frameLoadRequest,
-                                    FrameLoadType type,
-                                    NavigationPolicy navigationPolicy,
-                                    NavigationType navigationType) {
+NavigationPolicy FrameLoader::checkLoadCanStart(
+    FrameLoadRequest& frameLoadRequest,
+    FrameLoadType type,
+    NavigationPolicy navigationPolicy,
+    NavigationType navigationType) {
   if (m_frame->document()->pageDismissalEventBeingDispatched() !=
       Document::NoDismissal) {
-    return false;
+    return NavigationPolicyIgnore;
   }
 
   // Record the latest requiredCSP value that will be used when sending this
   // request.
   ResourceRequest& resourceRequest = frameLoadRequest.resourceRequest();
   recordLatestRequiredCSP();
   modifyRequestForCSP(resourceRequest, nullptr);
 
-  if (!shouldContinueForNavigationPolicy(
-          resourceRequest, frameLoadRequest.substituteData(), nullptr,
-          frameLoadRequest.shouldCheckMainWorldContentSecurityPolicy(),
-          navigationType, navigationPolicy, type,
-          frameLoadRequest.clientRedirect() ==
-              ClientRedirectPolicy::ClientRedirect,
-          frameLoadRequest.form())) {
-    return false;
-  }
-
-  m_frame->document()->cancelParsing();
-  detachDocumentLoader(m_provisionalDocumentLoader);
-
-  // beforeunload fired above, and detaching a DocumentLoader can fire events,
-  // which can detach this frame.
-  if (!m_frame->host())
-    return false;
-
-  return true;
+  return shouldContinueForNavigationPolicy(
+      resourceRequest, frameLoadRequest.substituteData(), nullptr,
+      frameLoadRequest.shouldCheckMainWorldContentSecurityPolicy(),
+      navigationType, navigationPolicy, type,
+      frameLoadRequest.clientRedirect() == ClientRedirectPolicy::ClientRedirect,
+      frameLoadRequest.form());
 }
 
 void FrameLoader::startLoad(FrameLoadRequest& frameLoadRequest,
                             FrameLoadType type,
                             NavigationPolicy navigationPolicy) {
   DCHECK(client()->hasWebView());
   ResourceRequest& resourceRequest = frameLoadRequest.resourceRequest();
   NavigationType navigationType = determineNavigationType(
       type, resourceRequest.httpBody() || frameLoadRequest.form(),
       frameLoadRequest.triggeringEvent());
   resourceRequest.setRequestContext(
       determineRequestContextFromNavigationType(navigationType));
   resourceRequest.setFrameType(m_frame->isMainFrame()
                                    ? WebURLRequest::FrameTypeTopLevel
                                    : WebURLRequest::FrameTypeNested);
 
-  if (!checkLoadCanStart(frameLoadRequest, type, navigationPolicy,
-                         navigationType)) {
-    if (m_isNavigationHandledByClient) {
-      // PlzNavigate: if the navigation is a commit of a client-handled
-      // navigation, record that there is no longer a navigation handled by the
-      // client.
-      if (!frameLoadRequest.resourceRequest().checkForBrowserSideNavigation()) {
-        clearNavigationHandledByClient();
-      } else {
-        DocumentLoader* loader = createDocumentLoader(
-            resourceRequest, frameLoadRequest, type, navigationType);
-        // PlzNavigate: If the navigation is handled by the client, then the
-        // didFinishDocumentLoad() event occurs before the
-        // didStartProvisionalLoad() notification which occurs after the
-        // navigation
-        // is committed. This causes a number of layout tests to fail. We
-        // workaround this by invoking the didStartProvisionalLoad()
-        // notification
-        // here. Consumers of the didStartProvisionalLoad() notification rely on
-        // the provisional loader and save navigation state in it. We want to
-        // avoid
-        // this dependency on the provisional loader. For now we create a
-        // temporary
-        // loader and pass it to the didStartProvisionalLoad() function.
-        // TODO(ananta)
-        // We should get rid of the dependency on the DocumentLoader in
-        // consumers
-        // of
-        // the didStartProvisionalLoad() notification.
-        client()->dispatchDidStartProvisionalLoad(loader);
-        DCHECK(loader);
-        loader->setSentDidFinishLoad();
-        loader->detachFromFrame();
-      }
+  bool hadPlaceholderClientDocumentLoader =
+      m_provisionalDocumentLoader && !m_provisionalDocumentLoader->didStart();
+  navigationPolicy = checkLoadCanStart(frameLoadRequest, type, navigationPolicy,
+                                       navigationType);
+  if (navigationPolicy == NavigationPolicyIgnore) {
+    if (hadPlaceholderClientDocumentLoader &&
+        !resourceRequest.checkForBrowserSideNavigation()) {
+      detachDocumentLoader(m_provisionalDocumentLoader);
     }
     return;
   }
 
+  // For PlzNavigate placeholder DocumentLoaders, don't send failure callbacks
+  // for a placeholder simply being replaced with a new DocumentLoader.
+  if (hadPlaceholderClientDocumentLoader)
+    m_provisionalDocumentLoader->setSentDidFinishLoad();
+  m_frame->document()->cancelParsing();
+  detachDocumentLoader(m_provisionalDocumentLoader);
+
+  // beforeunload fired above, and detaching a DocumentLoader can fire events,
+  // which can detach this frame.

[clamy, 2017/03/02 14:37:18]

How likely is it that detaching the DocumentLoader here will result in the frame
being deleted? This now happens just after we sent the navigation request to the
browser while it didn't happen before.

IIUC, this also mean that if we're trying to commit a navigation in this frame,
it will get cancelled by a new renderer-initiated navigation when previously it
wouldn't right? Though this closer to the non-PlzNavigate behavior I imagine.

[Nate Chapin, 2017/03/02 19:39:18]

In my experience, the primary user of this feature is clusterfuzz :)
I think the detachDocumentLoader() call can't run script in the
PlzNavigate-placeholder case, because of the setSentDidFinishLoad() call above.
Going through DocumentLoader::detachFromFrame(), stopFetching() will be a noop
in a placeholder, loadFailed() won't be called because sentDidFinishLoad() is
set to true above, and everything else should never trigger JS.

[clamy, 2017/03/03 13:45:34]

My question here was about the brief interval of time where we have a
non-placeholder provisional DocumentLoader when trying to commit the navigation.
I think it will get cancelled if we start a new renderer-initiated navigation,
as we'll try to create a placeholder provisional DocumentLoader and detach the
non-placeholder one we just had. Is that correct?

[Nate Chapin, 2017/03/03 20:19:14]

I believe that is correct. Though the possibly of a new renderer-initiated
navigation cancelling an in-process PlzNavigate commit is more tied to the if()
clause around m_frame->navigationScheduler().cancel(); below. That clause causes
a placeholder->commit PlzNavigate transition to not cancel scheduled
navigations, but anything else will (in effect, treating such a transition as a
continuation of a navigation, rather than a new provisional navigation).
external/wpt/html/browsers/windows/nested-browsing-contexts/frameElement.html
requires this behavior.

+  if (!m_frame->host())
+    return;
+
+  m_progressTracker->progressStarted(type);

[clamy, 2017/03/02 14:37:18]

Can we now call this all the time? We had an issue with one of the DevTools
layout tests where we would end up signalling the start of teh load twice, which
was unexpected.

[Nate Chapin, 2017/03/02 19:39:18]

Yes, see change to ProgressTracker.cpp. Alternately, I could just defend this
call with if (!m_frame->isLoading()).

[clamy, 2017/03/03 13:45:34]

Acknowledged.

+  // TODO(japhet): This case wants to flag the frame as loading and do nothing
+  // else. It'd be nice if it could go through the placeholder DocumentLoader
+  // path, too.
+  if (navigationPolicy == NavigationPolicyHandledByClientForInitialHistory)
+    return;
+  DCHECK(navigationPolicy == NavigationPolicyCurrentTab ||
+         navigationPolicy == NavigationPolicyHandledByClient);
+
   m_provisionalDocumentLoader = createDocumentLoader(
       resourceRequest, frameLoadRequest, type, navigationType);
 
   // PlzNavigate: We need to ensure that script initiated navigations are
   // honored.
-  if (!m_isNavigationHandledByClient) {
+  if (!hadPlaceholderClientDocumentLoader ||
+      navigationPolicy == NavigationPolicyHandledByClient) {
     m_frame->navigationScheduler().cancel();
     m_checkTimer.stop();
   }
 
   if (frameLoadRequest.form())
     client()->dispatchWillSubmitForm(frameLoadRequest.form());
 
-  bool isNavigationHandledByClient = m_isNavigationHandledByClient;
-  // If the loader wasn't waiting for the client to handle a navigation, update
-  // the progress tracker. Otherwise don't, as it was already notified before
-  // sending the navigation to teh client.
-  if (!m_isNavigationHandledByClient)
-    m_progressTracker->progressStarted(type);
-  else
-    m_isNavigationHandledByClient = false;
-
   m_provisionalDocumentLoader->appendRedirect(
       m_provisionalDocumentLoader->getRequest().url());
-  // TODO(ananta)
+  // TODO(ananta):
   // We should get rid of the dependency on the DocumentLoader in consumers of
   // the didStartProvisionalLoad() notification.
   client()->dispatchDidStartProvisionalLoad(m_provisionalDocumentLoader);
   DCHECK(m_provisionalDocumentLoader);
 
-  m_provisionalDocumentLoader->startLoadingMainResource();
-
-  // This should happen after the request is sent, so we don't use
-  // clearNavigationHandledByClient() above.
-  if (isNavigationHandledByClient)
+  if (navigationPolicy == NavigationPolicyCurrentTab) {
+    m_provisionalDocumentLoader->startLoadingMainResource();
+    // This should happen after the request is sent, so that the state
+    // the inspector stored in the matching frameScheduledClientNavigation()
+    // is available while sending the request.
     InspectorInstrumentation::frameClearedScheduledClientNavigation(m_frame);
+  } else {
+    InspectorInstrumentation::frameScheduledClientNavigation(m_frame);
+  }
 
   takeObjectSnapshot();
 }
 
 void FrameLoader::applyUserAgent(ResourceRequest& request) {
   String userAgent = this->userAgent();
   DCHECK(!userAgent.isNull());
   request.setHTTPUserAgent(AtomicString(userAgent));
 }
 
@@ skipping 187 matching lines @@
                             : defaultSubstituteDataForURL(request.url()),
       frameLoadRequest.clientRedirect());
 
   loader->setLoadType(loadType);
   loader->setNavigationType(navigationType);
   loader->setReplacesCurrentHistoryItem(loadType ==
                                         FrameLoadTypeReplaceCurrentItem);
   return loader;
 }
 
-void FrameLoader::setNavigationHandledByClient() {
-  m_isNavigationHandledByClient = true;
-  InspectorInstrumentation::frameScheduledClientNavigation(m_frame);
-}
-
-void FrameLoader::clearNavigationHandledByClient() {
-  m_isNavigationHandledByClient = false;
-  InspectorInstrumentation::frameClearedScheduledClientNavigation(m_frame);
-}
-
 }  // namespace blink