Concurrency issues in ActivityChooserModel

ASSIGNED
Assigned to

Status

()

Firefox for Android
General
ASSIGNED
4 years ago
3 years ago

People

(Reporter: ckitching, Assigned: ckitching)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Component: Overlays → General
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8470785 [details] [diff] [review]
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?
Attachment #8470785 - Flags: review?(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.
Attachment #8470785 - Flags: review?(rnewman)
You need to log in before you can comment on or make changes to this bug.