Fix ReactInstance error state to avoid crashes #4986
Conversation
| if (!strongThis->m_isLoaded) { | ||
| strongThis->m_isLoaded = true; | ||
| strongThis->m_state = ReactInstanceState::Loaded; | ||
| if (!errorCode) { |
There was a problem hiding this comment.
when I looked at this, I saw that if I let the instance stay in HasError, I could never reload the app:
the repro was:
don't start the bundler
start the app, get redbox, instance goes to HasError
press reload ----> no reload happens but redbox gets dismissed. app is left unusable.
Can you please confirm this works? can you think of why I might have seen that? 😋
There was a problem hiding this comment.
The reload button works. If I dismiss the redbox, the app seems to unusable, but I can click Ctrl+Shift+D to reload it. If I run the "yarn start" before the reload, then I see the UI.
I guess the main difference between this change and what probably you did is that I let all other code to run as it was before. It is very important for the rest of the system to report that the load process is completed, even if it is completed in an error state. It allows to propagate all the messages and complete the load operation in the ActionQueue.
The ActionQueue expects that each action reports completion before it starts the next one. BTW, it also coalesce the actions. E.g. if unload is requested multiple times, only one of them is kept.
There was a problem hiding this comment.
I have verified the scenario again. The reloading worked fine as described above. I saw the app crash when we resize the window. It was caused by the RedBox code. I think we must clean the m_redboxContent on the RedBox closing.
@acoates-ms could you please see if changes to the RedBox look good?
vmoroz
added
the
AutoMerge
|
Hello @vmoroz! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
* Fix ReactInstance error state to avoid crashes * Change files * Clean m_redboxContent after RedBox closing
* Fix ReactInstance error state to avoid crashes * Change files * Clean m_redboxContent after RedBox closing
|
Moving from "Request Backport" to "Backport Approved" after earlier discussion. |
|
Removing "Backport to 0.62" since we're about to rename it to "Backported to 0.62" as part of backporting label migration. This has been approved to go into 0.62 though, and such has the "Backport approved" label. |
* Implemented support for native module std::weak_ptr (#4980) (cherry picked from commit c23d3de) # Conflicts: # packages/microsoft-reactnative-sampleapps/windows/SampleLibraryCPP/SampleModuleCPP.h * Fix ReactInstance error state to avoid crashes (#4986) * Fix ReactInstance error state to avoid crashes * Change files * Clean m_redboxContent after RedBox closing (cherry picked from commit 05779f2) * ReactNotificationService to allow communication between native modules (#4953) Only picking up changes required for ReactDispatcher. (cherry picked from commit baba475) * Add UIDispatcher property to ReactInstanceSettings and IReactContext (#5007) * Add UIDispatcher property to ReactInstanceSettings and IReactContext * Change files * Fix ReactWindows-Desktop.sln compilation * Address PR feedback for the GetCurrentUIThreadQueue * Remove UIDispatcher setting from Playground project (cherry picked from commit 7236d61) # Conflicts: # packages/playground/windows/playground-win32/Playground-Win32.cpp # vnext/Desktop/React.Windows.Desktop.vcxproj.filters # vnext/Microsoft.ReactNative.Cxx.UnitTests/ReactContextTest.cpp # vnext/Microsoft.ReactNative.Cxx.UnitTests/ReactModuleBuilderMock.h # vnext/Microsoft.ReactNative.Cxx/ReactContext.h # vnext/Microsoft.ReactNative.Managed.UnitTests/ReactModuleBuilderMock.cs # vnext/Microsoft.ReactNative/IReactContext.cpp # vnext/Microsoft.ReactNative/IReactContext.h # vnext/Microsoft.ReactNative/IReactContext.idl # vnext/Microsoft.ReactNative/ReactInstanceSettings.h # vnext/Microsoft.ReactNative/ReactInstanceSettings.idl * Fix PlaygroundWin32 compilation
4 participants
This change is to address the issue #4923.
The crash was caused by React Instance not setting HasError state on error.
Since the state was as if the instance loaded correctly, it was causing crashes when code continues execution.
In this PR we are addressing the issue by setting the HasError state on error.
Microsoft Reviewers: Open in CodeFlow