RenderFrameImpl: do not call RFO::DidStartNavigation until actual start

Calling into WebLocalFrame::WillStartNavigation may cancel the navigation,
for example when dispatched javascript event handlers do "window.stop()".

Currently, we call RFO::DidStartNavigation prematurely, and never issue a
paired "canceled" call when WillStartNavigation fails. We can instead try
to start and only then issue observer notification to ensure proper sequence.

Tests which navigate from onload handler now correctly report "first
navigation finished, second navigation started" instead of the reverse.

onreadystatechange-detach previously reported a new navigation, which does not
actually happen because frame detaches itself before being able to start
navigating. This behavior is fixed now.

Bug: 855189
Change-Id: Ia12518ebddf54557f4699cd39168217762c874eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1800916Reviewed-by: Camille Lamy <clamy@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697044}

content/renderer/render_frame_impl.cc

@@ -6633,9 +6633,6 @@ void RenderFrameImpl::BeginNavigation(
     sync_navigation_callback_.Cancel();
     mhtml_body_loader_client_.reset();
 
-    for (auto& observer : observers_)
-      observer.DidStartNavigation(url, info->navigation_type);
-
     // First navigation in a frame to an empty document must be handled
     // synchronously.
     bool is_first_real_empty_document_navigation =
@@ -6644,6 +6641,8 @@ void RenderFrameImpl::BeginNavigation(
 
     if (is_first_real_empty_document_navigation &&
         !is_history_navigation_in_new_child_frame) {
+      for (auto& observer : observers_)
+        observer.DidStartNavigation(url, info->navigation_type);
       CommitSyncNavigation(std::move(info));
       return;
     }
@@ -6657,6 +6656,8 @@ void RenderFrameImpl::BeginNavigation(
       if (!frame_->WillStartNavigation(
               *info, false /* is_history_navigation_in_new_child_frame */))
         return;
+      for (auto& observer : observers_)
+        observer.DidStartNavigation(url, info->navigation_type);
       // Only the first navigation in a frame to an empty document must be
       // handled synchronously, the others are required to happen
       // asynchronously. So a PostTask is used.
@@ -7185,6 +7186,8 @@ void RenderFrameImpl::BeginNavigationInternal(
                                    is_history_navigation_in_new_child_frame))
     return;
 
+  for (auto& observer : observers_)
+    observer.DidStartNavigation(info->url_request.Url(), info->navigation_type);
   browser_side_navigation_pending_ = true;
   browser_side_navigation_pending_url_ = info->url_request.Url();
 

third_party/blink/web_tests/http/tests/history/post-replace-state-reload-expected.txt

@@ -2,8 +2,8 @@ main frame - didCommitLoadForFrame
 main frame - didReceiveTitle: 
 main frame - didFinishDocumentLoadForFrame
 main frame - BeginNavigation request to 'http://127.0.0.1:8000/history/post-replace-state-reload.html', http method POST
-main frame - DidStartNavigation
 main frame - didFinishLoadForFrame
+main frame - DidStartNavigation
 main frame - didHandleOnloadEventsForFrame
 main frame - ReadyToCommitNavigation
 main frame - didCommitLoadForFrame
@@ -11,8 +11,8 @@ main frame - didCommitLoadForFrame
 main frame - didReceiveTitle: 
 main frame - didFinishDocumentLoadForFrame
 main frame - BeginNavigation request to 'http://127.0.0.1:8000/history/post-replace-state-reload.html', http method GET
-main frame - DidStartNavigation
 main frame - didFinishLoadForFrame
+main frame - DidStartNavigation
 main frame - didHandleOnloadEventsForFrame
 main frame - ReadyToCommitNavigation
 main frame - didCommitLoadForFrame

third_party/blink/web_tests/http/tests/loading/307-after-303-after-post-expected.txt

@@ -4,8 +4,8 @@ main frame - didCommitLoadForFrame
 main frame - didReceiveTitle: 
 main frame - didFinishDocumentLoadForFrame
 main frame - BeginNavigation request to 'http://127.0.0.1:8000/loading/resources/post-to-303-target.php', http method POST
-main frame - DidStartNavigation
 main frame - didFinishLoadForFrame
+main frame - DidStartNavigation
 main frame - didHandleOnloadEventsForFrame
 main frame - ReadyToCommitNavigation
 main frame - didCommitLoadForFrame

third_party/blink/web_tests/http/tests/loading/onreadystatechange-detach-expected.txt

@@ -10,7 +10,6 @@ frame "iframe" - ReadyToCommitNavigation
 frame "iframe" - didCommitLoadForFrame
 CONSOLE MESSAGE: line 12: iframe: clicking
 frame "iframe" - BeginNavigation request to 'http://127.0.0.1:8000/loading/resources/onreadystatechange-detach-iframe.html?done', http method GET
-frame "iframe" - DidStartNavigation
 CONSOLE MESSAGE: line 5: iframe: onreadystatechange
 frame "iframe" - didFailLoadWithError
 main frame - didHandleOnloadEventsForFrame

third_party/blink/web_tests/http/tests/loading/redirect-methods-expected.txt

@@ -21,8 +21,8 @@ frame "0" - didCommitLoadForFrame
 frame "0" - didReceiveTitle: 
 frame "0" - didFinishDocumentLoadForFrame
 frame "0" - BeginNavigation request to 'http://127.0.0.1:8000/loading/resources/redirect-methods-result.php', http method POST
-frame "0" - DidStartNavigation
 frame "0" - didFinishLoadForFrame
+frame "0" - DidStartNavigation
 frame "0" - didHandleOnloadEventsForFrame
 frame "0" - ReadyToCommitNavigation
 frame "0" - didCommitLoadForFrame
@@ -46,8 +46,8 @@ frame "1" - didCommitLoadForFrame
 frame "1" - didReceiveTitle: 
 frame "1" - didFinishDocumentLoadForFrame
 frame "1" - BeginNavigation request to 'http://127.0.0.1:8000/loading/resources/redirect-methods-result.php', http method POST
-frame "1" - DidStartNavigation
 frame "1" - didFinishLoadForFrame
+frame "1" - DidStartNavigation
 frame "1" - didHandleOnloadEventsForFrame
 frame "1" - ReadyToCommitNavigation
 frame "1" - didCommitLoadForFrame
@@ -71,8 +71,8 @@ frame "2" - didCommitLoadForFrame
 frame "2" - didReceiveTitle: 
 frame "2" - didFinishDocumentLoadForFrame
 frame "2" - BeginNavigation request to 'http://127.0.0.1:8000/loading/resources/redirect-methods-result.php', http method POST
-frame "2" - DidStartNavigation
 frame "2" - didFinishLoadForFrame
+frame "2" - DidStartNavigation
 frame "2" - didHandleOnloadEventsForFrame
 frame "2" - ReadyToCommitNavigation
 frame "2" - didCommitLoadForFrame
@@ -96,8 +96,8 @@ frame "3" - didCommitLoadForFrame
 frame "3" - didReceiveTitle: 
 frame "3" - didFinishDocumentLoadForFrame
 frame "3" - BeginNavigation request to 'http://127.0.0.1:8000/loading/resources/redirect-methods-result.php', http method POST
-frame "3" - DidStartNavigation
 frame "3" - didFinishLoadForFrame
+frame "3" - DidStartNavigation
 frame "3" - didHandleOnloadEventsForFrame
 frame "3" - ReadyToCommitNavigation
 frame "3" - didCommitLoadForFrame

third_party/blink/web_tests/http/tests/loading/stop-load-at-commit-expected.txt

@@ -4,8 +4,8 @@ main frame - didCommitLoadForFrame
 main frame - didReceiveTitle: 
 main frame - didFinishDocumentLoadForFrame
 main frame - BeginNavigation request to 'http://127.0.0.1:8000/navigation/resources/pass-and-notify-done.html', http method GET
-main frame - DidStartNavigation
 main frame - didFinishLoadForFrame
+main frame - DidStartNavigation
 main frame - didHandleOnloadEventsForFrame
 main frame - ReadyToCommitNavigation
 main frame - didCommitLoadForFrame