PlzNavigate: Fix the FindInPageControllerTest.SearchWithinSpecialURL browser test.

content/browser/loader/resource_loader.cc

Index: content/browser/loader/resource_loader.cc
diff --git a/content/browser/loader/resource_loader.cc b/content/browser/loader/resource_loader.cc
index 1993da2f4a687ec9606ea633d777d7121c2575a0..82d54c6eb0ab427eb17e3fabb2694791ae6d0d32 100644
--- a/content/browser/loader/resource_loader.cc
+++ b/content/browser/loader/resource_loader.cc
@@ -25,6 +25,7 @@
 #include "content/browser/ssl/ssl_client_auth_handler.h"
 #include "content/browser/ssl/ssl_manager.h"
 #include "content/public/browser/resource_dispatcher_host_login_delegate.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
@@ -256,14 +257,19 @@ void ResourceLoader::OnReceivedRedirect(net::URLRequest* unused,
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
 
-  if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
-          info->GetChildID(), redirect_info.new_url)) {
-    DVLOG(1) << "Denied unauthorized request for "
-             << redirect_info.new_url.possibly_invalid_spec();
+  // With PlzNavigate for frame navigations this check is done in the
+  // NavigationRequest::OnRedirectChecksComplete() function.
+  if (!IsBrowserSideNavigationEnabled() || info->GetChildID() != -1 ||

[nasko, 2016/10/21 21:21:07]

Why check the process ID here? The check should be PlzNav and Frame navigation,
right?

[ananta, 2016/10/21 21:53:01]

For non plznavigate we want the check to happen 

[nasko, 2016/10/21 22:47:49]

Yes, that is already decided by "!IsBrowserSideNavigationEnabled()". What
happens if we have a bug in the non-PlzNavigate case where the child process id
is indeed -1? We will skip the check and default to success. This is bad from
security perspective, isn't it?

[Charlie Reis, 2016/10/21 23:01:43]

Agreed.  We should not have the child ID check here, allowing us to skip the
CanRequestURL check.

I also don't understand why the !IsResourceTypeFrame exception was added. 
CanRequestURL is not solely about navigation requests.  It's about all requests.

[ananta, 2016/10/22 00:20:42]

I removed the GetChildID check. The !IsBrowserSideNavigation() enabled check
will ensure that this works correctly for the non plznavigate case. The frame
checks have been added here with the expectation that this will get handled
eventually in NavigationRequest::OnRedirectChecksComplete() function. I verified
that the function does get called for redirects on the main and sub frames.

[Charlie Reis, 2016/10/24 22:04:36]

I see.  I was misreading it before-- the non-PlzNavigate case won't care about
the extra checks.

(I wonder if there's a way to make this code easier to read, like having a
"check_handled_elsewhere" bool set to IsBrowserSideNavigationEnabled() &&
IsResourceTypeFrame(info->GetResourceType()).  I suppose it's not a big deal,
though.)

[ananta, 2016/10/24 23:01:41]

Thanks. Done

+      !IsResourceTypeFrame(info->GetResourceType())) {
+    if (!ChildProcessSecurityPolicyImpl::GetInstance()->CanRequestURL(
+            info->GetChildID(), redirect_info.new_url)) {
+      DVLOG(1) << "Denied unauthorized request for "
+               << redirect_info.new_url.possibly_invalid_spec();
 
-    // Tell the renderer that this request was disallowed.
-    Cancel();
-    return;
+      // Tell the renderer that this request was disallowed.
+      Cancel();
+      return;
+    }
   }
 
   if (delegate_->HandleExternalProtocol(this, redirect_info.new_url)) {