Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

[Android] Prevent ObjectDisposedException in ButtonBackgroundTracker #1040

Closed
wants to merge 1 commit into from
Closed

Conversation

velocitysystems
Copy link
Contributor

Description of Change

Fixes an ObjectDisposedException thrown in the ButtonDrawable.Draw method. The drawable itself is held by the new class ButtonBackgroundTracker introduced in the refactoring PR #941 in XF 2.3.5.255-pre5.

Inside the UpdateDrawableMethod(), the field _backgroundDrawable is used to hold a reference to the drawable. Previously, we only recreate the drawable if it was null however this does not account for edge cases where the object has been disposed but is not null.

To ensure we have a valid instance of ButtonDrawable, we initialise a new instance.
Note: I experimented with simply checking the handle for IntPtr.Zero at this point, but this is not 100% effective. It appears there are certain cases whereby the object can be disposed between _nativeButton.SetBackground(_backgroundDrawable); and the native `Draw()' method being invoked.

This is particular noticeable when using advanced layout mechanisms such as Android's RecyclerView.

Bugs Fixed

https://bugzilla.xamarin.com/show_bug.cgi?id=57789

API Changes

None

Behavioral Changes

None

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense

@dnfclas
Copy link

dnfclas commented Jul 7, 2017

@mattrichnz,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by .NET Foundation. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@hartez
Copy link
Contributor

hartez commented Jul 24, 2017

@mattrichnz I believe that PR #1033 fixes this problem - if not, please re-open this PR and we'll take another look at it.

@hartez hartez closed this Jul 24, 2017
@velocitysystems
Copy link
Contributor Author

Thanks @hartez, we have tested #1033 but this does not resolve this particular issue.

As mentioned, there appear to be edge cases where "_backgroundDrawable" is getting disposed but is not null so throws an ObjectDisposedException when it is accessed. Prior to #941, this bug did not occur.

We tested a number of different solutions and without rewriting the entire implementation found that ensuring the drawable is recreated each time resolved the issue. Please feel free to improve on this however.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants