Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 201c629f authored by Victor Chang's avatar Victor Chang
Browse files

Fix crash in time zone picker due to race condition on view updates 

- Can't reproduce the race condition with manual test, probably the view
  updates are fast enough that only monkey test can reproduce the issue.
- Reproduced a similar stacktrace and IndexOutOfBoundsException with
  Robolectric test by assuming that the race condition happens after
  text filtering and view updates. Try to fix the bug with this assumption
- The fix is to bind the data (data position in adapter) with ViewHolder.

Bug: 75322108
Test: m RunSettingsRoboTests ROBOTEST_FILTER=com.android.settings.datetime.timezone
Change-Id: Ie5d932bce30590b8067e042c3380911c9608872f
parent d7ea524e

src/com/android/settings/datetime/timezone/BaseTimeZoneAdapter.java

+22 −22
Original line number Diff line number Diff line
@@ -18,6 +18,7 @@ package com.android.settings.datetime.timezone; 


import android.icu.text.BreakIterator;

import android.support.annotation.NonNull;

import android.support.annotation.VisibleForTesting;

import android.support.annotation.WorkerThread;

import android.support.v7.widget.RecyclerView;

import android.text.TextUtils;
@@ -42,14 +43,14 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 
        extends RecyclerView.Adapter<BaseTimeZoneAdapter.ItemViewHolder> {



    private final List<T> mOriginalItems;

    private final OnListItemClickListener mOnListItemClickListener;

    private final OnListItemClickListener<T> mOnListItemClickListener;

    private final Locale mLocale;

    private final boolean mShowItemSummary;



    private List<T> mItems;

    private ArrayFilter mFilter;



    public BaseTimeZoneAdapter(List<T> items, OnListItemClickListener

    public BaseTimeZoneAdapter(List<T> items, OnListItemClickListener<T>

            onListItemClickListener, Locale locale, boolean showItemSummary) {

        mOriginalItems = items;

        mItems = items;
@@ -69,14 +70,8 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 


    @Override

    public void onBindViewHolder(@NonNull ItemViewHolder holder, int position) {

        final AdapterItem item = mItems.get(position);

        holder.mSummaryFrame.setVisibility(

                mShowItemSummary ? View.VISIBLE : View.GONE);

        holder.mTitleView.setText(item.getTitle());

        holder.mIconTextView.setText(item.getIconText());

        holder.mSummaryView.setText(item.getSummary());

        holder.mTimeView.setText(item.getCurrentTime());

        holder.setPosition(position);

        holder.setAdapterItem(mItems.get(position));

        holder.mSummaryFrame.setVisibility(mShowItemSummary ? View.VISIBLE : View.GONE);

    }



    @Override
@@ -89,8 +84,7 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 
        return mItems.size();

    }



    public  @NonNull

    Filter getFilter() {

    public @NonNull ArrayFilter getFilter() {

        if (mFilter == null) {

            mFilter = new ArrayFilter();

        }
@@ -110,18 +104,18 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 
        String[] getSearchKeys();

    }



    public static class ItemViewHolder extends RecyclerView.ViewHolder

            implements View.OnClickListener{

    public static class ItemViewHolder<T extends BaseTimeZoneAdapter.AdapterItem>

            extends RecyclerView.ViewHolder implements View.OnClickListener {



        final OnListItemClickListener mOnListItemClickListener;

        final OnListItemClickListener<T> mOnListItemClickListener;

        final View mSummaryFrame;

        final TextView mTitleView;

        final TextView mIconTextView;

        final TextView mSummaryView;

        final TextView mTimeView;

        private int mPosition;

        private T mItem;



        public ItemViewHolder(View itemView, OnListItemClickListener onListItemClickListener) {

        public ItemViewHolder(View itemView, OnListItemClickListener<T> onListItemClickListener) {

            super(itemView);

            itemView.setOnClickListener(this);

            mSummaryFrame = itemView.findViewById(R.id.summary_frame);
@@ -132,13 +126,17 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 
            mOnListItemClickListener = onListItemClickListener;

        }



        public void setPosition(int position) {

            mPosition = position;

        public void setAdapterItem(T item) {

            mItem = item;

            mTitleView.setText(item.getTitle());

            mIconTextView.setText(item.getIconText());

            mSummaryView.setText(item.getSummary());

            mTimeView.setText(item.getCurrentTime());

        }



        @Override

        public void onClick(View v) {

            mOnListItemClickListener.onListItemClick(mPosition);

            mOnListItemClickListener.onListItemClick(mItem);

        }

    }
@@ -151,7 +149,8 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 
     * require additional pre-processing. Potentially, a trie structure can be used to match

     * prefixes of the search keys.

     */

    private class ArrayFilter extends Filter {

    @VisibleForTesting

    public class ArrayFilter extends Filter {



        private BreakIterator mBreakIterator = BreakIterator.getWordInstance(mLocale);
@@ -197,8 +196,9 @@ public class BaseTimeZoneAdapter<T extends BaseTimeZoneAdapter.AdapterItem> 
            return results;

        }



        @VisibleForTesting

        @Override

        protected void publishResults(CharSequence constraint, FilterResults results) {

        public void publishResults(CharSequence constraint, FilterResults results) {

            mItems = (List<T>) results.values;

            notifyDataSetChanged();

        }

src/com/android/settings/datetime/timezone/BaseTimeZoneInfoPicker.java

+3 −4
Original line number Diff line number Diff line
@@ -31,7 +31,6 @@ import java.util.ArrayList; 
import java.util.Date;

import java.util.List;

import java.util.Locale;

import java.util.Objects;



/**

 * Render a list of {@class TimeZoneInfo} into the list view in {@class BaseTimeZonePicker}
@@ -52,8 +51,8 @@ public abstract class BaseTimeZoneInfoPicker extends BaseTimeZonePicker { 
        return mAdapter;

    }



    private void onListItemClick(int position) {

        final TimeZoneInfo timeZoneInfo = mAdapter.getItem(position).mTimeZoneInfo;

    private void onListItemClick(TimeZoneInfoItem item) {

        final TimeZoneInfo timeZoneInfo = item.mTimeZoneInfo;

        getActivity().setResult(Activity.RESULT_OK, prepareResultData(timeZoneInfo));

        getActivity().finish();

    }
@@ -67,7 +66,7 @@ public abstract class BaseTimeZoneInfoPicker extends BaseTimeZonePicker { 
    protected static class ZoneAdapter extends BaseTimeZoneAdapter<TimeZoneInfoItem> {



        public ZoneAdapter(Context context, List<TimeZoneInfo> timeZones,

                OnListItemClickListener onListItemClickListener, Locale locale) {

                OnListItemClickListener<TimeZoneInfoItem> onListItemClickListener, Locale locale) {

            super(createTimeZoneInfoItems(context, timeZones, locale),

                    onListItemClickListener, locale,  true /* showItemSummary */);

        }

src/com/android/settings/datetime/timezone/BaseTimeZonePicker.java

+3 −2
Original line number Diff line number Diff line
@@ -17,6 +17,7 @@ 
package com.android.settings.datetime.timezone;



import android.os.Bundle;

import android.support.annotation.VisibleForTesting;

import android.support.v7.widget.LinearLayoutManager;

import android.support.v7.widget.RecyclerView;

import android.view.LayoutInflater;
@@ -161,8 +162,8 @@ public abstract class BaseTimeZonePicker extends InstrumentedFragment 
        return false;

    }



    public interface OnListItemClickListener {

        void onListItemClick(int position);

    public interface OnListItemClickListener<T extends BaseTimeZoneAdapter.AdapterItem> {

        void onListItemClick(T item);

    }



}

src/com/android/settings/datetime/timezone/RegionSearchPicker.java

+6 −4
Original line number Diff line number Diff line
@@ -18,15 +18,16 @@ package com.android.settings.datetime.timezone; 


import android.app.Activity;

import android.content.Intent;

import android.graphics.Paint;

import android.icu.text.Collator;

import android.icu.text.LocaleDisplayNames;

import android.os.Bundle;

import android.support.annotation.VisibleForTesting;

import android.util.Log;



import com.android.internal.logging.nano.MetricsProto;

import com.android.settings.R;

import com.android.settings.core.SubSettingLauncher;

import com.android.settings.datetime.timezone.BaseTimeZoneAdapter.AdapterItem;

import com.android.settings.datetime.timezone.model.FilteredCountryTimeZones;

import com.android.settings.datetime.timezone.model.TimeZoneData;
@@ -63,8 +64,8 @@ public class RegionSearchPicker extends BaseTimeZonePicker { 
        return mAdapter;

    }



    private void onListItemClick(int position) {

        final String regionId = mAdapter.getItem(position).getId();

    private void onListItemClick(RegionItem item) {

        final String regionId = item.getId();

        final FilteredCountryTimeZones countryTimeZones = mTimeZoneData.lookupCountryTimeZones(

                regionId);

        final Activity activity = getActivity();
@@ -119,7 +120,8 @@ public class RegionSearchPicker extends BaseTimeZonePicker { 
        return new ArrayList<>(items);

    }



    private static class RegionItem implements BaseTimeZoneAdapter.AdapterItem {

    @VisibleForTesting

    static class RegionItem implements AdapterItem {



        private final String mId;

        private final String mName;

tests/robotests/src/com/android/settings/datetime/timezone/RegionSearchPickerTest.java

+104 −11
Original line number Diff line number Diff line
@@ -16,7 +16,20 @@ 


package com.android.settings.datetime.timezone;



import static com.google.common.truth.Truth.assertThat;



import static org.junit.Assert.assertEquals;

import static org.mockito.Mockito.mock;

import static org.mockito.Mockito.when;



import android.app.Activity;

import android.app.Fragment;

import android.widget.Filter;

import android.widget.LinearLayout;



import com.android.settings.datetime.timezone.BaseTimeZoneAdapter.AdapterItem;

import com.android.settings.datetime.timezone.BaseTimeZoneAdapter.ItemViewHolder;

import com.android.settings.datetime.timezone.RegionSearchPicker.RegionItem;

import com.android.settings.datetime.timezone.model.TimeZoneData;

import com.android.settings.testutils.SettingsRobolectricTestRunner;
@@ -24,17 +37,23 @@ import libcore.util.CountryZonesFinder; 


import org.junit.Test;

import org.junit.runner.RunWith;

import org.robolectric.Robolectric;

import org.robolectric.RuntimeEnvironment;

import org.robolectric.annotation.Config;

import org.robolectric.annotation.Implementation;

import org.robolectric.annotation.Implements;



import java.util.ArrayList;

import java.util.Arrays;

import java.util.List;

import java.util.Locale;



import static com.google.common.truth.Truth.assertThat;

import static org.junit.Assert.assertEquals;

import static org.mockito.Mockito.mock;

import static org.mockito.Mockito.when;



@RunWith(SettingsRobolectricTestRunner.class)

@Config(shadows = {

        RegionSearchPickerTest.ShadowBaseTimeZonePicker.class,

        RegionSearchPickerTest.ShadowFragment.class,

    }

)

public class RegionSearchPickerTest {



    @Test
@@ -44,16 +63,90 @@ public class RegionSearchPickerTest { 
        CountryZonesFinder finder = mock(CountryZonesFinder.class);

        when(finder.lookupAllCountryIsoCodes()).thenReturn(regionList);



        RegionSearchPicker picker = new RegionSearchPicker() {

            @Override

            protected Locale getLocale() {

                return Locale.US;

            }

        };

        RegionSearchPicker picker = new RegionSearchPicker();

        BaseTimeZoneAdapter adapter = picker.createAdapter(new TimeZoneData(finder));

        assertEquals(1, adapter.getItemCount());

        AdapterItem item = adapter.getItem(0);

        assertEquals("United States", item.getTitle().toString());

        assertThat(Arrays.asList(item.getSearchKeys())).contains("United States");

    }



    // Test RegionSearchPicker does not crash due to the wrong assumption that no view is clicked

    // before all views are updated and after internal data structure is updated for text filtering.

    // This test mocks the text filtering event and emit click event immediately

    // http://b/75322108

    @Test

    public void clickItemView_duringRegionSearch_shouldNotCrash() {

        List regionList = new ArrayList();

        regionList.add("US");

        CountryZonesFinder finder = mock(CountryZonesFinder.class);

        when(finder.lookupAllCountryIsoCodes()).thenReturn(regionList);



        // Prepare the picker and adapter

        RegionSearchPicker picker = new RegionSearchPicker();

        BaseTimeZoneAdapter<RegionItem> adapter = picker.createAdapter(new TimeZoneData(finder));

        // Prepare and bind a new ItemViewHolder with United States

        ItemViewHolder viewHolder = adapter.onCreateViewHolder(

                new LinearLayout(RuntimeEnvironment.application), 0);

        adapter.onBindViewHolder(viewHolder, 0);

        assertEquals(1, adapter.getItemCount());



        // Pretend to search for a unknown region and no result is found.

        FilterWrapper filterWrapper = new FilterWrapper(adapter.getFilter());

        filterWrapper.publishEmptyResult("Unknown region 1");



        // Assert that the adapter should have been updated with no item

        assertEquals(0, adapter.getItemCount());

        viewHolder.itemView.performClick(); // This should not crash

    }



    // FilterResults is a protected inner class. Use FilterWrapper to create an empty FilterResults

    // instance.

    private static class FilterWrapper extends Filter {



        private final BaseTimeZoneAdapter.ArrayFilter mFilter;



        private FilterWrapper(BaseTimeZoneAdapter.ArrayFilter filter) {

            mFilter = filter;

        }



        @Override

        protected FilterResults performFiltering(CharSequence charSequence) {

            return null;

        }



        private void publishEmptyResult(CharSequence charSequence) {

            FilterResults filterResults = new FilterResults();

            filterResults.count = 0;

            filterResults.values = new ArrayList<>();

            publishResults(charSequence, filterResults);

        }



        @Override

        protected void publishResults(CharSequence charSequence, FilterResults filterResults) {

            mFilter.publishResults(charSequence, filterResults);

        }

    }



    // Robolectric can't start android.app.Fragment with support library v4 resources. Pretend

    // the fragment has started, and provide the objects in context here.

    @Implements(BaseTimeZonePicker.class)

    public static class ShadowBaseTimeZonePicker extends ShadowFragment {



        @Implementation

        protected Locale getLocale() {

            return Locale.US;

        }

    }



    @Implements(Fragment.class)

    public static class ShadowFragment {



        private Activity mActivity = Robolectric.setupActivity(Activity.class);



        @Implementation

        public final Activity getActivity() {

            return mActivity;

        }

    }

}