Reland "Fix Async Navigation Intercept using the wrong URL for fast redirects"
Original change's description:
Fix Async Navigation Intercept using the wrong URL for fast redirects
Prior to this change, if we received a redirect before the async
shouldIgnoreNavigation call was run (which does happen in practice) we
would still run the pending shouldIgnoreNavigation call but the
NavigationHandle would already be updated to the redirect URL and so
the result would potentially be wrong. It's also complicted to reason
about multiple checks in flight at once.
This change ensures that before processing the redirect, we finish
processing the previous step in the navigation (either the initial
navigation or a previous redirect).
Also, I moved the async task to Java to make use of the cached ExternalNavigationParams as the NavigationHandle is mutable and could be modified between when the task is posted and when it's run.
We'll still get the vast majority of the benefit from the async path
as it's extremely rare that we get a redirect before finishing the
check, so this should be performance neutral.
No new Kill Switch is needed for this change, as we can re-use the
kAsyncCheck Kill Switch to disable async nav altogether if we really
need to.
Bug: 381535042
Change-Id: I674aca65a4f2e5ca31670cf52929e8f87daf0528
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6194176
Reviewed-by: Bo Liu <[email protected]>
Commit-Queue: Michael Thiessen <[email protected]>
Reviewed-by: Ted Choc <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1410428}
diff --git a/components/navigation_interception/intercept_navigation_delegate.cc b/components/navigation_interception/intercept_navigation_delegate.cc
index 3604bb92..1bba9d50 100644
--- a/components/navigation_interception/intercept_navigation_delegate.cc
+++ b/components/navigation_interception/intercept_navigation_delegate.cc
@@ -47,18 +47,11 @@
const void* const kInterceptNavigationDelegateUserDataKey =
&kInterceptNavigationDelegateUserDataKey;
-bool CheckIfShouldIgnoreNavigationOnUIThread(
- content::NavigationHandle* navigation_handle) {
- DCHECK_CURRENTLY_ON(BrowserThread::UI);
- DCHECK(navigation_handle);
-
- InterceptNavigationDelegate* intercept_navigation_delegate =
- InterceptNavigationDelegate::Get(navigation_handle->GetWebContents());
- if (!intercept_navigation_delegate)
- return false;
-
- return intercept_navigation_delegate->ShouldIgnoreNavigation(
- navigation_handle);
+void AllowNavigationToProceed(
+ content::NavigationHandle* navigation_handle,
+ bool should_run_async,
+ InterceptNavigationThrottle::ResultCallback result_callback) {
+ std::move(result_callback).Run(false);
}
class RedirectURLLoader : public network::mojom::URLLoader {
@@ -161,9 +154,23 @@
if (!handle->IsInPrimaryMainFrame())
return nullptr;
+ InterceptNavigationDelegate* intercept_navigation_delegate =
+ InterceptNavigationDelegate::Get(handle->GetWebContents());
+
+ if (!intercept_navigation_delegate) {
+ return std::make_unique<InterceptNavigationThrottle>(
+ handle, base::BindRepeating(&AllowNavigationToProceed), mode,
+ base::DoNothing());
+ }
+
return std::make_unique<InterceptNavigationThrottle>(
- handle, base::BindRepeating(&CheckIfShouldIgnoreNavigationOnUIThread),
- mode);
+ handle,
+ base::BindRepeating(&InterceptNavigationDelegate::ShouldIgnoreNavigation,
+ base::Unretained(intercept_navigation_delegate)),
+ mode,
+ base::BindRepeating(
+ &InterceptNavigationDelegate::FinishPendingShouldIgnoreCheck,
+ base::Unretained(intercept_navigation_delegate)));
}
InterceptNavigationDelegate::InterceptNavigationDelegate(
@@ -175,21 +182,32 @@
InterceptNavigationDelegate::~InterceptNavigationDelegate() = default;
-bool InterceptNavigationDelegate::ShouldIgnoreNavigation(
- content::NavigationHandle* navigation_handle) {
+void InterceptNavigationDelegate::ShouldIgnoreNavigation(
+ content::NavigationHandle* navigation_handle,
+ bool should_run_async,
+ InterceptNavigationThrottle::ResultCallback result_callback) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+ // Avoid having two outstanding checks at once for simplicity.
+ if (should_ignore_result_callback_) {
+ FinishPendingShouldIgnoreCheck();
+ }
GURL escaped_url = escape_external_handler_value_
? GURL(base::EscapeExternalHandlerValue(
navigation_handle->GetURL().spec()))
: navigation_handle->GetURL();
- if (!escaped_url.is_valid())
- return false;
+ if (!escaped_url.is_valid()) {
+ std::move(result_callback).Run(false);
+ return;
+ }
JNIEnv* env = base::android::AttachCurrentThread();
ScopedJavaLocalRef<jobject> jdelegate = weak_jdelegate_.get(env);
- if (jdelegate.is_null())
- return false;
+ if (jdelegate.is_null()) {
+ std::move(result_callback).Run(false);
+ return;
+ }
bool hidden_cross_frame = false;
// Only main frame navigations use this path, so we only need to check if the
@@ -218,10 +236,28 @@
navigation_handle->SandboxFlagsInitiator() !=
network::mojom::WebSandboxFlags::kNone;
- return Java_InterceptNavigationDelegate_shouldIgnoreNavigation(
+ should_ignore_result_callback_ = std::move(result_callback);
+ Java_InterceptNavigationDelegate_callShouldIgnoreNavigation(
env, jdelegate, navigation_handle->GetJavaNavigationHandle(),
url::GURLAndroid::FromNativeGURL(env, escaped_url), hidden_cross_frame,
- is_sandboxed);
+ is_sandboxed, should_run_async);
+}
+
+void InterceptNavigationDelegate::OnShouldIgnoreNavigationResult(
+ bool should_ignore) {
+ std::move(should_ignore_result_callback_).Run(should_ignore);
+}
+
+void InterceptNavigationDelegate::FinishPendingShouldIgnoreCheck() {
+ JNIEnv* env = base::android::AttachCurrentThread();
+ ScopedJavaLocalRef<jobject> jdelegate = weak_jdelegate_.get(env);
+
+ if (jdelegate.is_null()) {
+ OnShouldIgnoreNavigationResult(false);
+ return;
+ }
+ Java_InterceptNavigationDelegate_finishPendingShouldIgnoreCheck(env,
+ jdelegate);
}
void InterceptNavigationDelegate::HandleSubframeExternalProtocol(
@@ -313,4 +349,19 @@
MaybeHandleSubframeAction();
}
+static void JNI_InterceptNavigationDelegate_OnShouldIgnoreNavigationResult(
+ JNIEnv* env,
+ const base::android::JavaParamRef<jobject>& jweb_contents,
+ jboolean should_ignore) {
+ content::WebContents* web_contents =
+ content::WebContents::FromJavaWebContents(jweb_contents);
+ if (!web_contents) {
+ return;
+ }
+ navigation_interception::InterceptNavigationDelegate* delegate =
+ navigation_interception::InterceptNavigationDelegate::Get(web_contents);
+ CHECK(delegate);
+ delegate->OnShouldIgnoreNavigationResult(should_ignore);
+}
+
} // namespace navigation_interception
