Removed the ref count work around from classes derived from ReactApplication #4755

vmoroz · 2020-04-30T15:53:04Z

If user code directly inherits from the XAML Application class then it works just fine.
The issue is that if we inherit our ReactApplication from the XAML Applicaiton, and then user code inherits from the ReacApplication, then the C++/WinRT current COM aggregation fails to work correctly and we have a crash.

To work around the crash we have added the following lines to the user App constructor:

App::App() noexcept {
 ....
 // This works around a cpp/winrt bug with composable/aggregable types tracked
 // by 22116519
 AddRef();
 m_inner.as<::IUnknown>()->Release();
}

It was good for a quick workaround while we worked on the ReactApplication prototype, but in the long run this code has no place in user applications, especially the Windows bug number that has no meaning for non-Microsoft users.

In this PR we are implementing a work around inside of our ReactApplication suggested by @jevansaks. The change makes sure that we pass a correct pointer to the outer class for the COM aggregation to work. It allowed us to remove the work around bits from the sample and template code.

IMPORTANT: All previously created applications MUST remove the work around code mentioned above to work with the new Microsoft.ReactNative.dll. Otherwise, they will crash. The new solution frees the user code from the work-around, but this is also a requirement to have applications working.

Microsoft Reviewers: Open in CodeFlow

…ications

NikoAri · 2020-04-30T16:44:01Z

vnext/Microsoft.ReactNative/ReactApplication.h

+  using base_type = NoDefaultCtorReactApplication_base;
+  using class_type = Microsoft::ReactNative::ReactApplication;
+  using implements_type = typename NoDefaultCtorReactApplication_base::implements_type;
+  using implements_type::implements_type;


using implements_type::implements_type; [](start = 2, length = 39)

(nit) what does this do?

It brings constructors from the implements_type base class. I am not sure why it is need. I effectively copy&pasted the generated code and did minimal changes to it: deleted default constructor.

In reply to: 418146298 [](ancestors = 418146298)

NikoAri

jevansaks · 2020-04-30T16:50:33Z

IMPORTANT: All previously created applications MUST remove the work around code mentioned above to work with the new Microsoft.ReactNative.dll. Otherwise, they will crash

It occurred to me that we might be able to cause user code using this pattern to fail to compile instead by exposing an "m_inner" protected field on ReactApplication of a bogus type. Then code of the old pattern would fail to compile with something like "no member Release()" or "ambiguous blah blah".

jevansaks · 2020-04-30T16:53:37Z

vnext/Microsoft.ReactNative/ReactApplication.h

+  }
+};
+
+struct ReactApplication : NoDefaultCtorReactApplication_base<ReactApplication> {


Why did the base type for this need to change? Was there something provided in the generated code that was conflicting with this override?

The default constructor of the ReactApplicationT calls the factory for the XAML Application class. If I do not replace the base class, then the XAML Application is going to be created twice: one in the base default constructor and the second in the our constructor with the outer reference.

Ideally, the generated class must have two constructors or one with an optional outer parameter.

In reply to: 418151912 [](ancestors = 418151912)

Oh I see. Yes that makes sense.

vmoroz · 2020-04-30T17:45:17Z

I do not see how we can do it. Users consume this code through the ABI boundary. There is nothing we can add there to make it fail. The code here does not affect the C++/WinRT generated code that is consumed in the user code.

In reply to: 621973435 [](ancestors = 621973435)

jevansaks · 2020-04-30T17:57:00Z

I do not see how we can do it. Users consume this code through the ABI boundary. There is nothing we can add there to make it fail. The code here does not affect the C++/WinRT generated code that is consumed in the user code.

Oh right, the m_inner is the one from C++/WinRT in the app's code. Hmmm. Well if this becomes a problem and we want a more visible way to flag it we might be able to come up with a way to detect this in ReactApplication at runtime.

NickGerleman · 2020-04-30T18:32:20Z

@vmoroz what's the motivation for getting this one into 0.62 instead of waiting until 0.63 (which should be coming sooner rather than later)?

Breaking changes in stable branches are best to avoid if possible. API defects in new bits are a good reason to cause breaks, but this issue has been around since 0.60 or 0.61, right?

vmoroz · 2020-04-30T19:48:30Z

I thought it would be better overall, since the 0.62 is not completely stable yet and it is the version people will try after the Build conference. Since it is a breaking change, we will have more unhappy customers down the road. Porting this to 0.62 seems to be a 'less evil' than doing it in a month. Anyway, I do not have a strong opinion about it. I am OK making it 0.63 only and thus removed the both tags.

In reply to: 622026743 [](ancestors = 622026743)

vmoroz · 2020-04-30T19:55:57Z

Agree. We should see first if it becomes a big problem after the release.

In reply to: 622009181 [](ancestors = 622009181)

NickGerleman · 2020-05-01T03:11:49Z

Adding the "breaking change" label back since we will want to document this regardless of when it goes in.

I can totally sympathize with wanting this in sooner so less people end up using the API in the first place. I'd still probably rather hold off on 0.62 just because we're trying to explicitly avoid breaking changes in preview builds when they aren't to correct issues we've just introduced.

…ication (microsoft#4755) * Removed the ref count work around from classes derived from ReactApplications * Change files

vmoroz added 2 commits April 30, 2020 08:22

Removed the ref count work around from classes derived from ReactAppl…

833861c

…ications

Change files

07a2b2e

vmoroz added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Apr 30, 2020

vmoroz requested review from a team, NickGerleman, acoates-ms, aeulitz, asklar and jonthysell April 30, 2020 15:53

vmoroz requested a review from a team as a code owner April 30, 2020 15:53

vmoroz changed the title ~~Removed the ref count work around from classes derived from ReactApplications~~ Removed AddRef/Release from classes derived from ReactApplications Apr 30, 2020

vmoroz changed the title ~~Removed AddRef/Release from classes derived from ReactApplications~~ Removed the ref count work around from classes derived from ReactApplication Apr 30, 2020

vmoroz added the Backport to 0.62 label Apr 30, 2020

NikoAri reviewed Apr 30, 2020

View reviewed changes

NikoAri approved these changes Apr 30, 2020

View reviewed changes

jevansaks reviewed Apr 30, 2020

View reviewed changes

vmoroz removed Backport to 0.62 Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release labels Apr 30, 2020

vmoroz merged commit 9d9a541 into microsoft:master Apr 30, 2020

NickGerleman added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label May 1, 2020

NickGerleman mentioned this pull request May 2, 2020

Determine How to Handle Breaking Change Causing Runtime Crash #4776

Closed

NickGerleman added the Backport to 0.62 label May 6, 2020

NickGerleman added a commit to NickGerleman/react-native-windows that referenced this pull request May 7, 2020

Fix namespaces for microsoft#4755

c887e8f

Removed the ref count work around from classes derived from ReactApplication #4755

Removed the ref count work around from classes derived from ReactApplication #4755

Uh oh!

Conversation

vmoroz commented Apr 30, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

NikoAri Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

vmoroz Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

NikoAri left a comment

Choose a reason for hiding this comment

Uh oh!

jevansaks commented Apr 30, 2020

Uh oh!

jevansaks Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

vmoroz Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

jevansaks Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

vmoroz commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jevansaks commented Apr 30, 2020

Uh oh!

NickGerleman commented Apr 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vmoroz commented Apr 30, 2020

Uh oh!

vmoroz commented Apr 30, 2020

Uh oh!

NickGerleman commented May 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vmoroz commented Apr 30, 2020 •

edited by ghost

Loading

vmoroz commented Apr 30, 2020 •

edited

Loading

NickGerleman commented Apr 30, 2020 •

edited

Loading