two-phase initialization support to prevent double-destruction on han…

…ding out this pointer in ctor (#1130) * two-phase initialization support to prevent double-destruction on handing out this pointer in ctor * PR feedback - primarily to hide/downplay the need to support Xaml with two-phase init * should Release on exception * remove unnecessary test case * PR feedback * use smart pointer instead of raw delete
microsoft · Mar 31, 2022 · 0400867 · 0400867
1 parent b69f40d
commit 0400867
Show file tree

Hide file tree

Showing 13 changed files with 202 additions and 27 deletions.
diff --git a/cppwinrt/component_writers.h b/cppwinrt/component_writers.h
@@ -860,7 +860,9 @@ catch (...) { return winrt::to_hresult(); }
         {
             auto format = R"(
 #if defined(WINRT_FORCE_INCLUDE_%_XAML_G_H) || __has_include("%.xaml.g.h")
+
 #include "%.xaml.g.h"
+
 #else
 
 namespace winrt::@::implementation

diff --git a/nuget/readme.md b/nuget/readme.md
@@ -77,6 +77,41 @@ To customize common C++/WinRT project properties:
 * expand the Common Properties item
 * select the C++/WinRT property page
 
+## InitializeComponent
+
+In older versions of C++/WinRT, Xaml objects called InitializeComponent from constructors. This can lead to memory corruption if InitializeComponent throws an exception.
+
+```cpp
+void MainPage::MainPage()
+{
+    // This pattern should no longer be used
+    InitializeComponent();
+}
+```
+
+C++/WinRT now calls InitializeComponent automatically and safely, after object construction. Explicit calls to InitializeComponent from constructors in existing code should now be removed. Multiple calls to InitializeComponent are idempotent.
+
+If a Xaml object needs to access a Xaml property during initialization, it should override InitializeComponent:
+
+```cpp
+void MainPage::InitializeComponent()
+{
+    // Call base InitializeComponent() to register with the Xaml runtime
+    MainPageT::InitializeComponent();
+    // Can now access Xaml properties
+    MyButton().Content(box_value(L"Click"));
+}
+```
+
+A non-Xaml object can also participate in two-phase construction by defining an InitializeComponent method.
+
+```cpp
+void MyComponent::InitializeComponent()
+{
+    // Execute initialization logic that may throw 
+}
+```
+
 ## Troubleshooting
 
 The msbuild verbosity level maps to msbuild message importance as follows:

diff --git a/strings/base_implements.h b/strings/base_implements.h
@@ -1218,6 +1218,29 @@ namespace winrt::impl
     };
 #endif
 
+    template<typename T>
+    class has_initializer
+    {
+        template <typename U, typename = decltype(std::declval<U>().InitializeComponent())> static constexpr bool get_value(int) { return true; }
+        template <typename> static constexpr bool get_value(...) { return false; }
+
+    public:
+        static constexpr bool value = get_value<T>(0);
+    };
+
+    template<typename T, typename... Args>
+    T* create_and_initialize(Args&&... args)
+    {
+        com_ptr<T> instance{ new heap_implements<T>(std::forward<Args>(args)...), take_ownership_from_abi };
+
+        if constexpr (has_initializer<T>::value)
+        {
+            instance->InitializeComponent();
+        }
+
+        return instance.detach();
+    }
+
     inline com_ptr<IStaticLifetimeCollection> get_static_lifetime_map()
     {
         auto const lifetime_factory = get_activation_factory<impl::IStaticLifetime>(L"Windows.ApplicationModel.Core.CoreApplication");
@@ -1233,7 +1256,7 @@ namespace winrt::impl
 
         if constexpr (!has_static_lifetime_v<D>)
         {
-            return { to_abi<result_type>(new heap_implements<D>), take_ownership_from_abi };
+            return { to_abi<result_type>(create_and_initialize<D>()), take_ownership_from_abi };
         }
         else
         {
@@ -1247,7 +1270,7 @@ namespace winrt::impl
                 return { result, take_ownership_from_abi };
             }
 
-            result_type object{ to_abi<result_type>(new heap_implements<D>), take_ownership_from_abi };
+            result_type object{ to_abi<result_type>(create_and_initialize<D>()), take_ownership_from_abi };
 
             static slim_mutex lock;
             slim_lock_guard const guard{ lock };
@@ -1293,17 +1316,17 @@ WINRT_EXPORT namespace winrt
         }
         else if constexpr (impl::has_composable<D>::value)
         {
-            impl::com_ref<I> result{ to_abi<I>(new impl::heap_implements<D>(std::forward<Args>(args)...)), take_ownership_from_abi };
+            impl::com_ref<I> result{ to_abi<I>(impl::create_and_initialize<D>(std::forward<Args>(args)...)), take_ownership_from_abi };
             return result.template as<typename D::composable>();
         }
         else if constexpr (impl::has_class_type<D>::value)
         {
             static_assert(std::is_same_v<I, default_interface<typename D::class_type>>);
-            return typename D::class_type{ to_abi<I>(new impl::heap_implements<D>(std::forward<Args>(args)...)), take_ownership_from_abi };
+            return typename D::class_type{ to_abi<I>(impl::create_and_initialize<D>(std::forward<Args>(args)...)), take_ownership_from_abi };
         }
         else
         {
-            return impl::com_ref<I>{ to_abi<I>(new impl::heap_implements<D>(std::forward<Args>(args)...)), take_ownership_from_abi };
+            return impl::com_ref<I>{ to_abi<I>(impl::create_and_initialize<D>(std::forward<Args>(args)...)), take_ownership_from_abi };
         }
     }
 
@@ -1325,7 +1348,7 @@ WINRT_EXPORT namespace winrt
         }
         else
         {
-            return { new impl::heap_implements<D>(std::forward<Args>(args)...), take_ownership_from_abi };
+            return { impl::create_and_initialize<D>(std::forward<Args>(args)...), take_ownership_from_abi };
         }
     }
 

diff --git a/test/test/initialize.cpp b/test/test/initialize.cpp
@@ -0,0 +1,119 @@
+#include "pch.h"
+
+using namespace winrt;
+using namespace Windows::Foundation;
+
+namespace
+{
+    class some_exception : public std::exception
+    {
+    public:
+        some_exception() noexcept
+            : exception("some_exception", 1)
+        {
+        }
+    };
+
+    template<typename D>
+    struct InitializeT : implements<D, IStringable>
+    {
+        bool& m_initialize_called;
+
+        InitializeT(bool& initialize_called) : m_initialize_called(initialize_called)
+        {
+        }
+
+        ~InitializeT()
+        {
+        }
+
+        void InitializeComponent()
+        {
+            m_initialize_called = true;
+            throw some_exception();
+        }
+
+        hstring ToString()
+        {
+            return {};
+        }
+    };
+
+    struct Initialize : InitializeT<Initialize>
+    {
+        Initialize(bool& initialize_called) : InitializeT(initialize_called)
+        {
+        }
+    };
+
+    struct ThrowingDerived : InitializeT<ThrowingDerived>
+    {
+        ThrowingDerived(bool& initialize_called) : InitializeT(initialize_called)
+        {
+            throw some_exception();
+        }
+    };
+
+    struct OverriddenInitialize : InitializeT<OverriddenInitialize>
+    {
+        OverriddenInitialize(bool& initialize_called) : InitializeT(initialize_called)
+        {
+        }
+
+        void InitializeComponent()
+        {
+            m_initialize_called = true;
+        }
+    };
+}
+
+TEST_CASE("initialize")
+{
+    // Ensure that failure to initialize is failure to instantiate, with no side effects
+    {
+        bool initialize_called{};
+        bool exception_caught{};
+        try
+        {
+            make<Initialize>(initialize_called);
+        }
+        catch (some_exception const&)
+        {
+            exception_caught = true;
+        }
+        REQUIRE(initialize_called);
+        REQUIRE(exception_caught);
+    }
+
+    // Ensure that base is never initialized if exception thrown from derived/base constructor
+    {
+        bool initialize_called{};
+        bool exception_caught{};
+        try
+        {
+            make<ThrowingDerived>(initialize_called);
+        }
+        catch (some_exception const&)
+        {
+            exception_caught = true;
+        }
+        REQUIRE(!initialize_called);
+        REQUIRE(exception_caught);
+    }
+
+    // Support for overriding initialization for post-processing (e.g., accessing Xaml properties)
+    {
+        bool initialize_called{};
+        bool exception_caught{};
+        try
+        {
+            make<OverriddenInitialize>(initialize_called);
+        }
+        catch (some_exception const&)
+        {
+            exception_caught = true;
+        }
+        REQUIRE(initialize_called);
+        REQUIRE(!exception_caught);
+    }
+}
diff --git a/test/test/test.vcxproj b/test/test/test.vcxproj
@@ -369,6 +369,7 @@
     </ClCompile>
     <ClCompile Include="hstring_empty.cpp" />
     <ClCompile Include="iid_ppv_args.cpp" />
+    <ClCompile Include="initialize.cpp" />
     <ClCompile Include="inspectable_interop.cpp">
       <PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">NotUsing</PrecompiledHeader>
       <PrecompiledHeader Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">NotUsing</PrecompiledHeader>

diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.cpp b/vsix/ItemTemplates/BlankPage/BlankPage.cpp
@@ -9,11 +9,6 @@ using namespace Windows::UI::Xaml;
 
 namespace winrt::$rootnamespace$::implementation
 {
-    $safeitemname$::$safeitemname$()
-    {
-        InitializeComponent();
-    }
-
     int32_t $safeitemname$::MyProperty()
     {
         throw hresult_not_implemented();

diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.h b/vsix/ItemTemplates/BlankPage/BlankPage.h
@@ -6,7 +6,11 @@ namespace winrt::$rootnamespace$::implementation
 {
     struct $safeitemname$ : $safeitemname$T<$safeitemname$>
     {
-        $safeitemname$();
+        $safeitemname$() 
+        {
+            // Xaml objects should not call InitializeComponent during construction.
+            // See https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent
+        }
 
         int32_t MyProperty();
         void MyProperty(int32_t value);

diff --git a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp
@@ -9,11 +9,6 @@ using namespace Windows::UI::Xaml;
 
 namespace winrt::$rootnamespace$::implementation
 {
-    $safeitemname$::$safeitemname$()
-    {
-        InitializeComponent();
-    }
-
     int32_t $safeitemname$::MyProperty()
     {
         throw hresult_not_implemented();

diff --git a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h
@@ -10,7 +10,11 @@ namespace winrt::$rootnamespace$::implementation
 {
     struct $safeitemname$ : $safeitemname$T<$safeitemname$>
     {
-        $safeitemname$();
+        $safeitemname$() 
+        {
+            // Xaml objects should not call InitializeComponent during construction.
+            // See https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent
+        }
 
         int32_t MyProperty();
         void MyProperty(int32_t value);

diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.cpp b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.cpp
@@ -14,12 +14,11 @@ using namespace $safeprojectname$;
 using namespace $safeprojectname$::implementation;
 
 /// <summary>
-/// Initializes the singleton application object.  This is the first line of authored code
+/// Creates the singleton application object.  This is the first line of authored code
 /// executed, and as such is the logical equivalent of main() or WinMain().
 /// </summary>
 App::App()
 {
-    InitializeComponent();
     Suspending({ this, &App::OnSuspending });
 
 #if defined _DEBUG && !defined DISABLE_XAML_GENERATED_BREAK_ON_UNHANDLED_EXCEPTION

diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h
@@ -6,7 +6,6 @@ namespace winrt::$safeprojectname$::implementation
     struct App : AppT<App>
     {
         App();
-
         void OnLaunched(Windows::ApplicationModel::Activation::LaunchActivatedEventArgs const&);
         void OnSuspending(IInspectable const&, Windows::ApplicationModel::SuspendingEventArgs const&);
         void OnNavigationFailed(IInspectable const&, Windows::UI::Xaml::Navigation::NavigationFailedEventArgs const&);

diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.cpp b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.cpp
@@ -7,11 +7,6 @@ using namespace Windows::UI::Xaml;
 
 namespace winrt::$safeprojectname$::implementation
 {
-    MainPage::MainPage()
-    {
-        InitializeComponent();
-    }
-
     int32_t MainPage::MyProperty()
     {
         throw hresult_not_implemented();

diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h
@@ -6,7 +6,11 @@ namespace winrt::$safeprojectname$::implementation
 {
     struct MainPage : MainPageT<MainPage>
     {
-        MainPage();
+        MainPage()
+        {
+            // Xaml objects should not call InitializeComponent during construction.
+            // See https://github.com/microsoft/cppwinrt/tree/master/nuget#initializecomponent
+        }
 
         int32_t MyProperty();
         void MyProperty(int32_t value);