Don't recreate feature policy on fragment navigation

content/browser/frame_host/navigator_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/browser/frame_host/navigator_impl.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 667 matching lines @@
   // (see https://crbug.com/560511).  Instead, the next cross-process navigation
   // or transfer should decide whether to swap as if the net error had not
   // occurred.
   // TODO(creis): Remove this block and always set the URL once transfers handle
   // network errors or PlzNavigate is enabled.  See https://crbug.com/588314.
   if (!params.url_is_unreachable)
     render_frame_host->set_last_successful_url(params.url);
 
   // After setting the last committed origin, reset the feature policy in the
   // RenderFrameHost to a blank policy based on the parent frame.
-  render_frame_host->ResetFeaturePolicy();
+  if (!did_navigate)

[alexmos, 2017/02/24 21:21:12]

A couple of concerns with this.  First, I think you meant "if (did_navigate)",
as the negation will skip the ResetFeaturePolicy() on almost every navigation,
including normal cross-site navigations.

But also to skip fragments and other in-page navigations, "if
(!is_navigation_within_page)" seems to be the better thing to check. 
|did_navigate| returns false only for auto_subframe and ignored navigations, but
I think it's possible to navigate to a hash on the same page for other types of
navigations. (E.g., from comment on NAVIGATION_TYPE_NEW_PAGE: "cases like
navigations to a hash on the same page are NEW_PAGE").

(I also ran this by creis@ to double check, and he agreed.)

[iclelland, 2017/03/03 15:26:31]

Thanks for that -- I've updated the condition, to include both |did_navigate|
and !is_navigation_within_page, and added tests to ensure that the policy is not
reset on fragment navigation.

+    render_frame_host->ResetFeaturePolicy();
 
   // Send notification about committed provisional loads. This notification is
   // different from the NAV_ENTRY_COMMITTED notification which doesn't include
   // the actual URL navigated to and isn't sent for AUTO_SUBFRAME navigations.
   if (details.type != NAVIGATION_TYPE_NAV_IGNORE && delegate_) {
     DCHECK_EQ(!render_frame_host->GetParent(),
               did_navigate ? details.is_main_frame : false);
     navigation_handle->DidCommitNavigation(params, details.did_replace_entry,
                                            details.previous_url, details.type,
                                            render_frame_host);
@@ skipping 588 matching lines @@
     if (navigation_handle)
       navigation_handle->update_entry_id_for_transfer(entry->GetUniqueID());
 
     controller_->SetPendingEntry(std::move(entry));
     if (delegate_)
       delegate_->NotifyChangedNavigationState(content::INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content