DevTools: fix Page.frameRequestedNavigation to report proper frame id when...

DevTools: fix Page.frameRequestedNavigation to report proper frame id when another frame is being navigated

- Replace FrameLoadRequest::SetClientRedirect() with SetClientRedirectReason()
    that implies the request is for client redirect and provides the reason;
- Move probe::frameRequestedNavigation to FrameLoader::StartNavigation()
    and as late there as possible, so we only fire it when we actually
    request browser to navigate;
- Pass Frame* target_frame to probe::frameRequestedNavigation() and report
    target frame as frameId if present, so that when a frame requests another
    frame to navigate, the id of the latter is reported.

Bug: b/130664370
Change-Id: Ia424542edc90d39977abf49335bb1a89345233a8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1570424
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#652240}

third_party/blink/renderer/core/frame/local_frame.cc

@@ -325,16 +325,6 @@ bool LocalFrame::IsLocalRoot() const {
   return Tree().Parent()->IsRemoteFrame();
 }
 
-static ClientNavigationReason ClientNavigationReasonFromRequest(
-    const FrameLoadRequest& request) {
-  if (request.Form()) {
-    if (request.GetResourceRequest().HttpMethod() == http_names::kPOST)
-      return ClientNavigationReason::kFormSubmissionPost;
-    return ClientNavigationReason::kFormSubmissionGet;
-  }
-  return ClientNavigationReason::kFrameNavigation;
-}
-
 void LocalFrame::ScheduleNavigation(Document& origin_document,
                                     const KURL& url,
                                     WebFrameLoadType frame_load_type,
@@ -350,12 +340,8 @@ void LocalFrame::Navigate(const FrameLoadRequest& request,
   if (!navigation_rate_limiter().CanProceed())
     return;
   if (request.ClientRedirect() == ClientRedirectPolicy::kClientRedirect) {
-    ClientNavigationReason reason = ClientNavigationReasonFromRequest(request);
     probe::FrameScheduledNavigation(this, request.GetResourceRequest().Url(),
-                                    0.0, reason);
-    probe::FrameRequestedNavigation(this, request.GetResourceRequest().Url(),
-                                    reason);
-
+                                    0.0, request.ClientRedirectReason());
     if (NavigationScheduler::MustReplaceCurrentItem(this))
       frame_load_type = WebFrameLoadType::kReplaceCurrentItem;
   }
@@ -541,11 +527,9 @@ void LocalFrame::Reload(WebFrameLoadType load_type) {
   FrameLoadRequest request = FrameLoadRequest(
       nullptr, loader_.ResourceRequestForReload(
                    load_type, ClientRedirectPolicy::kClientRedirect));
-  request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
+  request.SetClientRedirectReason(ClientNavigationReason::kReload);
   probe::FrameScheduledNavigation(this, request.GetResourceRequest().Url(), 0.0,
                                   ClientNavigationReason::kReload);
-  probe::FrameRequestedNavigation(this, request.GetResourceRequest().Url(),
-                                  ClientNavigationReason::kReload);
   loader_.StartNavigation(request, load_type);
   probe::FrameClearedScheduledNavigation(this);
 }

third_party/blink/renderer/core/frame/remote_frame.cc

@@ -20,6 +20,7 @@
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/page/plugin_script_forbidden_scope.h"
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
+#include "third_party/blink/renderer/core/probe/core_probes.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_layer.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_request.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_timing_info.h"
@@ -68,6 +69,8 @@ void RemoteFrame::ScheduleNavigation(Document& origin_document,
   frame_request.SetFrameType(
       IsMainFrame() ? network::mojom::RequestContextFrameType::kTopLevel
                     : network::mojom::RequestContextFrameType::kNested);
+  frame_request.SetClientRedirectReason(
+      ClientNavigationReason::kFrameNavigation);
   Navigate(frame_request, frame_load_type);
 }
 
@@ -108,6 +111,11 @@ void RemoteFrame::Navigate(const FrameLoadRequest& passed_request,
         frame->GetSecurityContext() &&
         frame->GetSecurityContext()->IsSandboxed(WebSandboxFlags::kDownloads);
     initiator_frame_is_ad = frame->IsAdSubframe();
+    if (passed_request.ClientRedirect() ==
+        ClientRedirectPolicy::kClientRedirect) {
+      probe::FrameRequestedNavigation(frame, this, url,
+                                      passed_request.ClientRedirectReason());
+    }
   }
 
   bool current_frame_has_download_sandbox_flag =

third_party/blink/renderer/core/html/forms/html_form_element.cc

@@ -494,7 +494,6 @@ void HTMLFormElement::ScheduleFormSubmission(FormSubmission* submission) {
   FrameLoadRequest frame_load_request =
       submission->CreateFrameLoadRequest(&GetDocument());
   frame_load_request.SetNavigationPolicy(submission->GetNavigationPolicy());
-  frame_load_request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
   frame_load_request.GetResourceRequest().SetHasUserGesture(
       LocalFrame::HasTransientUserActivation(GetDocument().GetFrame()));
   GetDocument().GetFrame()->Navigate(frame_load_request,

third_party/blink/renderer/core/inspector/browser_protocol.pdl

@@ -5579,7 +5579,7 @@ domain Page
   # Navigation may still be cancelled after the event is issued.
   experimental event frameRequestedNavigation
     parameters
-      # Id of the frame that has scheduled a navigation.
+      # Id of the frame that is being navigated.
       FrameId frameId
       # The reason for the navigation.
       ClientNavigationReason reason

third_party/blink/renderer/core/inspector/inspector_page_agent.cc

@@ -927,11 +927,11 @@ void InspectorPageAgent::FrameStoppedLoading(LocalFrame* frame) {
 }
 
 void InspectorPageAgent::FrameRequestedNavigation(
-    LocalFrame* frame,
+    Frame* target_frame,
     const KURL& url,
     ClientNavigationReason reason) {
   GetFrontend()->frameRequestedNavigation(
-      IdentifiersFactory::FrameId(frame),
+      IdentifiersFactory::FrameId(target_frame),
       ClientNavigationReasonToProtocol(reason), url.GetString());
 }
 

third_party/blink/renderer/core/inspector/inspector_page_agent.h

@@ -179,7 +179,7 @@ class CORE_EXPORT InspectorPageAgent final
   void FrameDetachedFromParent(LocalFrame*);
   void FrameStartedLoading(LocalFrame*);
   void FrameStoppedLoading(LocalFrame*);
-  void FrameRequestedNavigation(LocalFrame*,
+  void FrameRequestedNavigation(Frame* target_frame,
                                 const KURL&,
                                 ClientNavigationReason);
   void FrameScheduledNavigation(LocalFrame*,

third_party/blink/renderer/core/loader/appcache/application_cache_host.cc

@@ -160,7 +160,7 @@ void ApplicationCacheHost::SelectCacheWithManifest(const KURL& manifest_url) {
     // being loaded, because "foreign" entries are never picked during
     // navigation. see ApplicationCacheGroup::selectCache()
     FrameLoadRequest request(document, ResourceRequest(document->Url()));
-    request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
+    request.SetClientRedirectReason(ClientNavigationReason::kReload);
     frame->Navigate(request, WebFrameLoadType::kReplaceCurrentItem);
   }
 }

third_party/blink/renderer/core/loader/form_submission.cc

@@ -289,7 +289,9 @@ FrameLoadRequest FormSubmission::CreateFrameLoadRequest(
   if (!target_.IsEmpty())
     frame_request.SetFrameName(target_);
 
+  ClientNavigationReason reason = ClientNavigationReason::kFormSubmissionGet;
   if (method_ == FormSubmission::kPostMethod) {
+    reason = ClientNavigationReason::kFormSubmissionPost;
     frame_request.GetResourceRequest().SetHttpMethod(http_names::kPOST);
     frame_request.GetResourceRequest().SetHttpBody(form_data_);
 
@@ -301,6 +303,7 @@ FrameLoadRequest FormSubmission::CreateFrameLoadRequest(
           content_type_ + "; boundary=" + boundary_);
     }
   }
+  frame_request.SetClientRedirectReason(reason);
 
   frame_request.GetResourceRequest().SetUrl(RequestURL());
 

third_party/blink/renderer/core/loader/frame_load_request.h

@@ -75,8 +75,15 @@ struct CORE_EXPORT FrameLoadRequest {
   }
 
   ClientRedirectPolicy ClientRedirect() const { return client_redirect_; }
-  void SetClientRedirect(ClientRedirectPolicy client_redirect) {
-    client_redirect_ = client_redirect;
+
+  void SetClientRedirectReason(ClientNavigationReason reason) {
+    client_redirect_ = ClientRedirectPolicy::kClientRedirect;
+    client_navigation_reason_ = reason;
+  }
+
+  ClientNavigationReason ClientRedirectReason() const {
+    DCHECK_EQ(ClientRedirectPolicy::kClientRedirect, client_redirect_);
+    return client_navigation_reason_;
   }
 
   NavigationPolicy GetNavigationPolicy() const { return navigation_policy_; }
@@ -158,7 +165,11 @@ struct CORE_EXPORT FrameLoadRequest {
   ResourceRequest resource_request_;
   AtomicString frame_name_;
   AtomicString href_translate_;
+  // TODO(caseq): merge ClientRedirectPolicy and ClientNavigationReason.
+  // Currently, client_navigation_reason_ is set iff ClientRedirectPolicy
+  // is set to kClientRedirect.
   ClientRedirectPolicy client_redirect_;
+  ClientNavigationReason client_navigation_reason_;
   NavigationPolicy navigation_policy_ = kNavigationPolicyCurrentTab;
   WebTriggeringEventInfo triggering_event_info_ =
       WebTriggeringEventInfo::kNotFromEvent;

third_party/blink/renderer/core/loader/frame_loader.cc

@@ -948,6 +948,11 @@ void FrameLoader::StartNavigation(const FrameLoadRequest& passed_request,
     }
   }
 
+  if (request.ClientRedirect() == ClientRedirectPolicy::kClientRedirect) {
+    probe::FrameRequestedNavigation(frame_, frame_, url,
+                                    request.ClientRedirectReason());
+  }
+
   Client()->BeginNavigation(
       resource_request, request.GetFrameType(), origin_document,
       nullptr /* document_loader */, navigation_type,

third_party/blink/renderer/core/loader/navigation_scheduler.cc

@@ -97,7 +97,7 @@ class ScheduledRedirect final : public ScheduledNavigation {
           mojom::FetchCacheMode::kValidateCache);
       load_type = WebFrameLoadType::kReload;
     }
-    request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
+    request.SetClientRedirectReason(GetReason());
     frame->Loader().StartNavigation(request, load_type);
   }
 
@@ -156,7 +156,7 @@ class ScheduledFrameNavigation final : public ScheduledNavigation {
         CreateUserGestureIndicator();
     FrameLoadRequest request(OriginDocument(), ResourceRequest(Url()), "_self",
                              should_check_main_world_content_security_policy_);
-    request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
+    request.SetClientRedirectReason(GetReason());
     request.SetInputStartTime(InputTimestamp());
 
     if (blob_url_token_) {
@@ -260,8 +260,10 @@ void NavigationScheduler::ScheduleFrameNavigation(
         EqualIgnoringFragmentIdentifier(frame_->GetDocument()->Url(), url)) {
       FrameLoadRequest request(origin_document, ResourceRequest(url), "_self");
       request.SetInputStartTime(input_timestamp);
-      if (frame_load_type == WebFrameLoadType::kReplaceCurrentItem)
-        request.SetClientRedirect(ClientRedirectPolicy::kClientRedirect);
+      if (frame_load_type == WebFrameLoadType::kReplaceCurrentItem) {
+        request.SetClientRedirectReason(
+            ClientNavigationReason::kFrameNavigation);
+      }
       frame_->Loader().StartNavigation(request, frame_load_type);
       return;
     }
@@ -281,8 +283,6 @@ void NavigationScheduler::NavigateTask() {
   }
 
   ScheduledNavigation* redirect(redirect_.Release());
-  probe::FrameRequestedNavigation(frame_, redirect->Url(),
-                                  redirect->GetReason());
   redirect->Fire(frame_);
   probe::FrameClearedScheduledNavigation(frame_);
 }

third_party/blink/renderer/core/probe/core_probes.pidl

@@ -121,7 +121,7 @@ interface CoreProbes {
   void FrameOwnerContentUpdated([Keep] LocalFrame*, HTMLFrameOwnerElement*);
   void FrameStartedLoading([Keep] LocalFrame*);
   void FrameStoppedLoading([Keep] LocalFrame*);
-  void FrameRequestedNavigation([Keep] LocalFrame*, const KURL& url, ClientNavigationReason reason);
+  void FrameRequestedNavigation(LocalFrame*, Frame* target_frame, const KURL& url, ClientNavigationReason reason);
   void FrameScheduledNavigation([Keep] LocalFrame*, const KURL& url, double delay, ClientNavigationReason reason);
   void FrameClearedScheduledNavigation([Keep] LocalFrame*);
   void DidCreateWebSocket([Keep] ExecutionContext*, uint64_t identifier, const KURL& request_url, const String& protocol);

third_party/blink/web_tests/http/tests/inspector-protocol/page/frame-requested-navigation-subframe-expected.txt

+Tests that when a frame casues another frame to navigate, frameId is that of target frame
+Got frameRequestedNavigation from "subframe-same-origin", reason: formSubmissionPost
+Got frameRequestedNavigation from "subframe-cross-origin", reason: formSubmissionGet
+

third_party/blink/web_tests/http/tests/inspector-protocol/page/frame-requested-navigation-subframe.js

+(async function(testRunner) {
+  var {page, session, dp} = await testRunner.startBlank('Tests that when a frame casues another frame to navigate, frameId is that of target frame');
+
+  await dp.Page.enable();
+
+  const frameNameById = new Map();
+  function onFrameNavigated(event) {
+    const frame = event.params.frame;
+    frameNameById.set(frame.id, frame.name || '<no name>');
+  }
+  dp.Page.onFrameNavigated(onFrameNavigated);
+
+  dp.Target.onAttachedToTarget(e => {
+    const dp2 = session.createChild(e.params.sessionId).protocol;
+    dp2.Page.enable();
+    dp2.Page.onFrameNavigated(onFrameNavigated);
+    dp2.Runtime.runIfWaitingForDebugger();
+  });
+
+  await dp.Target.setAutoAttach({autoAttach: true, waitForDebuggerOnStart: true, flatten: true});
+  await session.navigate('resources/frame-requested-navigation-subframe.html');
+
+  async function waitForRequestedNavigationAndDump() {
+    const frameRequestedNavigation = (await dp.Page.onceFrameRequestedNavigation()).params;
+    const frameName = frameNameById.get(frameRequestedNavigation.frameId) || '<unknown frame>';
+    const reason = frameRequestedNavigation.reason;
+    testRunner.log(`Got frameRequestedNavigation from "${frameName}", reason: ${reason}`);
+  }
+
+  session.evaluate('document.forms[0].submit()');
+  await waitForRequestedNavigationAndDump();
+
+  session.evaluate('frameLoadPromise.then(() => document.forms[1].submit())');
+  await waitForRequestedNavigationAndDump();
+
+  testRunner.completeTest();
+})

third_party/blink/web_tests/http/tests/inspector-protocol/page/resources/frame-requested-navigation-subframe.html

+<html>
+  <body>
+    <form method="POST" action="/inspector-protocol/resources/iframe.html" target="subframe-same-origin">
+      <input type="hidden" name="step">
+    </form>
+    <form method="GET"
+        action="http://oopif.test:8000/inspector-protocol/resources/iframe.html"
+        target="subframe-cross-origin">
+      <input type="hidden" name="step">
+    </form>
+    <iframe name="subframe-same-origin"></iframe>
+    <iframe id="frame2" name="subframe-cross-origin" src="http://oopif.test:8000/inspector-protocol/resources/iframe.html"></iframe>
+    <script>
+    frameLoadPromise = new Promise(fulfill => document.getElementById("frame2").addEventListener("load", fulfill));
+    </script>
+  </body>
+</html>