PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 8b82079aa7bc60fc6c74c3f5b813b6145973f607..c8e7b2d117a245cc3cbe7783b6693ecf4c57f332 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -1514,11 +1514,21 @@ void RenderFrameImpl::OnBeforeUnload() {
   CHECK(!frame_->parent());
 
   base::TimeTicks before_unload_start_time = base::TimeTicks::Now();
-  bool proceed = frame_->dispatchBeforeUnloadEvent();
+
+  // TODO(clamy): Pass the right value for is_reload here.

[dcheng, 2016/05/03 21:34:30]

Let's fix this in this CL as well: since this is somewhat web-visible, we should
try not to break it. It shouldn't be too hard to add this parameter to
FrameMsg_BeforeUnload.

[clamy, 2016/05/04 13:50:15]

Done.

+  blink::WebLocalFrame::BeforeUnloadReturnValue return_value =
+      frame_->dispatchBeforeUnloadEvent(false);
+  if (return_value ==
+      blink::WebLocalFrame::BeforeUnloadReturnValue::FrameDestroyed) {

[dcheng, 2016/05/03 21:34:30]

Let's just use weak_factory_.GetWeakPtr() to detect this condition. Then we
won't need the enum at all, and the code will look simpler. Some other things
that run script hold a |weak_self| pointer to check if |this| is still live.

[clamy, 2016/05/04 13:50:15]

Done.

+    // Return as the frame was destroyed by the BeforeUnload event.
+    return;
+  }
+
   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_,
+      return_value == blink::WebLocalFrame::BeforeUnloadReturnValue::Proceed,
+      before_unload_start_time, before_unload_end_time));
 }
 
 void RenderFrameImpl::OnSwapOut(
@@ -4852,6 +4862,20 @@ WebNavigationPolicy RenderFrameImpl::decidePolicyForNavigation(
     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.
+  if (!is_redirect && (!IsBrowserSideNavigationEnabled() ||
+                       info.urlRequest.checkForBrowserSideNavigation())) {
+    // PlzNavigate: this is not executed when commiting the navigation.
+    if (frame_->dispatchBeforeUnloadEvent(info.navigationType ==
+                                          blink::WebNavigationTypeReload) !=
+        blink::WebLocalFrame::BeforeUnloadReturnValue::Proceed) {
+      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() &&
@@ -5603,7 +5627,6 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request,
                                       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