1051720 - Concurrency issues in ActivityChooserModel

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Kitching [:ckitching]]

As discussed in Comment 100 of Bug 1039898, ActivityChooserModel has some concurrency issues. I'm pulling these fixes out of the mechanised refactoring bugs: I really don't want to be backing out the generated patch if the concurrency fix goes south.

Comment 1

[Chris Kitching [:ckitching]]

Pasting the pertinent parts of rnewman's comments in the other bug here. Should provide a handy to-do list for issues that're known herein.


Split out TwoWayView for Lucas to give a second review and uplift.

::: mobile/android/base/widget/ActivityChooserModel.java
@@ +297,5 @@
>       * that the very first read succeeds and subsequent reads can be performed
>       * only after a call to {@link #persistHistoricalDataIfNeeded()} followed by change
>       * of the share records.
>       */
> +    /* inner-access */ boolean mCanReadHistoricalData = true;


Comment that this is protected by mInstanceLock.

Fix the one place that it isn't: in PersistHistoryAsyncTask.

@@ +321,5 @@
>  
>      /**
>       * Flag whether to reload the activities for the current intent.
>       */
> +    /* inner-access */ boolean mReloadActivities;


Same: onReceive should take the lock.

@@ +1227,5 @@
>       * Mozilla: Adapted significantly
>       */
>      private static final String LOGTAG = "GeckoActivityChooserModel";
>      private final class DataModelPackageMonitor extends BroadcastReceiver {
> +        /* inner-access */ Context mContext;


volatile.

::: mobile/android/base/widget/ArrowPopup.java
@@ +24,5 @@
>  import android.widget.RelativeLayout;
>  
>  public abstract class ArrowPopup extends PopupWindow {
> +    /* inner-access */ View mAnchor;
> +    /* inner-access */ ImageView mArrow;


Ugh, not thread safe. Leave this, but file.

DoorHangerPopup runnable ... addDoorHanger > ArrowPopup.init
OnSizeChangedListener

@@ +153,5 @@
>          public interface OnSizeChangedListener {
>              public void onSizeChanged();
>          }
>  
> +        /* inner-access */ OnSizeChangedListener mListener;


Should probably be volatile, too.

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Tweak concurrency properties of ActivityChooserModel.

Something approximately this shape seems to make sense.

I am concerned that this seems to be an upstream class... Are we completely wrong in our analysis, making none of this worthwhile?

Comment 3

[Richard Newman [:rnewman]]

It was originally an upstream class. That doesn't mean it's not buggy, and it doesn't prevent us from altering its concurrency properties by adding listeners, distribution support, etc.

Comment 4

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.