[EoC] Fix crash caused by invalid ukm_source_id

Bootstrap the id by always calling ReportEvent(FETCH_REQUESTED) before
fetching.
Report FETCH_DELAYED when we set the baseline timestamp.
Ensure that events generated by the tab and triggering
delayed fetch are not checked against page being loaded

Bug: 832712
Change-Id: Ida168013b0920a6428093af943f06762dfbe0600
Reviewed-on: https://chromium-review.googlesource.com/1012440
Commit-Queue: Patrick Noland <[email protected]>
Reviewed-by: Theresa <[email protected]>
Reviewed-by: Filip Gorski <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#551169}(cherry picked from commit ff3479f)
Reviewed-on: https://chromium-review.googlesource.com/1016863
Reviewed-by: Patrick Noland <[email protected]>
Cr-Commit-Position: refs/branch-heads/3396@{#84}
Cr-Branched-From: 9ef2aa8-refs/heads/master@{#550428}

Author: Patrick Noland

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

@@ -23,6 +23,7 @@
 import org.chromium.components.feature_engagement.EventConstants;
 import org.chromium.components.feature_engagement.FeatureConstants;
 import org.chromium.components.feature_engagement.Tracker;
+import org.chromium.content_public.browser.WebContents;
 import org.chromium.ui.widget.ViewRectProvider;
 
 import java.util.Collections;
@@ -158,6 +159,7 @@ public void onSettingsStateChanged(boolean enabled) {}
 
     @Override
     public void requestSuggestions(String url) {
+        reportEvent(ContextualSuggestionsEvent.FETCH_REQUESTED);
         mCurrentRequestUrl = url;
         mSuggestionsSource.fetchSuggestions(url, (suggestionsResult) -> {
             if (mSuggestionsSource == null) return;
@@ -182,6 +184,13 @@ public void clearState() {
         clearSuggestions();
     }
 
+    @Override
+    public void reportFetchDelayed(WebContents webContents) {
+        if (mTabModelSelector.getCurrentTab().getWebContents() == webContents) {
+            reportEvent(ContextualSuggestionsEvent.FETCH_DELAYED);
+        }
+    }
+
     /**
      * Called when suggestions are cleared either due to the user explicitly dismissing
      * suggestions via the close button or due to the FetchHelper signaling state should

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

@@ -17,6 +17,7 @@
 import org.chromium.chrome.browser.tabmodel.TabModelSelector;
 import org.chromium.chrome.browser.tabmodel.TabModelSelectorTabModelObserver;
 import org.chromium.chrome.browser.util.UrlUtilities;
+import org.chromium.content_public.browser.WebContents;
 
 import java.util.HashMap;
 import java.util.Map;
@@ -38,6 +39,11 @@ interface Delegate {
          * Called when the state should be reset e.g. when the user navigates away from a webpage.
          */
         void clearState();
+
+        /**
+         * Called when a document becomes eligible for fetching but the fetch is being delayed.
+         */
+        void reportFetchDelayed(WebContents webContents);
     }
 
     /** State of the tab with respect to fetching readiness */
@@ -71,8 +77,8 @@ boolean isTrackingPage() {
         }
 
         /**
-         * Sets the baseline from which the fetch delay is calculated (conceptually starting the
-         * timer).
+         * Sets the baseline from which the fetch delay is calculated if it was not
+         * already set (conceptually starting the timer).
          * @param fetchTimeBaselineMillis The new value to set the baseline fetch time to.
          * @return Whether the fetch time baseline was set.
          */
@@ -142,17 +148,23 @@ public void onUpdateUrl(Tab tab, String url) {
 
             @Override
             public void didFirstVisuallyNonEmptyPaint(Tab tab) {
-                assert !tab.isIncognito();
-                if (getTabFetchReadinessState(tab).setFetchTimeBaselineMillis(
-                            SystemClock.uptimeMillis())) {
-                    maybeStartFetch(tab);
-                }
+                setTimeBaselineAndMaybeFetch(tab);
             }
 
             @Override
             public void onPageLoadFinished(Tab tab) {
+                setTimeBaselineAndMaybeFetch(tab);
+            }
+
+            @Override
+            public void onLoadStopped(Tab tab, boolean toDifferentDocument) {
+                setTimeBaselineAndMaybeFetch(tab);
+            }
+
+            private void setTimeBaselineAndMaybeFetch(Tab tab) {
                 assert !tab.isIncognito();
-                if (maybeSetFetchReadinessBaseline(tab)) {
+                if (getTabFetchReadinessState(tab).setFetchTimeBaselineMillis(
+                            SystemClock.uptimeMillis())) {
                     maybeStartFetch(tab);
                 }
             }
@@ -254,6 +266,7 @@ private void maybeStartFetch(Tab tab) {
     }
 
     private void postDelayedFetch(final String url, final Tab tab, long delayMillis) {
+        mDelegate.reportFetchDelayed(tab.getWebContents());
         ThreadUtils.postOnUiThreadDelayed(new Runnable() {
             @Override
             public void run() {

chrome/browser/android/contextual_suggestions/contextual_suggestions_bridge.cc

@@ -134,6 +134,15 @@ void ContextualSuggestionsBridge::ReportEvent(
   ukm::SourceId ukm_source_id =
       ukm::GetSourceIdForWebContentsDocument(web_contents);
 
+  // It's possible but unlikely to be in this state; it can happen if we are
+  // triggering a fetch for a WebContents that does not have a committed
+  // navigation. This can happen, e.g., if we switched tabs and a navigation
+  // took a very long time to commit. TODO(pnoland): Check against this
+  // possiblity by deferring event reporting and fetching until we observe a
+  // commit.
+  if (ukm_source_id == ukm::kInvalidSourceId)
+    return;
+
   contextual_suggestions::ContextualSuggestionsEvent event =
       static_cast<contextual_suggestions::ContextualSuggestionsEvent>(
           j_event_id);

components/ntp_snippets/contextual/contextual_suggestions_fetch.cc

@@ -132,7 +132,6 @@ void ContextualSuggestionsFetch::Start(
     ReportFetchMetricsCallback metrics_callback,
     const scoped_refptr<network::SharedURLLoaderFactory>& loader_factory) {
   request_completed_callback_ = std::move(callback);
-  metrics_callback.Run(FETCH_REQUESTED);
   url_loader_ = MakeURLLoader();
   url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       loader_factory.get(),

components/ntp_snippets/contextual/contextual_suggestions_fetcher_impl_unittest.cc

@@ -269,8 +269,7 @@ TEST_F(ContextualSuggestionsFetcherTest, SingleSuggestionResponse) {
   ExpectResponsesMatch(std::move(callback), "Peek Text", DefaultClusters());
   EXPECT_EQ(metrics_callback.events,
             std::vector<ContextualSuggestionsEvent>(
-                {contextual_suggestions::FETCH_REQUESTED,
-                 contextual_suggestions::FETCH_COMPLETED}));
+                {contextual_suggestions::FETCH_COMPLETED}));
 }
 
 TEST_F(ContextualSuggestionsFetcherTest,
@@ -403,8 +402,7 @@ TEST_F(ContextualSuggestionsFetcherTest, ProtocolError) {
       ElementsAre(base::Bucket(/*min=*/net::HTTP_NOT_FOUND, /*count=*/1)));
   EXPECT_EQ(metrics_callback.events,
             std::vector<ContextualSuggestionsEvent>(
-                {contextual_suggestions::FETCH_REQUESTED,
-                 contextual_suggestions::FETCH_ERROR}));
+                {contextual_suggestions::FETCH_ERROR}));
 }
 
 TEST_F(ContextualSuggestionsFetcherTest, ServerUnavailable) {
@@ -424,8 +422,7 @@ TEST_F(ContextualSuggestionsFetcherTest, ServerUnavailable) {
                                /*count=*/1)));
   EXPECT_EQ(metrics_callback.events,
             std::vector<ContextualSuggestionsEvent>(
-                {contextual_suggestions::FETCH_REQUESTED,
-                 contextual_suggestions::FETCH_SERVER_BUSY}));
+                {contextual_suggestions::FETCH_SERVER_BUSY}));
 }
 
 TEST_F(ContextualSuggestionsFetcherTest, NetworkError) {
@@ -448,8 +445,7 @@ TEST_F(ContextualSuggestionsFetcherTest, NetworkError) {
 
   EXPECT_EQ(metrics_callback.events,
             std::vector<ContextualSuggestionsEvent>(
-                {contextual_suggestions::FETCH_REQUESTED,
-                 contextual_suggestions::FETCH_ERROR}));
+                {contextual_suggestions::FETCH_ERROR}));
 }
 
 TEST_F(ContextualSuggestionsFetcherTest, EmptyResponse) {
@@ -464,8 +460,7 @@ TEST_F(ContextualSuggestionsFetcherTest, EmptyResponse) {
 
   EXPECT_EQ(metrics_callback.events,
             std::vector<ContextualSuggestionsEvent>(
-                {contextual_suggestions::FETCH_REQUESTED,
-                 contextual_suggestions::FETCH_EMPTY}));
+                {contextual_suggestions::FETCH_EMPTY}));
 }
 
 TEST_F(ContextualSuggestionsFetcherTest, ResponseWithUnsetFields) {