Closed Bug 982172 Opened 6 years ago Closed 6 years ago

Fix race condition with filter stack in panel adapter

Categories

(Firefox for Android :: Awesomescreen, defect)

ARM
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: jdover, Assigned: jdover)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

From bug 975055, comment #38:

Just realized that there's a race here in how the adapter is updated. Very similar to one I fixed in the bookmarks panel before. The filter stack will be updated (pop/push) first (and this might affect the number of items in the adapter immediately) then the cursor is updated once the panel loader returns and delivers the results. There are 2 problems with this:

1. The number of items in the adapter might change once the a filter is pushed/popped but no matching notifyDatasetChanged() is made on the adapter. We'll crash if a layout round happens before the new cursor is delivered.
2. There's a temporary mismatch between the stack state and the current cursor while a new cursor is being loaded.

We'll have to change the way we load and deliver cursor in such a way that the both the stack and the cursor are updated at the same time. See how BookmarksPanel updates the stack. Let's land the core feature first though.
Attached patch patch (obsolete) — Splinter Review
Because the filter stack is not part of the adapter like the bookmarks panel is, I think an approach of simply queuing the push and pop actions until the dataset has loaded is the simplest approach as it does not require the adapter to have any knowledge of how its data is loaded and keeps that responsibility inside of PanelLayout.
Attachment #8389468 - Flags: review?(lucasr.at.mozilla)
Status: NEW → ASSIGNED
Comment on attachment 8389468 [details] [diff] [review]
patch

Review of attachment 8389468 [details] [diff] [review]:
-----------------------------------------------------------------

This patch has a couple of issues:
- The semantics of the stack operations is a bit confusing i.e. popFilter()/pushFilter() don't *really* do what says on the tin anymore.
- The state of the buffered operation is split into different members which is error-prone and likely to cause us some headaches later.

So, I'd really prefer this to follow the same pattern than the bookmarks panel. The fact that the stack is not in the adapter doesn't make this any different because the state of the stack still (indirectly) affects the number of items in the adapter. You can easily keep adapter unchanged while implementing the same approach than bookmarks panel here. Here's what I suggest:

1. Add a DatasetRequest.Type (or simply PanelLayout.RequestType) enum that can be LOAD, POP and PUSH. LOAD being a dataset load that does not change the filter stack (usually the initial dataset load)
2. Add a member to DatasetRequest that holds the type of the request. Add a constructor that doesn't take a 'type' argument and falls back to LOAD.
3. Change pushFilterOnView() to simply request a dataset of type PUSH without changing the stack.
4. Change popFilterOnView() to simply request a dataset of type POP without changing the stack.
5. Change updateViewsWithDataset() to take a DatasetRequest.Type argument as the first argument.
6. For each ViewState in the updateViewsWithDataset() loop, push or pop from the stack according to the request type

I might have missed a detail or two but that would be the idea.
Attachment #8389468 - Flags: review?(lucasr.at.mozilla)
Attached patch patch (obsolete) — Splinter Review
This uses the RequestType approach.
Attachment #8389468 - Attachment is obsolete: true
Attachment #8391454 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8391454 [details] [diff] [review]
patch

Review of attachment 8391454 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/DynamicPanel.java
@@ +217,5 @@
>              final String selection;
>              final String[] selectionArgs;
>  
>              // Null represents the root filter
> +            if (mRequest.filterDetail == null || mRequest.filterDetail.filter == null) {

This is smell for broken encapsulation. It's probably time to hide the DatasetRequest members behind a public API. In other words:

class DatasetRequest:
    private mType
    private mFilterDetail
    private mDatasetId

    public getType()
    public getFilter() // does the null check in mFilterDetail, returns mFilterDetail.filter
    public getDatasetId()

Ideally, do this refactoring in a patch that precedes this one. No big deal if you do it all in a single patch though. Up to you.

::: mobile/android/base/home/PanelLayout.java
@@ +78,5 @@
>          public void setDataset(Cursor cursor);
>          public void setFilterManager(FilterManager manager);
>      }
>  
> +    public enum RequestType { PUSH, POP };

nit: break lines for each element:

public enum RequestType {
    PUSH,
    POP
}

Looking at this now, I'd rather have this enum defined as DatasetRequest.Type instead (as this is only applicable to DatasetRequests anyway).

Also, make this enum Parcelable and handle it the same way we handle other enums in our home code for consistency.

@@ +87,5 @@
>       */
>      public static class DatasetRequest implements Parcelable {
>          public final String datasetId;
> +        public final FilterDetail filterDetail;
> +        public final RequestType type;

nit: move 'type' to be the first declared member.

@@ +96,5 @@
> +            this.type = RequestType.valueOf(RequestType.class, in.readString());
> +        }
> +
> +        public DatasetRequest(String datasetId, FilterDetail filterDetail) {
> +            this(datasetId, filterDetail, RequestType.PUSH);

The default RequestType shouldn't affect the filter at all i.e. the views that are just loading their initial content. Add a DATASET_LOAD type or something to represent this kind of dataset request. Rename PUSH to PUSH_FILTER and POP to POP_FILTER for extra clarity.

@@ +177,5 @@
>       * in response to a dataset request.
>       */
>      public final void deliverDataset(DatasetRequest request, Cursor cursor) {
>          Log.d(LOGTAG, "Delivering request: " + request);
> +        updateViewsWithDataset(request.datasetId, request, cursor);

This is looking a bit smelly now for a couple of reasons:
1. The datasetId is a bit redundant with the request arguments (which also has a datasetId)
2. In updateViewsWithDataset(), datasetId is not consistently used. Sometimes you use the one from the request, sometimes the datasetId argument. This is very bug-prone.

Maybe pass the request type instead of the request itself? Something like:

public void updateViewsWithDataset(String datasetId, DatasetRequest.Type type, Cursor cursor)

@@ +349,2 @@
>  
> +            if (filter == null) {

This is a bit fragile as it assumes that the filter being null means it's the initial dataset load. You don't need this if you implement the RequestType.DATASET_LOAD idea that I mentioned above. The filter stack stuff should not be created if the view never pushes a filter to the stack.
Attachment #8391454 - Flags: review?(lucasr.at.mozilla) → feedback+
Attachment #8392273 - Flags: review?(lucasr.at.mozilla)
Attachment #8391454 - Attachment is obsolete: true
Attachment #8392274 - Flags: review?(lucasr.at.mozilla)
Attachment #8392274 - Attachment is obsolete: true
Attachment #8392274 - Flags: review?(lucasr.at.mozilla)
Attachment #8392276 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8392273 [details] [diff] [review]
01 - Encapsulate DatasetRequest fields

Review of attachment 8392273 [details] [diff] [review]:
-----------------------------------------------------------------

Great.
Attachment #8392273 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8392276 [details] [diff] [review]
02 - Add request type to delay filter stack changes until data loads

Review of attachment 8392276 [details] [diff] [review]:
-----------------------------------------------------------------

This is looking great, just needs a final round of cleanups.

::: mobile/android/base/home/PanelLayout.java
@@ +86,5 @@
>      public static class DatasetRequest implements Parcelable {
> +        public enum Type implements Parcelable {
> +            DATASET_LOAD("dataset_load"),
> +            FILTER_PUSH("filter_push"),
> +            FILTER_POP("filter_pop");

No need to add the 'id' bits to Type. We only use it for enums that have to be saved by HomeConfig i.e. you can remove the constructor, the mId member, and the fromId() and toString() methods.

@@ +253,5 @@
>       * existing panel views.
>       */
>      public final void releaseDataset(String datasetId) {
> +        Log.d(LOGTAG, "Releasing dataset: " + datasetId);
> +        releaseViewsFromDataset(datasetId);

Good call on creating a separate method for releasing datasets.

@@ +329,5 @@
>              mViewStateMap.remove(view);
>          }
>      }
>  
> +    private void updateViewsWithDataset(DatasetRequest request, Cursor cursor) {

Rename to updateViewsFromRequest() for clarity.

@@ +335,5 @@
>              final ViewState detail = entry.getValue();
>  
>              // Update any views associated with the given dataset ID
> +            if (TextUtils.equals(detail.getDatasetId(), request.getDatasetId())) {
> +                if (request != null) {

Request can't really be null here. No need to check it here.

@@ +336,5 @@
>  
>              // Update any views associated with the given dataset ID
> +            if (TextUtils.equals(detail.getDatasetId(), request.getDatasetId())) {
> +                if (request != null) {
> +                    if (request.getType() == DatasetRequest.Type.FILTER_PUSH) {

I'd use a switch here to make this code look a little cleaner:

switch (request.getType()) {
     case FILTER_PUSH:
          ....
     CASE FILTER_POP:
          ....
}

@@ +349,5 @@
>              }
>          }
>      }
>  
> +    private void releaseViewsFromDataset(String datasetId) {

Rename to releaseViewsWithDataset()? That's a tricky one to name :-)

@@ -336,5 @@
>              if (mFilterStack == null) {
>                  mFilterStack = new LinkedList<FilterDetail>();
> -
> -                // Initialize with a null filter.
> -                // TODO: use initial filter from ViewConfig

I'd keep this comment. We will still need to do it.

@@ +507,3 @@
>  
> +            final String filter = toLoad.filter;
> +            final String title = toLoad.title;

Maybe do this to make the code a little nicer to read:

filter FilterDetail filter = new FilterDetail(toLoad.filter, toLoad.title);
final String datasetId = viewState.getDatasetId();

mDatasetHandler.requestDataset(
        new DatasetRequest(DatasetRequest.Type.FILTER_POP, datasetId, filter));
Attachment #8392276 - Flags: review?(lucasr.at.mozilla) → feedback+
Attachment #8392276 - Attachment is obsolete: true
Attachment #8392321 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8392321 [details] [diff] [review]
02 - Add request type to delay filter stack changes until data loads

Review of attachment 8392321 [details] [diff] [review]:
-----------------------------------------------------------------

Awesome. Giving r+, please upload an updated patch.

::: mobile/android/base/home/PanelLayout.java
@@ +116,2 @@
>          private final String datasetId;
> +        private final FilterDetail filterDetail;

Missed this in my original review: these should actually have the old 'm' prefix to be consistent with the rest of this file. So, mType, mDatasetId, mFilterDetail.

@@ +479,5 @@
>       * @return whether the filter has changed
>       */
>      private boolean popFilterOnView(ViewState viewState) {
> +        if (viewState.canPopFilter()) {
> +            final FilterDetail filter = viewState.getPreviousFilter();

filter -> filterDetail for clarity?
Attachment #8392321 - Flags: review?(lucasr.at.mozilla) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c20c80c50ae
https://hg.mozilla.org/mozilla-central/rev/47a31c965c9c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Attached patch patch for auroraSplinter Review
Note to release managers: I'm requesting uplift for a series of Firefox Hub bugs. The main fixes we need are in bugs at the end of the series, but trying to rebase those patches proved difficult and risky, so I think we should just uplift the dependecies.

Note to sheriffs: I have a local patch series that I can land on aurora when these bugs all get approval.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): dependency for initial Firefox Hub release (promoted feed add-ons in fx30)
User impact if declined: panels won't update when data changes
Testing completed (on m-c, etc.): baked on m-c
Risk to taking this patch (and alternatives if risky): only affects dynamic panels, need an add-on to trigger this
String or IDL/UUID changes made by this patch: none
Attachment #8412219 - Flags: approval-mozilla-aurora?
Attachment #8412219 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.