[EoC] Fixing race condition between page load and canonical URL getting

* While investigating the problem of not reporting triggering on some
  pages, I discovered a race condition in fetching canonical URL ->
  in some cases the tab does not provide correct infromation yet.
* This patch postpones fetching of canonical URL until later, when
  we are about to perform the fetch.
* Fetching URLs would also cause some problems with marking fetch as in
  progress, which could prevent actual fetch from happening.
* It appears to also be positively affecting the EoC start time when
  loading a page for the first time.

Bug: 873004, 841811
Change-Id: Ic7e91579d083efd54a695a10e251ec245797ba84
Reviewed-on: https://chromium-review.googlesource.com/1175108Reviewed-by: Patrick Noland <pnoland@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Filip Gorski <fgorski@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583334}

chrome/android/java/src/org/chromium/chrome/browser/contextual_suggestions/FetchHelper.java

@@ -296,46 +296,11 @@ class FetchHelper {
     private void maybeStartFetch(final Tab tab) {
         if (tab == null || tab != mCurrentTab) return;
 
+        assert !tab.isIncognito();
+
         // Skip additional requests for current tab, until clearState is called.
         if (mFetchRequestedForCurrentTab) return;
 
-        if (shouldFetchCanonicalUrl(tab)) {
-            mFetchRequestedForCurrentTab = true;
-            String url = tab.getUrl();
-            tab.getWebContents().getMainFrame().getCanonicalUrlForSharing(new Callback<String>() {
-                @Override
-                public void onResult(String result) {
-                    if (tab != mCurrentTab) {
-                        return;
-                    }
-
-                    // Reset mFetchRequestedForCurrentTab so that it remains false if
-                    // maybeStartFetchWithCanonicalURLResolved doesn't end up requesting
-                    // suggestions. If it does end up requesting suggestions,
-                    // mFetchRequestedForCurrentTab will be set back to true in the same
-                    // synchronous flow of execution, so there shouldn't be a race condition.
-                    mFetchRequestedForCurrentTab = false;
-
-                    TabFetchReadinessState tabFetchReadinessState = getTabFetchReadinessState(tab);
-                    if (tabFetchReadinessState != null && tabFetchReadinessState.isTrackingPage()
-                            && tabFetchReadinessState.isContextTheSame(url)) {
-                        tabFetchReadinessState.setCanonicalUrl(result);
-                        maybeStartFetchWithCanonicalURLResolved(
-                                tab, getUrlToFetchFor(tab.getUrl(), result));
-                    }
-                }
-            });
-            return;
-        } else {
-            maybeStartFetchWithCanonicalURLResolved(tab, tab.getUrl());
-        }
-    }
-
-    private void maybeStartFetchWithCanonicalURLResolved(Tab tab, String resolvedUrl) {
-        if (tab == null || tab != mCurrentTab) return;
-
-        assert !tab.isIncognito();
-
         TabFetchReadinessState tabFetchReadinessState = getTabFetchReadinessState(mCurrentTab);
 
         // If we are not tracking a valid page, we can bail.
@@ -354,43 +319,54 @@ class FetchHelper {
             return;
         }
 
-        long remainingFetchDelayMillis =
+        long currentDelayMillis =
                 SystemClock.uptimeMillis() - tabFetchReadinessState.getFetchTimeBaselineMillis();
-        long minimumFetchDelayMillis = getMinimumFetchDelayMillis();
-        if (!sDisableDelayForTesting && remainingFetchDelayMillis < minimumFetchDelayMillis) {
-            postDelayedFetch(
-                    resolvedUrl, mCurrentTab, minimumFetchDelayMillis - remainingFetchDelayMillis);
+        long delayMillis = Math.max(0, getMinimumFetchDelayMillis() - currentDelayMillis);
+        final String url = tabFetchReadinessState.getUrl();
+
+        if (sDisableDelayForTesting || delayMillis == 0) {
+            getCanonicalUrlThenFetch(tab, url);
             return;
         }
 
-        mFetchRequestedForCurrentTab = true;
-        mDelegate.requestSuggestions(resolvedUrl);
+        mDelegate.reportFetchDelayed(tab.getWebContents());
+        ThreadUtils.postOnUiThreadDelayed(() -> getCanonicalUrlThenFetch(tab, url), delayMillis);
     }
 
-    private void postDelayedFetch(final String url, final Tab tab, long delayMillis) {
-        if (tab == null) {
-            assert false;
+    private void getCanonicalUrlThenFetch(final Tab tab, final String url) {
+        if (!shouldFetchCanonicalUrl(tab)) {
+            fetchSuggestions(tab, url);
             return;
         }
 
-        mDelegate.reportFetchDelayed(tab.getWebContents());
-        ThreadUtils.postOnUiThreadDelayed(new Runnable() {
+        tab.getWebContents().getMainFrame().getCanonicalUrlForSharing(new Callback<String>() {
             @Override
-            public void run() {
-                // Make sure that the tab is currently selected.
+            public void onResult(String result) {
                 if (tab != mCurrentTab) return;
 
-                if (mFetchRequestedForCurrentTab) return;
+                TabFetchReadinessState tabFetchReadinessState = getTabFetchReadinessState(tab);
+                if (tabFetchReadinessState != null && tabFetchReadinessState.isTrackingPage()
+                        && tabFetchReadinessState.isContextTheSame(url)) {
+                    tabFetchReadinessState.setCanonicalUrl(result);
+                    fetchSuggestions(tab, getUrlToFetchFor(tab.getUrl(), result));
+                }
+            }
+        });
+    }
 
-                if (!isObservingTab(tab)) return;
+    private void fetchSuggestions(final Tab tab, final String url) {
+        // Make sure that the tab is currently selected.
+        if (tab != mCurrentTab) return;
 
-                // URL in tab changed since the task was originally posted.
-                if (!getTabFetchReadinessState(tab).isContextTheSame(url)) return;
+        if (mFetchRequestedForCurrentTab) return;
 
-                mFetchRequestedForCurrentTab = true;
-                mDelegate.requestSuggestions(url);
-            }
-        }, delayMillis);
+        if (!isObservingTab(tab)) return;
+
+        // URL in tab changed since the task was originally posted.
+        if (!getTabFetchReadinessState(tab).isContextTheSame(url)) return;
+
+        mFetchRequestedForCurrentTab = true;
+        mDelegate.requestSuggestions(url);
     }
 
     private void clearState() {

chrome/android/junit/src/org/chromium/chrome/browser/contextual_suggestions/FetchHelperTest.java

@@ -426,7 +426,9 @@ public final class FetchHelperTest {
         FetchHelper helper = createFetchHelper();
 
         getTabObserver().onPageLoadFinished(mTab);
+        verify(mFrameHost, times(0)).getCanonicalUrlForSharing(any());
         runUntilFetchPossible();
+        verify(mFrameHost, times(1)).getCanonicalUrlForSharing(any());
         verify(mDelegate, times(1)).requestSuggestions(DIFFERENT_URL);
     }