[WebLayer] Guard against duplicate tab destruction when launching intent

When the intent launching code determines that a tab should be closed
(e.g., because the initial navigation resulted in the launch of an
intent), it posts a task to the embedder to asynchronously close the
tab. We are seeing crashes in WebLayer indicating that in the interim
the tab is being closed for another reason. TabImpl does not guard
against its destroy() method being called multiple times, and the crash
results due to members being invoked and then nulled out in that method.

This CL adds a targeted fix for this flow, changing //weblayer's
InterceptNavigationDelegateClientImpl to short-circuit out of its
closeTab() method if it has already been destroyed. I avoided changing
TabImpl#destroy() to guard against multiple invocations as in general
we would regard any such flows as errors that should be detected and
fixed (like this one).

I investigated making a test for this case but so far have been unable
to program a flow where I can cause a tab opened via an intent launch
to be destroyed in the interim between the navigation starting and the
intent launching code destroying the tab. It is worth pointing out that
I still don't know what use case would result in this flow in production,
so this is a speculative fix. Nonetheless, it is a good change to make
in any case.

Bug: 1132983
Change-Id: I2b3c5a1598f6f13887f70bff90d822afbbd3f3d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2442626Reviewed-by: Bo <boliu@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812682}

weblayer/browser/java/org/chromium/weblayer_private/InterceptNavigationDelegateClientImpl.java

@@ -28,6 +28,7 @@ public class InterceptNavigationDelegateClientImpl implements InterceptNavigatio
     private RedirectHandler mRedirectHandler;
     private InterceptNavigationDelegateImpl mInterceptNavigationDelegate;
     private long mLastNavigationWithUserGestureTime = RedirectHandler.INVALID_TIME;
+    private boolean mDestroyed;
 
     InterceptNavigationDelegateClientImpl(TabImpl tab) {
         mTab = tab;
@@ -53,6 +54,7 @@ public class InterceptNavigationDelegateClientImpl implements InterceptNavigatio
     }
 
     public void destroy() {
+        mDestroyed = true;
         getWebContents().removeObserver(mWebContentsObserver);
         mInterceptNavigationDelegate.associateWithWebContents(null);
     }
@@ -118,6 +120,11 @@ public class InterceptNavigationDelegateClientImpl implements InterceptNavigatio
 
     @Override
     public void closeTab() {
+        // When InterceptNavigationDelegate determines that a tab needs to be closed, it posts a
+        // task invoking this method. It is possible that in the interim the tab was closed for
+        // another reason. In that case there is nothing more to do here.
+        if (mDestroyed) return;
+
         closeTab(mTab);
     }
 

weblayer/browser/java/org/chromium/weblayer_private/TabImpl.java

@@ -938,6 +938,7 @@ public final class TabImpl extends ITab.Stub implements LoginPrompt.Observer {
         }
 
         mInterceptNavigationDelegateClient.destroy();
+        mInterceptNavigationDelegateClient = null;
         mInterceptNavigationDelegate = null;
 
         mInfoBarContainer.destroy();