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

Fix weak references when getting targets #558

Merged
merged 1 commit into from Nov 24, 2016

Conversation

Projects
None yet
5 participants
@adrianknight89
Contributor

adrianknight89 commented Nov 24, 2016

Description of Change

When using WeakReference, one should not rely on IsAlive when it returns true as it could mean one of two things and we don't know which one. For more info, see bug description as well as the links in it.

It appears that XF is using a mix of WeakReference (.NET 1.1) and WeakReference<T> (4.5). For future references, WeakReference should probably be banned from being used.

Bugs Fixed

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
dataSourceProviderer?.UnmaskKey(_path);
}
var dataSourceProviderer = (IDataSourceProvider)_dataSourceRef?.Target;
dataSourceProviderer?.UnmaskKey(_path);

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 24, 2016

Contributor

Checking IsAlive and Target at the same time is overkill.

@adrianknight89

adrianknight89 Nov 24, 2016

Contributor

Checking IsAlive and Target at the same time is overkill.

((HubTile)weakRef.Target).Size = GetSize();
var hTile = (HubTile)weakRef.Target;
if (hTile != null)
hTile.Size = GetSize();

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 24, 2016

Contributor

Admittedly, this is WP8, but the usage was incorrect.

@adrianknight89

adrianknight89 Nov 24, 2016

Contributor

Admittedly, this is WP8, but the usage was incorrect.

@StephaneDelcroix

This comment has been minimized.

Show comment
Hide comment
@StephaneDelcroix

StephaneDelcroix Nov 24, 2016

Member

Hey @adrianknight89. This is a good one. there was no need to open a bug report for it.

Member

StephaneDelcroix commented Nov 24, 2016

Hey @adrianknight89. This is a good one. there was no need to open a bug report for it.

@@ -91,7 +91,7 @@ static void InnerSend(string message, Type senderType, Type argType, object send
List<Tuple<WeakReference, Action<object, object>>> actionsCopy = actions.ToList();
foreach (Tuple<WeakReference, Action<object, object>> action in actionsCopy)
{
if (action.Item1.IsAlive && actions.Contains(action))
if (action.Item1.Target != null && actions.Contains(action))
action.Item2(sender, args);

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 24, 2016

Contributor

Btw, it appears that the tuple decouples the subscriber from the action; however, we check to see if the subscriber is alive. I'm not sure if we want to or expect to see the subscriber stick around until the action completes. So an adjustment to the loop would look like this:

object subscriber = action.Item1.Target;

if(subscriber == null || !actions.Contains(action))
   continue;

action.Item2(sender, args);
GC.KeepAlive(subscriber);

Just a thought. Obviously, if the action had a strong reference to the subscriber, the GC would keep it alive, but this is just enforcing pseudo parent-child relationship.

@adrianknight89

adrianknight89 Nov 24, 2016

Contributor

Btw, it appears that the tuple decouples the subscriber from the action; however, we check to see if the subscriber is alive. I'm not sure if we want to or expect to see the subscriber stick around until the action completes. So an adjustment to the loop would look like this:

object subscriber = action.Item1.Target;

if(subscriber == null || !actions.Contains(action))
   continue;

action.Item2(sender, args);
GC.KeepAlive(subscriber);

Just a thought. Obviously, if the action had a strong reference to the subscriber, the GC would keep it alive, but this is just enforcing pseudo parent-child relationship.

@StephaneDelcroix StephaneDelcroix merged commit 1f3d5ec into xamarin:master Nov 24, 2016

3 of 4 checks passed

iOS9-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests failed: 3, …
Details
Android-UITests Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 9 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 349, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 9 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 9 :: Windows Debug : Tests passed: 3585, ignored: 10
Details

@samhouts samhouts added this to the 2.3.4 milestone Jun 27, 2018

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