Bug 1367024: Fix array index out of bounds in StreamRecyclerAdapter. r=liuche draft
authorMichael Comella <michael.l.comella@gmail.com>
Wed, 19 Jul 2017 16:35:44 -0700
changeset 611639 576e29298f131870fed987793663de499285ab1d
parent 611409 4b32e7ce740eaf6434180bc9e44731dab0aa67cc
child 638213 758b703ba2bf7d99ef5058d99462c308ff0a1097
push id69278
push usermichael.l.comella@gmail.com
push dateWed, 19 Jul 2017 23:37:20 +0000
reviewersliuche
bugs1367024
milestone56.0a1
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());