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 4a1b6f93e2d16a899f2d54a0f0c1a2ffc38e3a16..292b513d2f28e21313098c4db418f4d5d6b35b12 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -4671,6 +4671,7 @@ void RenderFrameImpl::OnFailedNavigation(
   WebURLRequest failed_request = CreateURLRequestForNavigation(
       common_params, std::unique_ptr<StreamOverrideParameters>(),
       frame_->isViewSourceModeEnabled());
+  failed_request.setBypassBeforeUnload(true);
   SendFailedProvisionalLoad(failed_request, error, frame_);
 
   // This check should have been done on the browser side already.
@@ -4858,9 +4859,15 @@ WebNavigationPolicy RenderFrameImpl::decidePolicyForNavigation(
   if (IsBrowserSideNavigationEnabled() &&
       info.urlRequest.checkForBrowserSideNavigation() &&
       ShouldMakeNetworkRequestForURL(url)) {
-    BeginNavigation(&info.urlRequest, info.replacesCurrentHistoryItem,
-                    info.isClientRedirect);
-    return blink::WebNavigationPolicyHandledByClient;
+    // Check if the navigation can be sent to the browser, otherwise ignore it.
+    bool proceeding_with_navigation =
+        BeginNavigation(&info.urlRequest, info.replacesCurrentHistoryItem,
+                        info.isClientRedirect);
+    // DO NOT ADD CODE AFTER THIS FUNCTION. It executes the BeforeUnload event

[nasko, 2016/04/15 14:42:22]

nit: "AFTER THIS FUNCTION" < The function or function call?

+    // for the frame, which may delete the RenderFrameImpl.
+    return proceeding_with_navigation
+               ? blink::WebNavigationPolicyHandledByClient
+               : blink::WebNavigationPolicyIgnore;

[nasko, 2016/04/15 14:42:22]

Isn't the beforeunload event already invoked before getting to
decidePolicyForNavigation? It seems a bit strange to have content/ orchestrate
the beforeunload firing for renderer-initiated navigations. Let's have dcheng@
weigh in on this.

[clamy, 2016/04/15 14:45:54]

No it's executed right afterwards (hence why I have to do this).

[dcheng, 2016/04/15 17:21:42]

Hmm, we should try to figure out how to make Blink handle this correctly: after
all, it knows that the embedder is (probably) going to navigate it somewhere.
Similar to how we currently call progressStarted() today in the "handled by
client case" to make sure progress state is set up correctly, we should probably
enforce that beforeunload didn't cancel the load at this time.

[clamy, 2016/04/25 11:36:55]

The problem is that the call to decidePolicyForNavigation also triggers the
navigation being sent to the browser. So if we execute BeforeUnload afterwards,
BeforeUnload can no longer cancel the navigation.

If we want to keep executing BeforeUnload in the FrameLoader, then we can no
longer have decidePolicyForNavigation send the navigation to the browser.
Instead, the FrameLoader has to ask the client whether to continue with the
navigation or not, get a HandledByClient (whith no side effect). Then execute
the BeforeUnload event, and when that's done send back the navigation to the
client if it got a NavigationPolicy::HandledByClient in the first place.

I'm not sure which is more understandable between having BeginNavigation ask for
BeforeUnload to execute, or having decidePolicyForNavigation return
WebNavigationPolicy::HandledByClient when it's not actually handling anything
until WebFrame calls another function to handle the navigation.

[dcheng, 2016/04/26 06:32:31]

Hmm, I see the problem, but I think it's awkward to have to add a bit on
WebURLRequest to track this: from what I can tell, WebURLRequest is used for a
lot of things other than frame loads.

Let me think about this tonight, sorry for the delay.

[clamy, 2016/04/26 10:07:09]

The bit in WebURLRequest is not coming from this problem, it's there to not
execute BeforeUnload a second time when we commit the navigation. Even if I
change the patch so that BeforeUnload executes from the FrameLoader, I would
still need a way to skip it when we commit. Now since I already need to know
that the WebURLRequest corresponds to a navigation commit so that we don't send
it to the browser, I could probably just have a single boolean
isNavigationCommit that I can use to skip BeforeUnload on commits.

[dcheng, 2016/04/27 09:21:29]

Part of me thinks that it's weird to expose internal Blink details like this:
ideally, I think I would prefer to structure it like this:
1) We revert to the original pre-PlzNavigate navigation policy.
2) Assuming it should continue, Blink continues on its typical loading path.
3) Before actually performing the navigation in Blink, give the embedder a
chance to intercept (e.g. today's HandledByClient enum, but as a separate
callback rather than using an enumerator value).

However, thinking about this more, it seems like this won't work well for OOPIF:
we won't run beforeunload events in cross-origin frames... so maybe we should
move to a model where the embedder *always* runs the beforeunload events in
behalf of Blink? WDYT?

[clamy, 2016/04/27 11:29:27]

I'm not really sure how OOPIF works here in the current architecture so not
comenting on this.

For PlzNavigate, what you suggest can work, but we would still have to know in
Blink when we are committing a navigation so that we don't execute the
BeforeUnload event again, and we don't send the navigation to the browser.

Note that when PlzNavigate lands, I have plans to do more changes in the
FrameLoader. In particular, have two separated methods, one that handles the
renderer-initiated load until it is sent to the browser. The other would do the
navigation commit.

[dcheng, 2016/04/27 13:42:04]

I think there should only be one place that runs the beforeunload events. I
believe that will be the case with either suggestion.

If we keep it wholly inside Blink, we'll have add a hook for PlzNavigate after
running the beforeunload events so content won't need to run it again even with
PlzNavigate on.

Alternatively, if we move the trigger location for beforeunload into the
embedder, then we should only have the embedder trigger it, even if PlzNavigate
is off. This will be a bit trickier, since we'll need to be careful not to touch
the RenderFrame after the beforeunload events run.

(My personal hope is that we could take advantage of approach #2 in the long run
to clean up some things for OOPIF, but I haven't thought through all the
implications yet.)

   }
 
   return info.defaultPolicy;
@@ -5291,6 +5298,13 @@ void RenderFrameImpl::NavigateInternal(
       CreateURLRequestForNavigation(common_params, std::move(stream_params),
                                     frame_->isViewSourceModeEnabled());
 
+  // PlzNavigate: If the navigation made a network request, the BeforeUnload
+  // event has already executed. Bypass it when committing the navigation.
+  if (IsBrowserSideNavigationEnabled() &&
+      ShouldMakeNetworkRequestForURL(common_params.url)) {
+    request.setBypassBeforeUnload(true);
+  }
+
   // Used to determine whether this frame is actually loading a request as part
   // of a history navigation.
   bool has_history_navigation_in_frame = false;
@@ -5597,12 +5611,16 @@ void RenderFrameImpl::PrepareRenderViewForNavigation(
   }
 }
 
-void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request,
+bool RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request,
                                       bool should_replace_current_entry,
                                       bool is_client_redirect) {
   CHECK(IsBrowserSideNavigationEnabled());
   DCHECK(request);
-  // TODO(clamy): Execute the beforeunload event.
+
+  // Execute the BeforeUnload for main frames, If asked not to proceed, do not
+  // send the navigation request to the browser.
+  if (!frame_->dispatchBeforeUnloadEvent())
+    return false;
 
   // 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
@@ -5653,6 +5671,7 @@ void RenderFrameImpl::BeginNavigation(blink::WebURLRequest* request,
                             request->skipServiceWorker(),
                             GetRequestContextTypeForWebURLRequest(*request)),
       GetRequestBodyForWebURLRequest(*request)));
+  return true;
 }
 
 void RenderFrameImpl::LoadDataURL(