Skip to content

[UIKit] UIGestureRecognizer's custon OnDispose method needs to queue the actual operation on the main thread - #5402

Merged
migueldeicaza merged 1 commit into
dotnet:masterfrom
migueldeicaza:fix-uigesturerecognizer-regression
Jan 15, 2019
Merged

[UIKit] UIGestureRecognizer's custon OnDispose method needs to queue the actual operation on the main thread#5402
migueldeicaza merged 1 commit into
dotnet:masterfrom
migueldeicaza:fix-uigesturerecognizer-regression

Conversation

@migueldeicaza

Copy link
Copy Markdown
Contributor

Proposed fixed for:

#5388

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling RemoveTarget one the main thread (delayed) fix the reported issue.

Another way (I was trying) would be to remove the thread-check on RemoveTarget since it seem (at least Apple Main Thread Checker allows it) to be a thread safe call.

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Build succeeded
API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 80 tests passed.

Failed tests

  • monotouch-test/iOS Unified 32-bits - simulator/Debug (static registrar): Failed

@migueldeicaza

Copy link
Copy Markdown
Contributor Author

The problem is that by the time it hits the main thread, the handle will have been cleared.

The Dispose() implementation just injects the result of the Dispose attribute, before calling Dispose(disposing). So there would be a potential race where the finalizer queue would run first, and release the handle, so our handle would be pointing to garbage.

That is why I took the ref, and then released it manually.

@rolfbjarne

Copy link
Copy Markdown
Member

Do we really need to call RemoveTarget when Dispose is called from the GC? Won't things be freed anyway when the object is freed on the native side?

@migueldeicaza

Copy link
Copy Markdown
Contributor Author

The hook is executed for all Dispose() kinds of invocations. I do not think it harms to keep it.

@migueldeicaza
migueldeicaza merged commit a2c80ed into dotnet:master Jan 15, 2019
chamons pushed a commit to chamons/xamarin-macios that referenced this pull request Jan 15, 2019
…izer-regression

[UIKit] UIGestureRecognizer's custon OnDispose method needs to queue the actual operation on the main thread
@VincentDondain

Copy link
Copy Markdown
Contributor

I think this PR broke this test:

GestureRecognizerTest
	[FAIL] GestureRecognizerTest.NoStrongCycles :   Any finalized
  Expected: True
  But was:  False

		  at MonoTouchFixtures.UIKit.GestureRecognizerTest.NoStrongCycles () [0x001ce] in /Users/builder/jenkins/workspace/xamarin-macios-pr-builder/tests/monotouch-test/UIKit/GestureRecognizerTest.cs:81 
		  at (wrapper managed-to-native) System.Reflection.MonoMethod.InternalInvoke(System.Reflection.MonoMethod,object,object[],System.Exception&)
		  at System.Reflection.MonoMethod.Invoke (System.Object obj, System.Reflection.BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x0003b] in /Library/Frameworks/Xamarin.iOS.framework/Versions/12.7.0.62/src/Xamarin.iOS/mcs/class/corlib/System.Reflection/MonoMethod.cs:305 

chamons added a commit that referenced this pull request Jan 30, 2019
…egression (#5414)

[UIKit] UIGestureRecognizer's custon OnDispose method needs to queue the actual operation on the main thread
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants