Bug 1367024: Fix array index out of bounds in StreamRecyclerAdapter. r=liuche
authorMichael Comella <michael.l.comella@gmail.com>
Wed, 19 Jul 2017 16:35:44 -0700
changeset 421806 d133484d4c24a918d5fd8028b9e2f8727e15229b
parent 421805 91727c7a6f5bb08ea5297e6b82690ece6ca2cd38
child 421807 e46f1af0d4318b508508a3755a64dd071bbc0845
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersliuche
bugs1367024
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1367024: Fix array index out of bounds in StreamRecyclerAdapter. r=liuche I'm a little concerned that this will hide other bugs but it's pretty complicated to track down without an STR (e.g. what are all the possible ways we load the HomePager with TopSites as the default and swap in new highlights? How about unloading? When is onSizeChanged called?) so I don't think it's worth investigating further. MozReview-Commit-ID: 6OuFJ2iQsdL
mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
--- a/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java
@@ -2,37 +2,41 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 package org.mozilla.gecko.activitystream.homepanel;
 
 import android.database.Cursor;
 import android.support.v7.widget.RecyclerView;
+import android.util.Log;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
 
 import org.mozilla.gecko.Telemetry;
 import org.mozilla.gecko.TelemetryContract;
 import org.mozilla.gecko.activitystream.ActivityStreamTelemetry;
 import org.mozilla.gecko.home.HomePager;
 import org.mozilla.gecko.activitystream.homepanel.model.Highlight;
 import org.mozilla.gecko.activitystream.homepanel.stream.HighlightItem;
 import org.mozilla.gecko.activitystream.homepanel.stream.HighlightsTitle;
 import org.mozilla.gecko.activitystream.homepanel.stream.StreamItem;
 import org.mozilla.gecko.activitystream.homepanel.stream.TopPanel;
 import org.mozilla.gecko.activitystream.homepanel.stream.WelcomePanel;
+import org.mozilla.gecko.util.StringUtils;
 import org.mozilla.gecko.widget.RecyclerViewClickSupport;
 
 import java.util.Collections;
 import java.util.EnumSet;
 import java.util.List;
 
 public class StreamRecyclerAdapter extends RecyclerView.Adapter<StreamItem> implements RecyclerViewClickSupport.OnItemClickListener {
+    private static final String LOGTAG = StringUtils.safeSubstring("Gecko" + StreamRecyclerAdapter.class.getSimpleName(), 0, 23);
+
     private Cursor topSitesCursor;
 
     private HomePager.OnUrlOpenListener onUrlOpenListener;
     private HomePager.OnUrlOpenInBackgroundListener onUrlOpenInBackgroundListener;
 
     private int tiles;
     private int tilesWidth;
     private int tilesHeight;
@@ -116,16 +120,31 @@ public class StreamRecyclerAdapter exten
 
     @Override
     public void onItemClicked(RecyclerView recyclerView, int position, View v) {
         if (getItemViewType(position) != HighlightItem.LAYOUT_ID) {
             // Headers (containing topsites and/or the highlights title) do their own click handling as needed
             return;
         }
 
+        // The position this method receives is from RecyclerView.ViewHolder.getAdapterPosition, whose docs state:
+        // "Note that if you've called notifyDataSetChanged(), until the next layout pass, the return value of this
+        // method will be NO_POSITION."
+        //
+        // At the time of writing, we call notifyDataSetChanged for:
+        // - swapHighlights (can't do anything about this)
+        // - setTileSize (in theory, we might be able to do something hacky to get the existing highlights list)
+        //
+        // Given the low crash rate (34 crashes in 23 days), I don't think it's worth investigating further
+        // or adding a hack.
+        if (position == RecyclerView.NO_POSITION) {
+            Log.w(LOGTAG, "onItemClicked: received NO_POSITION. Returning");
+            return;
+        }
+
         final int actualPosition = translatePositionToCursor(position);
         final Highlight highlight = highlights.get(actualPosition);
 
         ActivityStreamTelemetry.Extras.Builder extras = ActivityStreamTelemetry.Extras.builder()
                 .forHighlightSource(highlight.getSource())
                 .set(ActivityStreamTelemetry.Contract.SOURCE_TYPE, ActivityStreamTelemetry.Contract.TYPE_HIGHLIGHTS)
                 .set(ActivityStreamTelemetry.Contract.ACTION_POSITION, actualPosition)
                 .set(ActivityStreamTelemetry.Contract.COUNT, highlights.size());