PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

content/renderer/render_frame_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/render_frame_impl.h"
 
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 1487 matching lines @@
 
 ManifestManager* RenderFrameImpl::manifest_manager() {
   return manifest_manager_;
 }
 
 void RenderFrameImpl::SetPendingNavigationParams(
     std::unique_ptr<NavigationParams> navigation_params) {
   pending_navigation_params_ = std::move(navigation_params);
 }
 
-void RenderFrameImpl::OnBeforeUnload() {
+void RenderFrameImpl::OnBeforeUnload(bool is_reload) {
   TRACE_EVENT1("navigation", "RenderFrameImpl::OnBeforeUnload",
                "id", routing_id_);
   // TODO(creis): Right now, this is only called on the main frame.  Make the
   // browser process send dispatchBeforeUnloadEvent to every frame that needs
   // it.
   CHECK(!frame_->parent());
 
   base::TimeTicks before_unload_start_time = base::TimeTicks::Now();
-  bool proceed = frame_->dispatchBeforeUnloadEvent();
+
+  // Keep a WeakPtr to this RenderFrameHost to detect if executing the

[nasko, 2016/05/09 21:56:04]

RenderFrame. The RFH is browser side.

[clamy, 2016/05/10 01:58:09]

Removed this part as per dcheng's comment.

+  // BeforeUnload event destriyed this frame.

[nasko, 2016/05/09 21:56:04]

destroyed

[clamy, 2016/05/10 01:58:09]

Same.

+  base::WeakPtr<RenderFrameImpl> weak_self = weak_factory_.GetWeakPtr();
+  bool proceed = frame_->dispatchBeforeUnloadEvent(is_reload);
+  if (!weak_self)
+    return;

[dcheng, 2016/05/04 18:41:35]

It's probably slightly safer (and consistent with current behavior) to not check
weak_self here.

[clamy, 2016/05/10 01:58:09]

Done.

+
   base::TimeTicks before_unload_end_time = base::TimeTicks::Now();
-  Send(new FrameHostMsg_BeforeUnload_ACK(routing_id_, proceed,
-                                         before_unload_start_time,
-                                         before_unload_end_time));
+  Send(new FrameHostMsg_BeforeUnload_ACK(
+      routing_id_, proceed, before_unload_start_time, before_unload_end_time));
 }
 
 void RenderFrameImpl::OnSwapOut(
     int proxy_routing_id,
     bool is_loading,
     const FrameReplicationState& replicated_frame_state) {
   TRACE_EVENT1("navigation", "RenderFrameImpl::OnSwapOut", "id", routing_id_);
   RenderFrameProxy* proxy = NULL;
 
   // This codepath should only be hit for subframes when in --site-per-process.
@@ skipping 3313 matching lines @@
       // Must be a JavaScript navigation, which appears as "other".
       info.navigationType == blink::WebNavigationTypeOther;
 
   if (is_fork) {
     // Open the URL via the browser, not via WebKit.
     OpenURL(url, Referrer(), info.defaultPolicy,
             info.replacesCurrentHistoryItem, false);
     return blink::WebNavigationPolicyIgnore;
   }
 
+  // Execute the BeforeUnload event. If asked not to proceed or the frame is
+  // destroyed, ignore the navigation. There is no need to execute the
+  // BeforeUnload event during a redirect, since it was already executed at the
+  // start of the navigation.
+  // PlzNavigate: this is not executed when commiting the navigation.
+  if (!is_redirect && (!IsBrowserSideNavigationEnabled() ||
+                       info.urlRequest.checkForBrowserSideNavigation())) {

[nasko, 2016/05/09 21:56:04]

I'm slightly puzzled by this check. If PlzNavagaite is not enabled, why does it
expect checkForBrowserSideNavigation() to be true?

+    // Keep a WeakPtr to this RenderFrameHost to detect if executing the
+    // BeforeUnload event destriyed this frame.

[nasko, 2016/05/09 21:56:04]

This comment is a copy of the above, so fixes of the original comment should be
applied here too.

+    base::WeakPtr<RenderFrameImpl> weak_self = weak_factory_.GetWeakPtr();
+
+    if (!frame_->dispatchBeforeUnloadEvent(info.navigationType ==
+                                           blink::WebNavigationTypeReload) ||
+        !weak_self) {
+      return blink::WebNavigationPolicyIgnore;
+    }
+  }
+
   // PlzNavigate: if the navigation is not synchronous, send it to the browser.
   // This includes navigations with no request being sent to the network stack.
   if (IsBrowserSideNavigationEnabled() &&
       info.urlRequest.checkForBrowserSideNavigation() &&
       ShouldMakeNetworkRequestForURL(url)) {
     BeginNavigation(&info.urlRequest, info.replacesCurrentHistoryItem,
                     info.isClientRedirect);
     return blink::WebNavigationPolicyHandledByClient;
   }
 
@@ skipping 731 matching lines @@
     CHECK_EQ(-1, render_view_->history_list_offset_);
     CHECK_EQ(0, render_view_->history_list_length_);
   }
 }
 
 void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request,
                                       bool should_replace_current_entry,
                                       bool is_client_redirect) {
   CHECK(IsBrowserSideNavigationEnabled());
   DCHECK(request);
-  // TODO(clamy): Execute the beforeunload event.
 
   // Note: At this stage, the goal is to apply all the modifications the
   // renderer wants to make to the request, and then send it to the browser, so
   // that the actual network request can be started. Ideally, all such
   // modifications should take place in willSendRequest, and in the
   // implementation of willSendRequest for the various InspectorAgents
   // (devtools).
   //
   // TODO(clamy): Apply devtools override.
   // TODO(clamy): Make sure that navigation requests are not modified somewhere
@@ skipping 416 matching lines @@
                                     int match_count,
                                     int ordinal,
                                     const WebRect& selection_rect,
                                     bool final_status_update) {
   Send(new FrameHostMsg_Find_Reply(routing_id_, request_id, match_count,
                                    selection_rect, ordinal,
                                    final_status_update));
 }
 
 }  // namespace content