New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Dispose check before setting properties on Button #1013

Merged
merged 1 commit into from Jun 26, 2017

Conversation

Projects
None yet
6 participants
@samhouts
Member

samhouts commented Jun 24, 2017

Description of Change

Clicking a button quickly to Navigate pages can cause a crash because we weren't always checking to see if the control was disposed. Now we're checking all over the place.

Bugs Fixed

  • None

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
void UpdateBitmap()
{
if (Element == null)
if (Element == null || _isDisposed)

This comment has been minimized.

@StephaneDelcroix

StephaneDelcroix Jun 24, 2017

Member

can't we do the check at a single place, in OnPropertyChanged ? and do it on the base class, so it applies to all controls ?

@StephaneDelcroix

StephaneDelcroix Jun 24, 2017

Member

can't we do the check at a single place, in OnPropertyChanged ? and do it on the base class, so it applies to all controls ?

This comment has been minimized.

@samhouts

samhouts Jun 26, 2017

Member

These methods are called all over this renderer, not just in OnPropertyChanged. The original PR that "fixed" this issue added it in just OnElementChanged. This seemed like the best way to prevent regressing this bug, and since it's a crasher, I wanted to be safe.

@samhouts

samhouts Jun 26, 2017

Member

These methods are called all over this renderer, not just in OnPropertyChanged. The original PR that "fixed" this issue added it in just OnElementChanged. This seemed like the best way to prevent regressing this bug, and since it's a crasher, I wanted to be safe.

This comment has been minimized.

@StephaneDelcroix
@StephaneDelcroix

@rmarinho rmarinho merged commit 39dee7a into master Jun 26, 2017

5 checks passed

Android-UITests-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passe…
Details
Android-UITests-Stable-LegacyRenderers Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 (Legacy Rende…
Details
OSX-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Debug : Tests passed: 3909, ignored: 10
Details
Windows-Release-Stable Finished TeamCity Build Xamarin.Forms :: Debug :: Stable - Cycle 9 :: Windows Release Unit Tests : Tests passed: 3909, ignored: 10
Details

@rmarinho rmarinho deleted the fastrendererbuttoncrash branch Jun 26, 2017

rmarinho added a commit that referenced this pull request Jun 26, 2017

@@ -450,7 +460,7 @@ void UpdateTextColor()
void UpdateDrawable()
{
_backgroundTracker.UpdateDrawable();
_backgroundTracker?.UpdateDrawable();

This comment has been minimized.

@mattrichnz

mattrichnz Jun 27, 2017

Contributor

Just wondering why there is no disposed check here?

@mattrichnz

mattrichnz Jun 27, 2017

Contributor

Just wondering why there is no disposed check here?

@VitalyKnyazev

This comment has been minimized.

Show comment
Hide comment
@VitalyKnyazev

VitalyKnyazev Jun 28, 2017

Contributor

Were latest checks included into 2.3.5.255-pre5?

Still crashing with "CANNOT ACCESS A DISPOSED OBJECT. OBJECT NAME: 'XAMARIN.FORMS.PLATFORM.ANDROID.FASTRENDERERS.BUTTONRENDERER'" from within IVisualElementRenderer.GetDesiredSize.

Full call stack:

  • JniPeerMembers.AssertSelf (Java.Interop.IJavaPeerable self)
  • JniPeerMembers+JniInstanceMethods.InvokeNonvirtualVoidMethod (System.String encodedMember, Java.Interop.IJavaPeerable self, Java.Interop.JniArgumentValue* parameters)
  • View.Measure (System.Int32 widthMeasureSpec, System.Int32 heightMeasureSpec)
  • IVisualElementRenderer.GetDesiredSize (System.Int32 widthConstraint, System.Int32 heightConstraint)
  • IPlatform.GetNativeSize (Xamarin.Forms.VisualElement view, System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.OnSizeRequest (System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.OnMeasure (System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.GetSizeRequest (System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.Measure (System.Double widthConstraint, System.Double heightConstraint, Xamarin.Forms.MeasureFlags flags)
  • Grid.CalculateAutoCells (System.Double width, System.Double height)
  • Grid.MeasureGrid (System.Double width, System.Double height, System.Boolean requestSize)
  • Grid.LayoutChildren (System.Double x, System.Double y, System.Double width, System.Double height)
  • Layout.UpdateChildrenLayout ()
  • Layout.OnSizeAllocated (System.Double width, System.Double height)
  • Layout+<>c.b__39_0 ()
  • Thread+RunnableImplementor.Run ()
Contributor

VitalyKnyazev commented Jun 28, 2017

Were latest checks included into 2.3.5.255-pre5?

Still crashing with "CANNOT ACCESS A DISPOSED OBJECT. OBJECT NAME: 'XAMARIN.FORMS.PLATFORM.ANDROID.FASTRENDERERS.BUTTONRENDERER'" from within IVisualElementRenderer.GetDesiredSize.

Full call stack:

  • JniPeerMembers.AssertSelf (Java.Interop.IJavaPeerable self)
  • JniPeerMembers+JniInstanceMethods.InvokeNonvirtualVoidMethod (System.String encodedMember, Java.Interop.IJavaPeerable self, Java.Interop.JniArgumentValue* parameters)
  • View.Measure (System.Int32 widthMeasureSpec, System.Int32 heightMeasureSpec)
  • IVisualElementRenderer.GetDesiredSize (System.Int32 widthConstraint, System.Int32 heightConstraint)
  • IPlatform.GetNativeSize (Xamarin.Forms.VisualElement view, System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.OnSizeRequest (System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.OnMeasure (System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.GetSizeRequest (System.Double widthConstraint, System.Double heightConstraint)
  • VisualElement.Measure (System.Double widthConstraint, System.Double heightConstraint, Xamarin.Forms.MeasureFlags flags)
  • Grid.CalculateAutoCells (System.Double width, System.Double height)
  • Grid.MeasureGrid (System.Double width, System.Double height, System.Boolean requestSize)
  • Grid.LayoutChildren (System.Double x, System.Double y, System.Double width, System.Double height)
  • Layout.UpdateChildrenLayout ()
  • Layout.OnSizeAllocated (System.Double width, System.Double height)
  • Layout+<>c.b__39_0 ()
  • Thread+RunnableImplementor.Run ()

assemhakmeh added a commit to assemhakmeh/Xamarin.Forms that referenced this pull request Jul 28, 2017

Merge branch '2.3.5' into ios-fastrenderers
* 2.3.5:
  [UWP] Fixes for usage of XF with .net native toolchain (#1024)
  [UWP] Make sure to update HitTestVisible when IsEnable changes (#1015)
  [Android] Dispose check before setting properties on Button (#1013)
  Add missing member variable to FormsApplicationActivity
  Fix NRE when background color of button set in FormsApplicationActivity (#1010)
  Fix border on android buttons  (#941)
  [iOS] ListView with UnevenRows and Cell Heights will no longer be slow to load (#994)
  Set the Id field for Android Views created by Forms #1004
  Fix build
  Fix possible crash on API 21+ at launch when using Holo theme and FormsApplicationActivity (#961)
  [Android] Remove the ". " on empty labels (Accessibility) on Fastrenderers (#915)
  Remove debug outputs (#1008)
  Add check for instance of UITableView (#885)
  [XamlC] fix release builds of Xaml Unit Tests
  Dispose check on ButtonRenderer (#975)
  [previewer] make sure we do not crash even if the previewer doesn't s… (#946)
  [XamlC] fix build
  Remove VisualElement finalizer (#918)
  [XamlC] process symbols if DebugType is set (#925)

@samhouts samhouts added D-15.4 and removed cla-not-required labels Oct 10, 2017

@samhouts samhouts modified the milestones: 2.3.0, 2.3.5 Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment