Skip to content
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

[Bug] A callback was made on a garbage collected delegate of type 'Microsoft.Identity.Client.NativeInterop' #3578

Closed
1 of 7 tasks
lacomc opened this issue Aug 9, 2022 · 4 comments
Assignees
Milestone

Comments

@lacomc
Copy link

lacomc commented Aug 9, 2022

Logs and network traces
Logs: https://microsoft-my.sharepoint.com/:t:/p/laco/EeiLU2TnhIZPodyDkS5jA_oBqCjiIu-aMll07KumUjnwrQ?e=8w5w0o

Error messages shown when running in Visual Studio:
First error:

Managed Debugging Assistant 'CallbackOnCollectedDelegate'
Message=Managed Debugging Assistant 'CallbackOnCollectedDelegate' : 'A callback was made on a garbage collected delegate of type 'Microsoft.Identity.Client.NativeInterop!Microsoft.Identity.Client.NativeInterop.API+MSALRUNTIME_COMPLETION_ROUTINE::Invoke'. This may cause application crashes, corruption and data loss. When passing delegates to unmanaged code, they must be kept alive by the managed application until it is guaranteed that they will never be called.'

Continue, then hit second error:

An unhandled exception of type 'System.NullReferenceException' occurred in Unknown Module.
Object reference not set to an instance of an object.

Entire program exits

Which version of MSAL.NET are you using?
We are using special preview versions of the Microsoft.Identity.Client packages created for compatibility with 4.5 with the help of @gladjohn. Specifically related to this problem, the listed version of the NativeInterop package is being copied to our bin folder after the build has completed.
Package References:

  • Microsoft.Identity.Client Version="4.47.0"
  • Microsoft.Identity.Client.Broker Version="4.47.0-preview1-preview"
  • Microsoft.Identity.Client.Extensions.Msal Version="2.18.4"
  • Microsoft.Identity.Client.NativeInterop Version="0.11.1"

Platform
NET 4.5 with Asset Target Fallback to NET 4.6.1

What authentication flow has the issue?

  • Desktop / Mobile
    • Interactive
    • Integrated Windows Authentication
    • Username Password
    • Device code flow (browserless)
  • Web app
    • Authorization code
    • On-Behalf-Of
  • Daemon app
    • Service to Service calls

Is this a new or existing app?
The app is in production but using an older version of sign-in that does not have this issue. We started noticing the issue when attempting to upgrade.

Repro

In our csproj (truncated to only the most important info):

<PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFrameworkVersion>v4.5</TargetFrameworkVersion>
</PropertyGroup>
<PropertyGroup>
    <!--Copied from Setup.Download, allows us to use net462 libraries when available. This enables WAM sign in.-->
    <AssetTargetFallback>$(AssetTargetFallback);net461</AssetTargetFallback>
    <NoWarn>$(NoWarn);NU1701;MSB3274;MSB3275</NoWarn>
    <ResolveAssemblyReferenceIgnoreTargetFrameworkAttributeVersionMismatch>true</ResolveAssemblyReferenceIgnoreTargetFrameworkAttributeVersionMismatch>
</PropertyGroup>
<ItemGroup>
    <PackageReference Include="Microsoft.Identity.Client" Version="4.47.0" />
    <PackageReference Include="Microsoft.Identity.Client.Broker" Version="4.47.0-preview1-preview" />
    <PackageReference Include="Microsoft.Identity.Client.Extensions.Msal" Version="2.18.4" />
    <PackageReference Include="Microsoft.Identity.Client.NativeInterop" Version="0.11.1" />
</ItemGroup>
<Target Name="CopyNativeInteropDLL" AfterTargets="Build">
    <Message Text="Executing CopyNativeInteropDLL task" Importance="High" />
    <Copy SourceFiles="C:\Users\laco\.nuget\packages\microsoft.identity.client.nativeinterop\0.11.1\lib\net461\Microsoft.Identity.Client.NativeInterop.dll" DestinationFolder="C:\Setup-Engine\src\Feedback\bin\Debug\net45" />
    <Message Text="Copied build files" Importance="High" />
</Target>

C# Code:

internal async Task<SignInResult> SignInAsync(ClientType clientType)
{
    try
    {
        IPublicClientApplication appAad = await CreateAuthClientAsync(clientType, isMSA: false); // ok to use just Aad client app since acquiring token interactive (with UI)
        AuthenticationResult result = await appAad.AcquireTokenInteractive(new string[] { VsoResourceId })
            .WithPrompt(Prompt.SelectAccount)
            .ExecuteAsync();

        // get data from VSO
        UserProfileResult profile = await GetUserProfileAsync(result.AccessToken);
        if (profile == null)
        {
            return new SignInResult()
            {
                State = SignInState.Error,
                Accounts = null,
                VsidAccountMap = null,
                Error = "ERR_UNKNOWN_VSO_ACCOUNT",
            };
        }

        // return value
        UserInfo user = new UserInfo
        {
            email = result.Account.Username,
            name = profile.displayName,
            logo = "data:image/png;base64," + profile.avatarBase64,
            providerId = profile.origin,
            stale = false,
            vsid = profile.id,
        };

        Dictionary<string, IAccount> accountMap = new Dictionary<string, IAccount>
        {
            { user.vsid, result.Account },
        };

        return new SignInResult
        {
            State = SignInState.SignedIn,
            Accounts = new Accounts
            {
                users = new UserInfo[1] { user },
            },
            VsidAccountMap = accountMap,
            Error = null,
        };
    }
    catch (MsalClientException msalEx) when (msalEx.ErrorCode == MsalError.AuthenticationCanceledError)
    {
        return new SignInResult()
        {
            State = SignInState.Cancelled,
            Accounts = null,
            VsidAccountMap = null,
            Error = null,
        };
    }
    catch (Exception e)
    {
        Debug.WriteLine(e.ToString());
        return new SignInResult()
        {
            State = SignInState.Error,
            Accounts = null,
            VsidAccountMap = null,
            Error = e.ToString(),
        };
    }
}

private async Task<IPublicClientApplication> CreateAuthClientAsync(ClientType clientType, bool isMSA)
{
    IPublicClientApplication app = GetPublicClientApplication(clientType, isMSA);
    string cacheToUse = await GetCacheToUse(clientType);
    MsalCacheHelper cacheHelper = await CreateCacheHelper(cacheToUse);
    cacheHelper.RegisterCache(app.UserTokenCache);
    return app;
}

private IPublicClientApplication GetPublicClientApplication(ClientType clientType, bool isMSA)
{
    string clientId = (clientType == ClientType.VSPost2017_WAMDisabled_WAMClientId || clientType == ClientType.VSPost2017_WAMEnabled) ?
        VisualStudioClientId_WAMEnabled :
        VisualStudioClientId_NonWAM;
    PublicClientApplicationBuilder appBuilder = PublicClientApplicationBuilder.Create(clientId)
        .WithLegacyCacheCompatibility(clientType == ClientType.VS2017)
        // uncomment and implement to support proxies outside of what .NET supports by default:
        // https://docs.microsoft.com/en-us/azure/active-directory/develop/msal-net-provide-httpclient
        // part of VS uses just the .NET default code, part does not - we don't support authenticated proxies with User != SignedInUser
        // prior code we had in ServiceHub was also using .NET defaults, thus OK to keep as-is (commented)
        //.WithHttpClientFactory(IMsalHttpClientFactory...)
        .WithDefaultRedirectUri()
        .WithExtraQueryParameters(QueryParameters);

    // add tenant/authority information
    appBuilder = isMSA ? appBuilder = appBuilder.WithTenantId(ApplicationTenantId) : appBuilder = appBuilder.WithAuthority(AadAuthorityAudience.AzureAdMultipleOrgs);

    // if using WAM, enable Windows broker
    appBuilder = clientType == ClientType.VSPost2017_WAMEnabled ? EnableWAMIfAvailable(appBuilder) : appBuilder;

    return appBuilder.Build();
}

private PublicClientApplicationBuilder EnableWAMIfAvailable(PublicClientApplicationBuilder AppBuilder)
{
    return
        appBuilder
            .WithExperimentalFeatures()
            .WithWindowsBrokerOptions(
                new WindowsBrokerOptions
                {
                    MsaPassthrough = true,
                })
            .WithParentActivityOrWindow(() => GetForegroundWindow())
            .WithBrokerPreview()
            .WithLogging(MyLoggingMethod, LogLevel.Info, enablePiiLogging: true, enableDefaultPlatformLogging: false);
}

Steps:

This happens sometimes but not every time I repeat the following steps:

  1. Run the program
  2. Select "Use another account"
  3. When the account picker appears, either select an item from the list or enter new account information
  4. Click continue

Expected behavior
User is successfully logged in with the selected account and redirected to our page.

Actual behavior
The program throws the following errors:
First error:

Managed Debugging Assistant 'CallbackOnCollectedDelegate'
Message=Managed Debugging Assistant 'CallbackOnCollectedDelegate' : 'A callback was made on a garbage collected delegate of type 'Microsoft.Identity.Client.NativeInterop!Microsoft.Identity.Client.NativeInterop.API+MSALRUNTIME_COMPLETION_ROUTINE::Invoke'. This may cause application crashes, corruption and data loss. When passing delegates to unmanaged code, they must be kept alive by the managed application until it is guaranteed that they will never be called.'

Continue, then hit second error:

An unhandled exception of type 'System.NullReferenceException' occurred in Unknown Module.
Object reference not set to an instance of an object.

Possible solution
It appears that garbage collection is being done on objects from the NativeInterop package before the necessary callback is successfully made. Garbage collection should not occur until the callback is made successfully.

Additional context / logs / screenshots / links to code
This issue is specifically occurring for us due to a couple of things:

  1. We need to be compatible with NET 4.5, so we've had to work with @gladjohn to get packages specifically made for us which support this version
  2. In the project that targets NET 4.5, we add an AssetTargetFallback to NET 4.6.1 to allow the code to run successfully when NET 4.6.1 is present on the machine
  3. Likely due to some of these interesting cases with the packages, the NativeInterop dll is not being automatically copied to our bin folder and has to be copied manually
    For reference, our code can be found here for internal Microsoft users: https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/Setup-Engine?path=/&version=GBdev/laco/WAM_elevatedProcess_fix

Relevant files:

  • C:\Setup-Engine\src\Feedback\Feedback.csproj
  • C:\Setup-Engine\src\Feedback\Helpers\AuthHelper.cs
@bgavrilMS
Copy link
Member

This is saying that there is a managed delegate that is being given to C wrapper. C wrapper invokes it, but the managed GC will have already collected it.... Now, we just have to find it :)

@gladjohn gladjohn added this to the 4.47.0 milestone Aug 18, 2022
@gladjohn
Copy link
Contributor

For a repro, use the native interop debug app and add the Native Interop as a project reference. And make the following code changes to debug app

Image

Note : sometimes the issue repro's easily, if not increase the loop count.

@gladjohn
Copy link
Contributor

gladjohn commented Sep 9, 2022

private drop was given to @lacomc and she was able to verify the fix. Next MSAL release will have this issue fixed.

@gladjohn
Copy link
Contributor

MSAL 4.47.0 has been released, This issue is now fixed.

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

No branches or pull requests

4 participants