Crash in OnRebindCallback

[wongcain]

Issue

We are getting intermittent calls from the OnRebindCallback.onCancelled(...) implementation on this line: https://github.com/evant/binding-collection-adapter/blob/33c7a1740df565979bf6f72582272bc1d79937d7/bindingcollectionadapter-recyclerview/src/main/java/me/tatarka/bindingcollectionadapter2/BindingRecyclerViewAdapter.java#L160

The error is "java.lang.IllegalStateException: Cannot call this method while RecyclerView is computing a layout or scrolling"

I think either a check against getScrollState() is required, or the notifyItemChanged(position, DATA_INVALIDATION) call needs to be wrapped in a handler.post(...)

Unfortunately, since onCreateViewHolder is final, I'm unable to test my theories by a simple override of this method. However, I have found this which supports the theory that posting on the handler will help: https://stackoverflow.com/questions/43221847/cannot-call-this-method-while-recyclerview-is-computing-a-layout-or-scrolling-wh?noredirect=1&lq=1

[evant]

blarg, so the recyclerView.isComputingLayout() check should already account for this. What version of recyclerview are you using?

[wongcain]

AndroidX 1.0.0

[wongcain]

I see Google has addressed some bugs in later versions regarding scroll-state like this one: https://android-review.googlesource.com/c/platform/frameworks/support/+/812576/ Do you think that is related?

[evant]

I don't think that's related, it's checking the same isComputingLayout() internally so even if there was a bug around that not getting unset it should still skip the notify. Another idea, can you check that onCanceled() is always being triggered on the main thread?

[wongcain]

I'm not sure how I'd go about checking that. It's called by the Data Binding framework, no?

[wongcain]

I'm thinking it might be worth a try to upgrade to RecyclerView 1.1.0-beta just to see if it helps. My thought is that if onPreBind() and onCanceled() are not called in the same UI Loop, then a bug in setting the scroll state could cause isComputingLayout() to evaluate incorrectly/inconsistently i.e. return false when it should return true

[wongcain]

Actually... looking at the ViewDataBinding.java code, it looks like onPreBind() and onCanceled() are called back to back within a single executePendingBindings() call (if I am following the code correctly). That that seems to all happen on the UI thread. 🤔

[wongcain]

Hi Evan. I have some more information for you.

Luckily someone on our QA team seems to be able to replicate the crash fairly reliably. I wrapped notifyItemChanged(position, DATA_INVALIDATION); in a try/catch block.

Doing this prevents the crash and I don't see any negative side-effects.

For caught exceptions I added logging to see the following:

• Are we on the UI Thread? (via Looper.myLooper() == Looper.getMainLooper())
• recyclerView.isComputingLayout()
• recyclerView.getScrollState()

In every case, there is no indication that there is a problem. I.e. we are in the UI thread, isComputingLayout() is false, and getScrollState() is 0 (IDLE).

So I suspect that there is a yet to be fixed bug on Google's side. In the meantime, would you accept a pull request that just adds a noop try/catch exception handler?

[evant]

Huh, that's interesting, have you filed a bug yet? Looking through the recyclerview source code I didn't see a way to trigger this but might be missing something. I would accept a pr, it should be safe as it's supposed to bail out there anyway.

[evant]

For caught exceptions I added logging to see the following

just curious, did you add logging for when it didn't trigger an exception? If it got called on a background thread it could trigger an exception in the main thread.

[wongcain]

How do you mean?

This is what I did:

            @Override
            public void onCanceled(ViewDataBinding binding) {
                RecyclerView rv = recyclerView;
                if (rv == null || rv.isComputingLayout()) {
                    return;
                }
                int position = holder.getAdapterPosition();
                if (position != RecyclerView.NO_POSITION) {
                    try {
                        notifyItemChanged(position, DATA_INVALIDATION);
                    } catch (IllegalStateException e) {
                        Map<String, Object> props = new HashMap<>();
                        props.put("isUiThread", Looper.myLooper() == Looper.getMainLooper());
                        props.put("isComputingLayout", rv.isComputingLayout());
                        props.put("scrollState", rv.getScrollState());
                        DevLogs.log("BindingRecyclerViewAdapterCrash", props);
                    }
                }
            }

I don't see how this can jump threads. Also, looking at the RecyclerView and DataBinding code, I can't see a path that onCanceled() could be called on anything but the UiThread.

[evant]

I mean like

            @Override
            public void onCanceled(ViewDataBinding binding) {
                 RecyclerView rv = recyclerView;

                 Map<String, Object> props = new HashMap<>();
                 props.put("isUiThread", Looper.myLooper() == Looper.getMainLooper());
                 props.put("isComputingLayout", rv.isComputingLayout());
                 props.put("scrollState", rv.getScrollState());
                 DevLogs.log("BindingRecyclerViewAdapterCrash", props);

                if (rv == null || rv.isComputingLayout()) {
                    return;
                }
                int position = holder.getAdapterPosition();
                if (position != RecyclerView.NO_POSITION) {
                    try {
                        notifyItemChanged(position, DATA_INVALIDATION);
                    } catch (IllegalStateException e) {
                    }
                }
            }

so you can see all calls not just the failing one. Just still trying to rule out threading issues, I might be wrong.

[wongcain]

I see. I can give it a try.

[wongcain]

It's always on the UI thread. ¯_(ツ)_/¯

I'll put up the PR.