Add a way to cancel deferred navigations in NavigationHandle

components/navigation_interception/intercept_navigation_throttle.cc

Index: components/navigation_interception/intercept_navigation_throttle.cc
diff --git a/components/navigation_interception/intercept_navigation_throttle.cc b/components/navigation_interception/intercept_navigation_throttle.cc
index cc256a1273e0a0889e1df2cbabd3f999c3350bbb..83542e55dd5969e6b8ee8df247466855557c62c6 100644
--- a/components/navigation_interception/intercept_navigation_throttle.cc
+++ b/components/navigation_interception/intercept_navigation_throttle.cc
@@ -8,13 +8,37 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
 
+using content::BrowserThread;
+
 namespace navigation_interception {
 
+namespace {
+
+using ChecksPerformedCallback = base::Callback<void(bool)>;
+
+// This is used to run |should_ignore_callback| if it can destroy the
+// WebContents (and the InterceptNavigationThrottle along). In that case,
+// |on_checks_performed_callback| will be a no-op.
+void RunCallback(
+    content::WebContents* web_contents,
+    const NavigationParams& navigation_params,
+    InterceptNavigationThrottle::CheckCallback should_ignore_callback,
+    ChecksPerformedCallback on_checks_performed_callback) {
+  bool should_ignore_navigation =
+      should_ignore_callback.Run(web_contents, navigation_params);
+  on_checks_performed_callback.Run(should_ignore_navigation);
+}
+
+}  // namespace
+
 InterceptNavigationThrottle::InterceptNavigationThrottle(
     content::NavigationHandle* navigation_handle,
-    CheckCallback should_ignore_callback)
+    CheckCallback should_ignore_callback,
+    bool run_callback_synchronously)
     : content::NavigationThrottle(navigation_handle),
-      should_ignore_callback_(should_ignore_callback) {}
+      should_ignore_callback_(should_ignore_callback),
+      run_callback_synchronously_(run_callback_synchronously),
+      weak_factory_(this) {}
 
 InterceptNavigationThrottle::~InterceptNavigationThrottle() {}
 
@@ -39,11 +63,40 @@ InterceptNavigationThrottle::CheckIfShouldIgnoreNavigation(bool is_redirect) {
       navigation_handle()->GetPageTransition(), is_redirect,
       navigation_handle()->IsExternalProtocol(), true);
 
-  bool should_ignore_navigation = should_ignore_callback_.Run(
-      navigation_handle()->GetWebContents(), navigation_params);
-  return should_ignore_navigation
-             ? content::NavigationThrottle::CANCEL_AND_IGNORE
-             : content::NavigationThrottle::PROCEED;
+  if (run_callback_synchronously_) {
+    bool should_ignore_navigation = should_ignore_callback_.Run(
+        navigation_handle()->GetWebContents(), navigation_params);
+    return should_ignore_navigation
+               ? content::NavigationThrottle::CANCEL_AND_IGNORE
+               : content::NavigationThrottle::PROCEED;
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&InterceptNavigationThrottle::RunCallbackAsynchronously,
+                 weak_factory_.GetWeakPtr(), navigation_params));
+  return DEFER;
+}
+
+void InterceptNavigationThrottle::RunCallbackAsynchronously(
+    const NavigationParams& navigation_params) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  // Run the callback in a helper function as it may lead ot the destruction of
+  // this InterceptNavigationThrottle.
+  RunCallback(

[nasko, 2015/11/04 22:44:26]

If the callback can destruct this object, then it doesn't seem correct to still
run it through this object. Shouldn't the PostTask on line 74 bind it to the
helper function? This way there is no object being destroyed that we return back
into.

The way the code is structured right now, the only guard is for
InterceptNavigationThrottle being deleted between the posting of the task and
the execution. But if the callback can delete the object, we aren't guarding
this.

[clamy, 2015/11/05 15:52:23]

The callback needs the WebContents to run, which may go away, hence why I can't
PostTask the execution of the callback (unless I pull out the routing id +
process id for the RVH and get the WebContents back using those).

This is why I made sure we would not execute anything in the object after
running the callback. I thought that having the handling of the result in a
second callback bound on a weak_ptr to this object would ensure the second
callback did not run when this was deleted.

+      navigation_handle()->GetWebContents(), navigation_params,
+      should_ignore_callback_,
+      base::Bind(&InterceptNavigationThrottle::OnAsynchronousChecksPerformed,
+                 weak_factory_.GetWeakPtr()));
+}
+
+void InterceptNavigationThrottle::OnAsynchronousChecksPerformed(
+    bool should_ignore_navigation) {
+  if (should_ignore_navigation)
+    navigation_handle()->CancelDeferredNavigation(true);
+  else
+    navigation_handle()->Resume();
 }
 
 }  // namespace navigation_interception