From ef7c5341912a8d77031dd63974bc044e93abf605 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Tue, 29 Mar 2022 17:56:06 -0700 Subject: [PATCH 1/6] two-phase initialization support to prevent double-destruction on handing out this pointer in ctor --- cppwinrt/component_writers.h | 12 ++ strings/base_implements.h | 62 ++++++- test/test/initialize_instance.cpp | 165 ++++++++++++++++++ test/test/test.vcxproj | 1 + vsix/ItemTemplates/BlankPage/BlankPage.cpp | 7 +- vsix/ItemTemplates/BlankPage/BlankPage.h | 1 + .../BlankUserControl/BlankUserControl.cpp | 7 +- .../BlankUserControl/BlankUserControl.h | 1 + .../VC/Windows Universal/BlankApp/App.cpp | 12 +- .../VC/Windows Universal/BlankApp/App.h | 3 +- .../Windows Universal/BlankApp/MainPage.cpp | 7 +- .../VC/Windows Universal/BlankApp/MainPage.h | 1 + 12 files changed, 267 insertions(+), 12 deletions(-) create mode 100644 test/test/initialize_instance.cpp diff --git a/cppwinrt/component_writers.h b/cppwinrt/component_writers.h index f562dabef..ee372d3c7 100644 --- a/cppwinrt/component_writers.h +++ b/cppwinrt/component_writers.h @@ -860,7 +860,17 @@ 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" +namespace winrt::@::implementation +{ + template + auto initialize_instance(%T& instance) + { + static_cast(instance).InitializeComponent(); + } +} + #else namespace winrt::@::implementation @@ -883,6 +893,8 @@ namespace winrt::@::implementation include_path, type_namespace, type_name, + type_namespace, + type_name, type_name); } } diff --git a/strings/base_implements.h b/strings/base_implements.h index e2c6f3a12..e5cb6feea 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1218,6 +1218,56 @@ namespace winrt::impl }; #endif + template + class has_initialize_instance_member + { + template ().initialize_instance())> static constexpr bool get_value(int) { return true; } + template static constexpr bool get_value(...) { return false; } + + public: + static constexpr bool value = get_value(0); + }; + + template + class has_initialize_instance_free + { + template static constexpr bool get_value(int) { return true; } + template static constexpr bool get_value(...) { return false; } + + public: + static constexpr bool value = get_value(0); + }; + + template + auto initialize_instance(T* instance) + { + if constexpr (has_initialize_instance_member::value) + { + try + { + instance->initialize_instance(); + } + catch (...) + { + delete instance; + throw; + } + } + else if constexpr (has_initialize_instance_free::value) + { + try + { + initialize_instance(*instance); + } + catch (...) + { + delete instance; + throw; + } + } + return instance; + } + inline com_ptr get_static_lifetime_map() { auto const lifetime_factory = get_activation_factory(L"Windows.ApplicationModel.Core.CoreApplication"); @@ -1233,7 +1283,7 @@ namespace winrt::impl if constexpr (!has_static_lifetime_v) { - return { to_abi(new heap_implements), take_ownership_from_abi }; + return { to_abi(initialize_instance(new heap_implements)), take_ownership_from_abi }; } else { @@ -1247,7 +1297,7 @@ namespace winrt::impl return { result, take_ownership_from_abi }; } - result_type object{ to_abi(new heap_implements), take_ownership_from_abi }; + result_type object{ to_abi(initialize_instance(new heap_implements)), take_ownership_from_abi }; static slim_mutex lock; slim_lock_guard const guard{ lock }; @@ -1293,17 +1343,17 @@ WINRT_EXPORT namespace winrt } else if constexpr (impl::has_composable::value) { - impl::com_ref result{ to_abi(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; + impl::com_ref result{ to_abi(impl::initialize_instance(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; return result.template as(); } else if constexpr (impl::has_class_type::value) { static_assert(std::is_same_v>); - return typename D::class_type{ to_abi(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; + return typename D::class_type{ to_abi(impl::initialize_instance(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; } else { - return impl::com_ref{ to_abi(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; + return impl::com_ref{ to_abi(impl::initialize_instance(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; } } @@ -1325,7 +1375,7 @@ WINRT_EXPORT namespace winrt } else { - return { new impl::heap_implements(std::forward(args)...), take_ownership_from_abi }; + return { impl::initialize_instance(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; } } diff --git a/test/test/initialize_instance.cpp b/test/test/initialize_instance.cpp new file mode 100644 index 000000000..c91f67865 --- /dev/null +++ b/test/test/initialize_instance.cpp @@ -0,0 +1,165 @@ +#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) + { + } + }; + + struct MemberInitialize : implements + { + bool& m_initialize_called; + + MemberInitialize(bool& initialize_called) : m_initialize_called(initialize_called) + { + } + + ~MemberInitialize() + { + } + + void initialize_instance() + { + m_initialize_called = true; + throw some_exception(); + } + + hstring ToString() + { + return {}; + } + }; + + template + struct FreeInitializeT : implements + { + bool& m_initialize_called; + + FreeInitializeT(bool& initialize_called) : m_initialize_called(initialize_called) + { + } + + ~FreeInitializeT() + { + } + + void InitializeComponent() + { + m_initialize_called = true; + throw some_exception(); + } + + hstring ToString() + { + return {}; + } + }; + + struct FreeInitialize : FreeInitializeT + { + FreeInitialize(bool& initialize_called) : FreeInitializeT(initialize_called) + { + } + }; + + template + auto initialize_instance(FreeInitializeT& instance) + { + static_cast(instance).InitializeComponent(); + } + + struct ThrowingDerived : FreeInitializeT + { + ThrowingDerived(bool& initialize_called) : FreeInitializeT(initialize_called) + { + throw some_exception(); + } + }; + + struct OverriddenInitialize : FreeInitializeT + { + OverriddenInitialize(bool& initialize_called) : FreeInitializeT(initialize_called) + { + } + + void InitializeComponent() + { + m_initialize_called = true; + } + }; +} + +TEST_CASE("initialize_instance") +{ + // Ensure that failure to initialize is failure to instantiate, with no side effects + { + bool initialize_called{}; + bool exception_caught{}; + try + { + make(initialize_called); + } + catch (some_exception const&) + { + exception_caught = true; + } + REQUIRE(initialize_called); + REQUIRE(exception_caught); + } + + // Same as above, but with free initialization function (Xaml scenario) + { + bool initialize_called{}; + bool exception_caught{}; + try + { + make(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(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(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 index dc6c96cea..232edd950 100644 --- a/test/test/test.vcxproj +++ b/test/test/test.vcxproj @@ -369,6 +369,7 @@ + NotUsing NotUsing diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.cpp b/vsix/ItemTemplates/BlankPage/BlankPage.cpp index 9af284cec..590384120 100644 --- a/vsix/ItemTemplates/BlankPage/BlankPage.cpp +++ b/vsix/ItemTemplates/BlankPage/BlankPage.cpp @@ -11,7 +11,12 @@ namespace winrt::$rootnamespace$::implementation { $safeitemname$::$safeitemname$() { - InitializeComponent(); + // Xaml properties should be accessed after InitializeComponent + } + + void $safeitemname$::InitializeComponent() + { + $safeitemname$T::InitializeComponent(); } int32_t $safeitemname$::MyProperty() diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.h b/vsix/ItemTemplates/BlankPage/BlankPage.h index f1dc903e8..f8c427131 100644 --- a/vsix/ItemTemplates/BlankPage/BlankPage.h +++ b/vsix/ItemTemplates/BlankPage/BlankPage.h @@ -7,6 +7,7 @@ namespace winrt::$rootnamespace$::implementation struct $safeitemname$ : $safeitemname$T<$safeitemname$> { $safeitemname$(); + void InitializeComponent(); int32_t MyProperty(); void MyProperty(int32_t value); diff --git a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp index 9af284cec..590384120 100644 --- a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp +++ b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp @@ -11,7 +11,12 @@ namespace winrt::$rootnamespace$::implementation { $safeitemname$::$safeitemname$() { - InitializeComponent(); + // Xaml properties should be accessed after InitializeComponent + } + + void $safeitemname$::InitializeComponent() + { + $safeitemname$T::InitializeComponent(); } int32_t $safeitemname$::MyProperty() diff --git a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h index 7b994b1df..781a3fc2a 100644 --- a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h +++ b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h @@ -11,6 +11,7 @@ namespace winrt::$rootnamespace$::implementation struct $safeitemname$ : $safeitemname$T<$safeitemname$> { $safeitemname$(); + void 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 index 8806e25cb..fff589fbe 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.cpp +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.cpp @@ -14,12 +14,12 @@ using namespace $safeprojectname$; using namespace $safeprojectname$::implementation; /// -/// 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(). +/// Xaml properties should be accessed after InitializeComponent. /// App::App() { - InitializeComponent(); Suspending({ this, &App::OnSuspending }); #if defined _DEBUG && !defined DISABLE_XAML_GENERATED_BREAK_ON_UNHANDLED_EXCEPTION @@ -34,6 +34,14 @@ App::App() #endif } +/// +/// Initializes the singleton application object, registering it with the Xaml runtime. +/// +void App::InitializeComponent() +{ + AppT::InitializeComponent(); +} + /// /// Invoked when the application is launched normally by the end user. Other entry points /// will be used such as when the application is launched to open a specific file. diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h index 8208308c8..514551738 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h @@ -6,7 +6,8 @@ namespace winrt::$safeprojectname$::implementation struct App : AppT { App(); - + void InitializeComponent(); + void initialize_instance(){ InitializeComponent(); } 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 index 5caaa46e6..6a77fac72 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.cpp +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.cpp @@ -9,7 +9,12 @@ namespace winrt::$safeprojectname$::implementation { MainPage::MainPage() { - InitializeComponent(); + // Xaml properties should be accessed after InitializeComponent + } + + void MainPage::InitializeComponent() + { + MainPageT::InitializeComponent(); } int32_t MainPage::MyProperty() diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h index 96c433917..d29d37cd8 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h @@ -7,6 +7,7 @@ namespace winrt::$safeprojectname$::implementation struct MainPage : MainPageT { MainPage(); + void InitializeComponent(); int32_t MyProperty(); void MyProperty(int32_t value); From 2e0656b4c0f983ae0d98d43315c6aecf33edb269 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Wed, 30 Mar 2022 11:03:18 -0700 Subject: [PATCH 2/6] PR feedback - primarily to hide/downplay the need to support Xaml with two-phase init --- cppwinrt/component_writers.h | 10 ----- strings/base_implements.h | 43 +++++-------------- ...initialize_instance.cpp => initialize.cpp} | 10 +---- test/test/test.vcxproj | 2 +- vsix/ItemTemplates/BlankPage/BlankPage.cpp | 10 ----- vsix/ItemTemplates/BlankPage/BlankPage.h | 3 +- .../BlankUserControl/BlankUserControl.cpp | 10 ----- .../BlankUserControl/BlankUserControl.h | 3 +- .../VC/Windows Universal/BlankApp/App.cpp | 9 ---- .../VC/Windows Universal/BlankApp/App.h | 2 - .../Windows Universal/BlankApp/MainPage.cpp | 10 ----- .../VC/Windows Universal/BlankApp/MainPage.h | 3 +- 12 files changed, 17 insertions(+), 98 deletions(-) rename test/test/{initialize_instance.cpp => initialize.cpp} (94%) diff --git a/cppwinrt/component_writers.h b/cppwinrt/component_writers.h index ee372d3c7..fcb9f5ac7 100644 --- a/cppwinrt/component_writers.h +++ b/cppwinrt/component_writers.h @@ -862,14 +862,6 @@ catch (...) { return winrt::to_hresult(); } #if defined(WINRT_FORCE_INCLUDE_%_XAML_G_H) || __has_include("%.xaml.g.h") #include "%.xaml.g.h" -namespace winrt::@::implementation -{ - template - auto initialize_instance(%T& instance) - { - static_cast(instance).InitializeComponent(); - } -} #else @@ -893,8 +885,6 @@ namespace winrt::@::implementation include_path, type_namespace, type_name, - type_namespace, - type_name, type_name); } } diff --git a/strings/base_implements.h b/strings/base_implements.h index e5cb6feea..60238fa0c 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1219,45 +1219,24 @@ namespace winrt::impl #endif template - class has_initialize_instance_member + class has_initializer { - template ().initialize_instance())> static constexpr bool get_value(int) { return true; } + template ().InitializeComponent())> static constexpr bool get_value(int) { return true; } template static constexpr bool get_value(...) { return false; } public: static constexpr bool value = get_value(0); }; - template - class has_initialize_instance_free - { - template static constexpr bool get_value(int) { return true; } - template static constexpr bool get_value(...) { return false; } - - public: - static constexpr bool value = get_value(0); - }; template - auto initialize_instance(T* instance) + auto initialize(T* instance) { - if constexpr (has_initialize_instance_member::value) - { - try - { - instance->initialize_instance(); - } - catch (...) - { - delete instance; - throw; - } - } - else if constexpr (has_initialize_instance_free::value) + if constexpr (has_initializer::value) { try { - initialize_instance(*instance); + instance->InitializeComponent(); } catch (...) { @@ -1283,7 +1262,7 @@ namespace winrt::impl if constexpr (!has_static_lifetime_v) { - return { to_abi(initialize_instance(new heap_implements)), take_ownership_from_abi }; + return { to_abi(initialize(new heap_implements)), take_ownership_from_abi }; } else { @@ -1297,7 +1276,7 @@ namespace winrt::impl return { result, take_ownership_from_abi }; } - result_type object{ to_abi(initialize_instance(new heap_implements)), take_ownership_from_abi }; + result_type object{ to_abi(initialize(new heap_implements)), take_ownership_from_abi }; static slim_mutex lock; slim_lock_guard const guard{ lock }; @@ -1343,17 +1322,17 @@ WINRT_EXPORT namespace winrt } else if constexpr (impl::has_composable::value) { - impl::com_ref result{ to_abi(impl::initialize_instance(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; + impl::com_ref result{ to_abi(impl::initialize(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; return result.template as(); } else if constexpr (impl::has_class_type::value) { static_assert(std::is_same_v>); - return typename D::class_type{ to_abi(impl::initialize_instance(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; + return typename D::class_type{ to_abi(impl::initialize(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; } else { - return impl::com_ref{ to_abi(impl::initialize_instance(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; + return impl::com_ref{ to_abi(impl::initialize(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; } } @@ -1375,7 +1354,7 @@ WINRT_EXPORT namespace winrt } else { - return { impl::initialize_instance(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; + return { impl::initialize(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; } } diff --git a/test/test/initialize_instance.cpp b/test/test/initialize.cpp similarity index 94% rename from test/test/initialize_instance.cpp rename to test/test/initialize.cpp index c91f67865..6e091ac73 100644 --- a/test/test/initialize_instance.cpp +++ b/test/test/initialize.cpp @@ -26,7 +26,7 @@ namespace { } - void initialize_instance() + void InitializeComponent() { m_initialize_called = true; throw some_exception(); @@ -70,12 +70,6 @@ namespace } }; - template - auto initialize_instance(FreeInitializeT& instance) - { - static_cast(instance).InitializeComponent(); - } - struct ThrowingDerived : FreeInitializeT { ThrowingDerived(bool& initialize_called) : FreeInitializeT(initialize_called) @@ -97,7 +91,7 @@ namespace }; } -TEST_CASE("initialize_instance") +TEST_CASE("initialize") { // Ensure that failure to initialize is failure to instantiate, with no side effects { diff --git a/test/test/test.vcxproj b/test/test/test.vcxproj index 232edd950..8a570d258 100644 --- a/test/test/test.vcxproj +++ b/test/test/test.vcxproj @@ -369,7 +369,7 @@ - + NotUsing NotUsing diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.cpp b/vsix/ItemTemplates/BlankPage/BlankPage.cpp index 590384120..7410a619b 100644 --- a/vsix/ItemTemplates/BlankPage/BlankPage.cpp +++ b/vsix/ItemTemplates/BlankPage/BlankPage.cpp @@ -9,16 +9,6 @@ using namespace Windows::UI::Xaml; namespace winrt::$rootnamespace$::implementation { - $safeitemname$::$safeitemname$() - { - // Xaml properties should be accessed after InitializeComponent - } - - void $safeitemname$::InitializeComponent() - { - $safeitemname$T::InitializeComponent(); - } - int32_t $safeitemname$::MyProperty() { throw hresult_not_implemented(); diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.h b/vsix/ItemTemplates/BlankPage/BlankPage.h index f8c427131..0f9313d96 100644 --- a/vsix/ItemTemplates/BlankPage/BlankPage.h +++ b/vsix/ItemTemplates/BlankPage/BlankPage.h @@ -6,8 +6,7 @@ namespace winrt::$rootnamespace$::implementation { struct $safeitemname$ : $safeitemname$T<$safeitemname$> { - $safeitemname$(); - void InitializeComponent(); + $safeitemname$() {} int32_t MyProperty(); void MyProperty(int32_t value); diff --git a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp index 590384120..7410a619b 100644 --- a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp +++ b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.cpp @@ -9,16 +9,6 @@ using namespace Windows::UI::Xaml; namespace winrt::$rootnamespace$::implementation { - $safeitemname$::$safeitemname$() - { - // Xaml properties should be accessed after InitializeComponent - } - - void $safeitemname$::InitializeComponent() - { - $safeitemname$T::InitializeComponent(); - } - int32_t $safeitemname$::MyProperty() { throw hresult_not_implemented(); diff --git a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h index 781a3fc2a..7be344989 100644 --- a/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h +++ b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h @@ -10,8 +10,7 @@ namespace winrt::$rootnamespace$::implementation { struct $safeitemname$ : $safeitemname$T<$safeitemname$> { - $safeitemname$(); - void InitializeComponent(); + $safeitemname$() {} 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 index fff589fbe..2f3132d65 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.cpp +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.cpp @@ -16,7 +16,6 @@ using namespace $safeprojectname$::implementation; /// /// 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(). -/// Xaml properties should be accessed after InitializeComponent. /// App::App() { @@ -34,14 +33,6 @@ App::App() #endif } -/// -/// Initializes the singleton application object, registering it with the Xaml runtime. -/// -void App::InitializeComponent() -{ - AppT::InitializeComponent(); -} - /// /// Invoked when the application is launched normally by the end user. Other entry points /// will be used such as when the application is launched to open a specific file. diff --git a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h index 514551738..b7fe65ef7 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/App.h @@ -6,8 +6,6 @@ namespace winrt::$safeprojectname$::implementation struct App : AppT { App(); - void InitializeComponent(); - void initialize_instance(){ InitializeComponent(); } 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 index 6a77fac72..af94324c3 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.cpp +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.cpp @@ -7,16 +7,6 @@ using namespace Windows::UI::Xaml; namespace winrt::$safeprojectname$::implementation { - MainPage::MainPage() - { - // Xaml properties should be accessed after InitializeComponent - } - - void MainPage::InitializeComponent() - { - MainPageT::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 index d29d37cd8..aa4d5485f 100644 --- a/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h +++ b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h @@ -6,8 +6,7 @@ namespace winrt::$safeprojectname$::implementation { struct MainPage : MainPageT { - MainPage(); - void InitializeComponent(); + MainPage(){} int32_t MyProperty(); void MyProperty(int32_t value); From 6f2b56ea4dd3783b404052d10fa43b0ba3301b71 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Wed, 30 Mar 2022 11:50:06 -0700 Subject: [PATCH 3/6] should Release on exception --- strings/base_implements.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/strings/base_implements.h b/strings/base_implements.h index 60238fa0c..266228c34 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1240,7 +1240,7 @@ namespace winrt::impl } catch (...) { - delete instance; + instance->Release(); throw; } } From 8a5291989b13b4774a5820bebacd9b16952b7977 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Wed, 30 Mar 2022 12:24:11 -0700 Subject: [PATCH 4/6] remove unnecessary test case --- test/test/initialize.cpp | 60 +++++++--------------------------------- 1 file changed, 10 insertions(+), 50 deletions(-) diff --git a/test/test/initialize.cpp b/test/test/initialize.cpp index 6e091ac73..dbd95c3fe 100644 --- a/test/test/initialize.cpp +++ b/test/test/initialize.cpp @@ -14,40 +14,16 @@ namespace } }; - struct MemberInitialize : implements - { - bool& m_initialize_called; - - MemberInitialize(bool& initialize_called) : m_initialize_called(initialize_called) - { - } - - ~MemberInitialize() - { - } - - void InitializeComponent() - { - m_initialize_called = true; - throw some_exception(); - } - - hstring ToString() - { - return {}; - } - }; - template - struct FreeInitializeT : implements + struct InitializeT : implements { bool& m_initialize_called; - FreeInitializeT(bool& initialize_called) : m_initialize_called(initialize_called) + InitializeT(bool& initialize_called) : m_initialize_called(initialize_called) { } - ~FreeInitializeT() + ~InitializeT() { } @@ -63,24 +39,24 @@ namespace } }; - struct FreeInitialize : FreeInitializeT + struct Initialize : InitializeT { - FreeInitialize(bool& initialize_called) : FreeInitializeT(initialize_called) + Initialize(bool& initialize_called) : InitializeT(initialize_called) { } }; - struct ThrowingDerived : FreeInitializeT + struct ThrowingDerived : InitializeT { - ThrowingDerived(bool& initialize_called) : FreeInitializeT(initialize_called) + ThrowingDerived(bool& initialize_called) : InitializeT(initialize_called) { throw some_exception(); } }; - struct OverriddenInitialize : FreeInitializeT + struct OverriddenInitialize : InitializeT { - OverriddenInitialize(bool& initialize_called) : FreeInitializeT(initialize_called) + OverriddenInitialize(bool& initialize_called) : InitializeT(initialize_called) { } @@ -99,23 +75,7 @@ TEST_CASE("initialize") bool exception_caught{}; try { - make(initialize_called); - } - catch (some_exception const&) - { - exception_caught = true; - } - REQUIRE(initialize_called); - REQUIRE(exception_caught); - } - - // Same as above, but with free initialization function (Xaml scenario) - { - bool initialize_called{}; - bool exception_caught{}; - try - { - make(initialize_called); + make(initialize_called); } catch (some_exception const&) { From f8d0247144f4f0a20441e7e1456791a24bfd9a47 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Thu, 31 Mar 2022 08:43:24 -0700 Subject: [PATCH 5/6] PR feedback --- nuget/readme.md | 35 +++++++++++++++++++ strings/base_implements.h | 18 +++++----- vsix/ItemTemplates/BlankPage/BlankPage.h | 6 +++- .../BlankUserControl/BlankUserControl.h | 6 +++- .../VC/Windows Universal/BlankApp/MainPage.h | 6 +++- 5 files changed, 59 insertions(+), 12 deletions(-) diff --git a/nuget/readme.md b/nuget/readme.md index 566a4fd18..4d925d06d 100644 --- 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 index 266228c34..ef850b280 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1228,10 +1228,10 @@ namespace winrt::impl static constexpr bool value = get_value(0); }; - - template - auto initialize(T* instance) + template + auto create_and_initialize(Args&&... args) { + auto instance = new heap_implements(std::forward(args)...); if constexpr (has_initializer::value) { try @@ -1262,7 +1262,7 @@ namespace winrt::impl if constexpr (!has_static_lifetime_v) { - return { to_abi(initialize(new heap_implements)), take_ownership_from_abi }; + return { to_abi(create_and_initialize()), take_ownership_from_abi }; } else { @@ -1276,7 +1276,7 @@ namespace winrt::impl return { result, take_ownership_from_abi }; } - result_type object{ to_abi(initialize(new heap_implements)), take_ownership_from_abi }; + result_type object{ to_abi(create_and_initialize()), take_ownership_from_abi }; static slim_mutex lock; slim_lock_guard const guard{ lock }; @@ -1322,17 +1322,17 @@ WINRT_EXPORT namespace winrt } else if constexpr (impl::has_composable::value) { - impl::com_ref result{ to_abi(impl::initialize(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; + impl::com_ref result{ to_abi(impl::create_and_initialize(std::forward(args)...)), take_ownership_from_abi }; return result.template as(); } else if constexpr (impl::has_class_type::value) { static_assert(std::is_same_v>); - return typename D::class_type{ to_abi(impl::initialize(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; + return typename D::class_type{ to_abi(impl::create_and_initialize(std::forward(args)...)), take_ownership_from_abi }; } else { - return impl::com_ref{ to_abi(impl::initialize(new impl::heap_implements(std::forward(args)...))), take_ownership_from_abi }; + return impl::com_ref{ to_abi(impl::create_and_initialize(std::forward(args)...)), take_ownership_from_abi }; } } @@ -1354,7 +1354,7 @@ WINRT_EXPORT namespace winrt } else { - return { impl::initialize(new impl::heap_implements(std::forward(args)...)), take_ownership_from_abi }; + return { impl::create_and_initialize(std::forward(args)...), take_ownership_from_abi }; } } diff --git a/vsix/ItemTemplates/BlankPage/BlankPage.h b/vsix/ItemTemplates/BlankPage/BlankPage.h index 0f9313d96..58c0f3648 100644 --- 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.h b/vsix/ItemTemplates/BlankUserControl/BlankUserControl.h index 7be344989..0ab08377a 100644 --- 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/MainPage.h b/vsix/ProjectTemplates/VC/Windows Universal/BlankApp/MainPage.h index aa4d5485f..92e0a689d 100644 --- 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() + { + // 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); From ed7fd74c0ec4102b77eb412c0ef9fcbb5f96a3a4 Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Thu, 31 Mar 2022 09:02:30 -0700 Subject: [PATCH 6/6] use smart pointer instead of raw delete --- strings/base_implements.h | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/strings/base_implements.h b/strings/base_implements.h index ef850b280..212f2ecf7 100644 --- a/strings/base_implements.h +++ b/strings/base_implements.h @@ -1229,22 +1229,16 @@ namespace winrt::impl }; template - auto create_and_initialize(Args&&... args) + T* create_and_initialize(Args&&... args) { - auto instance = new heap_implements(std::forward(args)...); + com_ptr instance{ new heap_implements(std::forward(args)...), take_ownership_from_abi }; + if constexpr (has_initializer::value) { - try - { - instance->InitializeComponent(); - } - catch (...) - { - instance->Release(); - throw; - } + instance->InitializeComponent(); } - return instance; + + return instance.detach(); } inline com_ptr get_static_lifetime_map()