two-phase initialization support to prevent double-destruction on handing out this pointer in ctor

[Scottj1s]

Initialization routines for components are now called after construction, rather than during. This prevents 'this' pointers from being handed out and retained during construction, leading to multiple destructor calls. If a component has an initialize_instance function (hopefully ugly enough named to prevent need for opting out), member or free (so that the Xaml compiler doesn't require a coordinated fix), then this function is called after the implementation is completely constructed. If the initializer throws, the instance is destructed and the exception propagates as before. Existing code can now remove InitializeComponent calls from class constructors, or override in order to access Xaml properties after initialization. The support is general - non-Xaml code can also define initialize_instance.

[sylveon]

Is there such an example of this pointers being handed out and causing double destruction?

Also, isn't this change potentially source breaking? e.g. after updating cppwinrt now InitializeComponent will be called twice unless the developer goes and update their code for all controls (which could be a lot).

In any case, I'm not sure how I feel about moving InitializeComponent out of the constructor, since it adds a place where C# and C++ code for the same thing diverges (making porting or moving between languages harder)

[kennykerr]

I never liked this pattern. Not sure I like the idea of adding it now...

[kennykerr]

Initialization routines for components are now called after construction, rather than during. This prevents 'this' pointers from being handed out and retained during construction, leading to multiple destructor calls.

Can you provide a minimal example of this problem? Maybe there's a less intrusive solution.

[oldnewthing]

@sylveon @kennykerr The scenario for this is as follows:

namespace winrt::Contoso::implementation
{
    struct MyPage : MyPageT<MyPage>
    {
        MyPage()
        {
            this->InitializeComponent();
            something_that_throws();
        }
    };
}

template<typename D, typename... I>
void MyPageT<D, I...>::InitializeComponent()
{
    if (!_contentLoaded)
    {
        _contentLoaded = true;
        Uri resourceLocator{ L"ms-appx:///MyPage.xaml" };
        Application::LoadComponent(*this, resourceLocator, ComponentResourceLocation::Application);
    }
}

The InitializeComponent method hands out a reference to a partially constructed this to Application::LoadComponent, which dutifully calls AddRef to retain the reference.

And then something_that_throws(); throws an exception, which causes construction to fail, and the object is forcible destroyed, leaving LoadComponent with a dangling reference.

The problem is that InitializeComponent() is writing a check it may not be able to cover: It is calling LoadComponent with an IInspectable, which comes with the promise that AddRef can be used to extend its lifetime. But since the object is still constructing, the constructor may fail, and then the object forcibly goes away, ignoring any reference count.

This is the canonical example, but there are other cases, such as event registration with a strong reference, or setting up bindings, which have the same root cause.

[jlaanstra]

Does Application.LoadComponent hold on to this after the LoadComponent call returns? That seems surprising to me. How would I tell xaml to give up that reference to allow my class to get destructed? Seems like as long as the LoadComponent call doesn't hold on to its own copy of this there is no issue?

[Scottj1s]

Also, isn't this change potentially source breaking? e.g. after updating cppwinrt now InitializeComponent will be called twice unless the developer goes and update their code for all controls (which could be a lot).

Generally, that would be a concern. But in this case (InitializeComponent, the motivation), redundant calls are idempotent.

[Scottj1s]

In any case, I'm not sure how I feel about moving InitializeComponent out of the constructor, since it adds a place where C# and C++ code for the same thing diverges (making porting or moving between languages harder)

Yes, the behavior is different for C# and C++. They're different languages and porting between them has never been trivial.

[Scottj1s]

I never liked this pattern. Not sure I like the idea of adding it now...

The trick is how to handle initialization outside construction, without causing major disruption to existing code.

[Scottj1s]

Can you provide a minimal example of this problem? Maybe there's a less intrusive solution.

Unfortunately, I don't have a repro (they're hard to come by for this issue). But the root cause is handing out 'this' pointers from a constructor which is a Bad Idea. Totally open to alternatives. But moving InitializeComponent out of the constructor is going to be somewhat disruptive. Xaml property accessors, like MyButton(), must not be called before InitializeComponent. So they would have to move along with the call, and that at minimum requires support for user overriding.

[Scottj1s]

Does Application.LoadComponent hold on to this after the LoadComponent call returns? That seems surprising to me. How would I tell xaml to give up that reference to allow my class to get destructed? Seems like as long as the LoadComponent call doesn't hold on to its own copy of this there is no issue?

Yes, it certainly does, via a PegNoRef call in the Xaml runtime. And ordinarily, that would be paired with an UnpegNoRef to take Xaml's hands off the same raw pointer. But in the case of premature object tear-down due to exceptions in construction (which can originate from winrt::check_hresult calls in the Xaml runtime as well), we then have a dangling pointer and a time-bomb when UnpegNoRef attempts to release it.

[sylveon]

Don't we hit the same double-free issue with this PR?

void MyControl::InitializeComponent()
{
    MyControlT::InitializeComponent();
    something_that_throws();
}

Because the XAML framework holds a reference to it but then something_that_throws causes C++/WinRT to immediately call delete on it, ignoring any ref counting going on:

cppwinrt/strings/base_implements.h

Line 1252 in ef7c534

delete instance;

[ChrisGuzak]

This is a classic problem that is not specific to Xaml. There is a variation of this for free-threaded events, where the event is raised before the object is fully constructed. The advice we give is never hand out a reference to this in the constructor, use 2 phase construction based on a static factory function for the class.

Another approach would make this the developer's problem, ask them to opt into this behavior.

[Scottj1s]

This is a classic problem that is not specific to Xaml. There is a variation of this for free-threaded events, where the event is raised before the object is fully constructed. The advice we give is never hand out a reference to this in the constructor, use 2 phase construction based on a static factory function for the class.

Another approach would make this the developer's problem, ask them to opt into this behavior.

Agree with you and Kenny. I will update the PR to make this solution specific to Xaml (not general), which will keep it hidden away as an implementation detail. Class authors can invent an InitializeComponent method if they want to participate, but it won't be documented.

[Scottj1s]

Don't we hit the same double-free issue with this PR?

No, we don't. The reason this is safe is because we've eliminated the overlap between exception-based destruction and storing an object pointer - all from within the constructor chain. Here, the object that's held indirectly by InitializeComponent is fully constructed, with proper refcount bookkeeping. The unit test exercises this scenario.

[ChrisGuzak]

@Scottj1s I'd like this to be available for use outside of Xaml. I've not looked at the change yet to see if that is viable.

[jlaanstra]

@Scottj1s I'd like this to be available for use outside of Xaml. I've not looked at the change yet to see if that is viable.

Agree with this. It sounds like this is useful outside of Xaml. Could we make this similar to final_release, not required, but if you want to use it it's there? Maybe the PR already does that :).

[Scottj1s]

@ChrisGuzak, @jlaanstra - there is some utility to this functionality, but I'm persuaded we shouldn't go out of our way to advertise it. So I'll standardize on Xaml's function name, InitializeComponent, versus the more winrt-friendly name, initialize_instance. If any class defines (or derives from a class defining) an InitializeComponent, that function will be called outside construction.

[Scottj1s]

Second iteration - all component template ctors are now empty and defined inline. This reduces the temptation to do anything interesting in that constructor. E.g., if a developer drops in a call to MyButton(), it will fail without a preceding InitializeComponent(), which is what we're trying to discourage.

[jlaanstra]

Does Application.LoadComponent hold on to this after the LoadComponent call returns? That seems surprising to me. How would I tell xaml to give up that reference to allow my class to get destructed? Seems like as long as the LoadComponent call doesn't hold on to its own copy of this there is no issue?

Yes, it certainly does, via a PegNoRef call in the Xaml runtime. And ordinarily, that would be paired with an UnpegNoRef to take Xaml's hands off the same raw pointer. But in the case of premature object tear-down due to exceptions in construction (which can originate from winrt::check_hresult calls in the Xaml runtime as well), we then have a dangling pointer and a time-bomb when UnpegNoRef attempts to release it.

Could UnpefNoRef simply be inside a wil::scope_exit block so it release the pointer when the stack unwinds avoiding the dangling pointer? Or are you saying that xaml holds on to this even after InitializeComponent returns? If it holds on even after InitializeComponent returns how does Xaml decide to release the pointer eventually and not cause a memory leak?

[sylveon]

Here, the object that's held indirectly by InitializeComponent is fully constructed, with proper refcount bookkeeping

But in winrt::impl::initialize_instance the object is delete'd without checking the ref count at all if the initializer throws. So unless I'm missing something, the object is deleted while Xaml still holds a reference to it, then Xaml calls Release() on an already deleted object. This seems especially true if there is user code that may throw after the Xaml InitializeComponent function. Because "something_that_throws(); throws an exception, which causes construction to fail, and the object is forcible destroyed, leaving LoadComponent with a dangling reference." is exactly what happens here. The object is forcibly destroyed if the user-provided InitializeComponent function throws.

but I'm persuaded we shouldn't go out of our way to advertise it.

I think the feature should be at least documented because it's important to let devs know that Xaml elements can't be manipulated in the constructor anymore, and there is a "magic name" they need to use to be able to do that.

[Scottj1s]

I've opened a related improvement request for the Xaml compiler, to diagnose property accessor calls without InitializeComponent having first been called.

https://microsoft.visualstudio.com/OS/_workitems/edit/38777131

[Scottj1s]

thanks all!

[sylveon]

Trying to adopt this in my code - but now I'm hitting a little issue. I do some stuff in a constructor that both manipulates XAML, and accesses constructor params. This can't work if I want to move the XAML manipulation stuff in an overriden InitializeComponent because I don't get passed constructor parameters there. Is the recommended solution in this case to just store the constructor parameters needed in fields? Doesn't that waste memory, especially if passing big objects/strings and then forgetting to release them at the end of the custom InitializeComponent's body?

Maybe adding a way to also pass constructor parameters to InitializeComponent would be nice. Something like checking if is_invocable<&D::InitializeComponent, D*, Args...> is true, if yes pass constructor params if not call without params.