Skip to content

Commit

Permalink
Multi window race condition fixes (#1926)
Browse files Browse the repository at this point in the history 
1. Sometimes JS tried to create views before the parent root view has been added to shadow tree. This is due to UIManagerModule.AddMeasuredRootView being a "fire and forget" method. We made this an awaitable asynchronous function, so now AppRegistry.runApplication is only called after the measured root view addition completed.
2. Back to back "create/remove" root view operations had some corner cases (trying to remove a root view when its tag hasn't been set yet, or trying to remove a root view that is still waiting for measuring to occur)
Fixed those.
3. Added a stress UT.
  • Loading branch information
@reseul
reseul committed Aug 8, 2018
1 parent cfd5368 commit 6273bc6
Show file tree
Hide file tree
Showing 8 changed files with 272 additions and 24 deletions.
18 changes: 9 additions & 9 deletions ReactWindows/ReactNative.Shared/ReactInstanceManager.cs
View file
Expand Up @@ -422,7 +422,7 @@ public async Task AttachMeasuredRootViewAsync(ReactRootView rootView)
rootView.Children.Clear();
ViewExtensions.ClearData(rootView);

await DispatcherHelpers.CallOnDispatcher(() =>
await DispatcherHelpers.CallOnDispatcher(async () =>
{
_attachedRootViews.Add(rootView);
Expand All @@ -433,11 +433,11 @@ public async Task AttachMeasuredRootViewAsync(ReactRootView rootView)
var currentReactContext = _currentReactContext;
if (currentReactContext != null)
{
AttachMeasuredRootViewToInstance(rootView, currentReactContext.ReactInstance);
await AttachMeasuredRootViewToInstanceAsync(rootView, currentReactContext.ReactInstance);
}
return true;
}, true); // inlining allowed
}, true).Unwrap(); // inlining allowed
}

/// <summary>
Expand Down Expand Up @@ -591,7 +591,7 @@ private Task<ReactContext> CreateReactContextFromCachedPackagerBundleAsync(Cance
try
{
var reactContext = await CreateReactContextCoreAsync(jsExecutorFactory, jsBundleLoader, token);
SetupReactContext(reactContext);
await SetupReactContextAsync(reactContext);
return reactContext;
}
catch (OperationCanceledException)
Expand All @@ -607,7 +607,7 @@ private Task<ReactContext> CreateReactContextFromCachedPackagerBundleAsync(Cance
return null;
}

private void SetupReactContext(ReactContext reactContext)
private async Task SetupReactContextAsync(ReactContext reactContext)
{
DispatcherHelpers.AssertOnDispatcher();
if (_currentReactContext != null)
Expand All @@ -624,7 +624,7 @@ private void SetupReactContext(ReactContext reactContext)

foreach (var rootView in _attachedRootViews)
{
AttachMeasuredRootViewToInstance(rootView, reactInstance);
await AttachMeasuredRootViewToInstanceAsync(rootView, reactInstance);
}
}

Expand All @@ -634,13 +634,13 @@ private void InvokeDefaultOnBackPressed()
_defaultBackButtonHandler?.Invoke();
}

private void AttachMeasuredRootViewToInstance(
private async Task AttachMeasuredRootViewToInstanceAsync(
ReactRootView rootView,
IReactInstance reactInstance)
{
DispatcherHelpers.AssertOnDispatcher();
var rootTag = reactInstance.GetNativeModule<UIManagerModule>()
.AddMeasuredRootView(rootView);
var rootTag = await reactInstance.GetNativeModule<UIManagerModule>()
.AddMeasuredRootViewAsync(rootView);

var jsAppModuleName = rootView.JavaScriptModuleName;
var appParameters = new Dictionary<string, object>
Expand Down
13 changes: 8 additions & 5 deletions ReactWindows/ReactNative.Shared/ReactRootView.cs
View file
Expand Up @@ -134,7 +134,9 @@ private async Task StartReactApplicationAsync(ReactInstanceManager reactInstance

var getReactContextTaskTask =
DispatcherHelpers.CallOnDispatcher(async () => await _reactInstanceManager.GetOrCreateReactContextAsync(CancellationToken.None), true);


await getReactContextTaskTask.Unwrap();

// We need to wait for the initial `Measure` call, if this view has
// not yet been measured, we set the `_attachScheduled` flag, which
// will enable deferred attachment of the root node.
Expand All @@ -146,8 +148,6 @@ private async Task StartReactApplicationAsync(ReactInstanceManager reactInstance
{
_attachScheduled = true;
}

await getReactContextTaskTask.Unwrap();
}

/// <summary>
Expand All @@ -167,6 +167,7 @@ public void StopReactApplication()
/// <remarks>
/// Has to be called under the dispatcher associated with the view.
/// </remarks>
/// <returns>Awaitable task.</returns>
public async Task StopReactApplicationAsync()
{
DispatcherHelpers.AssertOnDispatcher(this);
Expand All @@ -176,6 +177,8 @@ public async Task StopReactApplicationAsync()
{
await reactInstanceManager.DetachRootViewAsync(this);
}

_attachScheduled = false;
}

/// <summary>
Expand Down Expand Up @@ -241,9 +244,9 @@ private async Task MeasureOverrideHelperAsync()
var reactInstanceManager = _reactInstanceManager;
if (_attachScheduled && reactInstanceManager != null)
{
_attachScheduled = false;

await reactInstanceManager.AttachMeasuredRootViewAsync(this);

_attachScheduled = false;
}
}

Expand Down
1 change: 1 addition & 0 deletions ReactWindows/ReactNative.Shared/UIManager/NativeViewHierarchyManager.cs
View file
Expand Up @@ -663,6 +663,7 @@ private void AddRootViewParent(int tag, FrameworkElement view, ThemedReactContex
_tagsToViewManagers.Add(tag, _rootViewManager);
_rootTags.Add(tag, true);

// Keeping here for symmetry, tag on root views is set early, in UIManagerModule.AddMeasuredRootViewAsync
ViewExtensions.SetTag(view, tag);
ViewExtensions.SetReactContext(view, themedContext);
#if WINDOWS_UWP
Expand Down
8 changes: 6 additions & 2 deletions ReactWindows/ReactNative.Shared/UIManager/UIManagerModule.cs
View file
Expand Up @@ -125,17 +125,20 @@ public UIImplementation UIImplementation
/// JavaScript can use the returned tag with to add or remove children
/// to this view through <see cref="manageChildren(int, int[], int[], int[], int[], int[])"/>.
/// </remarks>
public int AddMeasuredRootView(ReactRootView rootView)
public async Task<int> AddMeasuredRootViewAsync(ReactRootView rootView)
{
// Called on main dispatcher thread
DispatcherHelpers.AssertOnDispatcher();

var tag = _nextRootTag;
_nextRootTag += RootViewTagIncrement;

// Set tag early in case of concurrent DetachRootViewAsync
rootView.SetTag(tag);

var context = new ThemedReactContext(Context);

DispatcherHelpers.RunOnDispatcher(rootView.Dispatcher, () =>
await DispatcherHelpers.CallOnDispatcher(rootView.Dispatcher, () =>
{
var width = rootView.ActualWidth;
var height = rootView.ActualHeight;
Expand Down Expand Up @@ -167,6 +170,7 @@ public int AddMeasuredRootView(ReactRootView rootView)
// Register view in DeviceInfoModule for tracking its dimensions
Context.GetNativeModule<DeviceInfoModule>().RegisterRootView(rootView, tag);
#endif
return true;
}, true); // Allow inlining

return tag;
Expand Down
36 changes: 28 additions & 8 deletions ReactWindows/ReactNative.Tests/Internal/DispatcherHelpers.cs
View file
Expand Up @@ -9,16 +9,26 @@ namespace ReactNative.Tests
{
static class DispatcherHelpers
{
public static async Task RunOnDispatcherAsync(Action action)
public static Task RunOnDispatcherAsync(Action action)
{
await App.Dispatcher.RunAsync(CoreDispatcherPriority.Normal, new DispatchedHandler(action)).AsTask().ConfigureAwait(false);
return RunOnDispatcherAsync(App.Dispatcher, action);
}

public static async Task<T> CallOnDispatcherAsync<T>(Func<T> func)
public static async Task RunOnDispatcherAsync(CoreDispatcher dispatcher, Action action)
{
await dispatcher.RunAsync(CoreDispatcherPriority.Normal, new DispatchedHandler(action)).AsTask().ConfigureAwait(false);
}

public static Task<T> CallOnDispatcherAsync<T>(Func<T> func)
{
return CallOnDispatcherAsync<T>(App.Dispatcher, func);
}

public static async Task<T> CallOnDispatcherAsync<T>(CoreDispatcher dispatcher, Func<T> func)
{
var tcs = new TaskCompletionSource<T>(TaskCreationOptions.RunContinuationsAsynchronously);

await RunOnDispatcherAsync(() =>
await RunOnDispatcherAsync(dispatcher, () =>
{
try
{
Expand All @@ -34,11 +44,16 @@ public static async Task<T> CallOnDispatcherAsync<T>(Func<T> func)
return await tcs.Task.ConfigureAwait(false);
}

public static async Task CallOnDispatcherAsync(Func<Task> asyncFunc)
public static Task CallOnDispatcherAsync(Func<Task> asyncFunc)
{
return CallOnDispatcherAsync(App.Dispatcher, asyncFunc);
}

public static async Task CallOnDispatcherAsync(CoreDispatcher dispatcher, Func<Task> asyncFunc)
{
var tcs = new TaskCompletionSource<bool>(TaskCreationOptions.RunContinuationsAsynchronously);

await RunOnDispatcherAsync(async () =>
await RunOnDispatcherAsync(dispatcher, async () =>
{
try
{
Expand All @@ -54,11 +69,16 @@ public static async Task CallOnDispatcherAsync(Func<Task> asyncFunc)
await tcs.Task.ConfigureAwait(false);
}

public static async Task<T> CallOnDispatcherAsync<T>(Func<Task<T>> asyncFunc)
public static Task<T> CallOnDispatcherAsync<T>(Func<Task<T>> asyncFunc)
{
return CallOnDispatcherAsync(App.Dispatcher, asyncFunc);
}

public static async Task<T> CallOnDispatcherAsync<T>(CoreDispatcher dispatcher, Func<Task<T>> asyncFunc)
{
var tcs = new TaskCompletionSource<T>(TaskCreationOptions.RunContinuationsAsynchronously);

await RunOnDispatcherAsync(async () =>
await RunOnDispatcherAsync(dispatcher, async () =>
{
try
{
Expand Down
16 changes: 16 additions & 0 deletions ReactWindows/ReactNative.Tests/ReactNative.Tests.csproj
View file
Expand Up @@ -18,8 +18,15 @@
<ProjectTypeGuids>{A5A43C5B-DE2A-4C0C-9213-0A381AF9435A};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
<PackageCertificateKeyFile>ReactNative.Tests_TemporaryKey.pfx</PackageCertificateKeyFile>
<UnitTestPlatformVersion Condition="'$(UnitTestPlatformVersion)' == ''">14.0</UnitTestPlatformVersion>
<DependencyConfiguration>Debug</DependencyConfiguration>
<PackageCertificateThumbprint>F0FB82837F45EE9E0EFC3B38576F3C1523EAC45A</PackageCertificateThumbprint>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)' == 'Debug' or '$(Configuration)' == 'DebugBundle'">
<DependencyConfiguration>Debug</DependencyConfiguration>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)' == 'Release' or '$(Configuration)' == 'ReleaseBundle'">
<DependencyConfiguration>Release</DependencyConfiguration>
</PropertyGroup>
<PropertyGroup Condition="'$(Configuration)|$(Platform)' == 'Debug|x86'">
<DebugSymbols>true</DebugSymbols>
<OutputPath>bin\x86\Debug\</OutputPath>
Expand Down Expand Up @@ -111,6 +118,7 @@
<Compile Include="Modules\Storage\AsyncStorageModuleTests.cs" />
<Compile Include="Properties\AssemblyInfo.cs" />
<Compile Include="ReactInstanceManagerTests.cs" />
<Compile Include="ReactRootViewTests.cs" />
<Compile Include="UIManager\BorderedCanvasTests.cs" />
<Compile Include="UIManager\Events\EventDispatcherTests.cs" />
<Compile Include="UIManager\Events\EventTests.cs" />
Expand Down Expand Up @@ -142,6 +150,7 @@
<Content Include="Assets\StoreLogo.png" />
<Content Include="Assets\Wide310x150Logo.scale-200.png" />
<Content Include="Resources\immediate.js" />
<Content Include="Resources\mwtest.js" />
<Content Include="Resources\sync.js">
<CopyToOutputDirectory>Always</CopyToOutputDirectory>
</Content>
Expand All @@ -158,6 +167,13 @@
<Name>ReactNative</Name>
</ProjectReference>
</ItemGroup>
<ItemGroup>
<Content Include="$(MSBuildThisFileDirectory)..\..\Yoga\csharp\Yoga\bin\Universal\$(Platform)\$(DependencyConfiguration)\yoga.dll">
<Link>%(Filename)%(Extension)</Link>
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
<Visible>False</Visible>
</Content>
</ItemGroup>
<Import Project="..\ReactNative.Shared.Tests\ReactNative.Shared.Tests.projitems" Label="Shared" />
<ItemGroup>
<PackageReference Include="Microsoft.NETCore.UniversalWindowsPlatform">
Expand Down

0 comments on commit 6273bc6

Please sign in to comment.