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 4653 matching lines @@
   // that the load is properly tracked by the WebNavigation API.
   Send(new FrameHostMsg_DidStartProvisionalLoad(
       routing_id_, common_params.url, common_params.navigation_start));
 
   // Send the provisional load failure.
   blink::WebURLError error =
       CreateWebURLError(common_params.url, has_stale_copy_in_cache, error_code);
   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.
   if (!ShouldDisplayErrorPageForFailedLoad(error_code, common_params.url)) {
     NOTREACHED();
     return;
   }
 
   // Make sure errors are not shown in view source mode.
   frame_->enableViewSourceMode(false);
@@ skipping 167 matching lines @@
     OpenURL(url, Referrer(), info.defaultPolicy,
             info.replacesCurrentHistoryItem, false);
     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;
+    // 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;
 }
 
 void RenderFrameImpl::OnGetSavableResourceLinks() {
   std::vector<GURL> resources_list;
   std::vector<SavableSubframe> subframes;
   SavableResourcesResult result(&resources_list, &subframes);
 
@@ skipping 410 matching lines @@
           ? blink::WebFrameLoadType::ReplaceCurrentItem
           : blink::WebFrameLoadType::Standard;
   blink::WebHistoryLoadType history_load_type =
       blink::WebHistoryDifferentDocumentLoad;
   bool should_load_request = false;
   WebHistoryItem item_for_history_navigation;
   WebURLRequest request =
       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;
 
 #if defined(OS_ANDROID)
   request.setHasUserGesture(start_params.has_user_gesture);
 #endif
 
   // PlzNavigate: Make sure that Blink's loader will not try to use browser side
   // navigation for this request (since it already went to the browser).
@@ skipping 286 matching lines @@
   render_view_->history_list_offset_ =
       request_params.current_history_list_offset;
   render_view_->history_list_length_ =
       request_params.current_history_list_length;
   if (request_params.should_clear_history_list) {
     CHECK_EQ(-1, render_view_->history_list_offset_);
     CHECK_EQ(0, render_view_->history_list_length_);
   }
 }
 
-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
   // 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 30 matching lines @@
 
   Send(new FrameHostMsg_BeginNavigation(
       routing_id_,
       MakeCommonNavigationParams(request, should_replace_current_entry),
       BeginNavigationParams(GetWebURLRequestHeaders(*request),
                             GetLoadFlagsForWebURLRequest(*request),
                             request->hasUserGesture(),
                             request->skipServiceWorker(),
                             GetRequestContextTypeForWebURLRequest(*request)),
       GetRequestBodyForWebURLRequest(*request)));
+  return true;
 }
 
 void RenderFrameImpl::LoadDataURL(
     const CommonNavigationParams& params,
     const RequestNavigationParams& request_params,
     WebLocalFrame* frame,
     blink::WebFrameLoadType load_type,
     blink::WebHistoryItem item_for_history_navigation,
     blink::WebHistoryLoadType history_load_type,
     bool is_client_redirect) {
@@ skipping 371 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