Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(windows): Fix crash when running windows module #719

Merged

Conversation

vmoroz
Copy link
Contributor

@vmoroz vmoroz commented Nov 28, 2021

Summary

Lately we saw a number of Watson crash reports in Windows that show crashes due to the result callbacks called twice in case if there are errors related to the database operations. The analysis has shown that the code has no protection from the callbacks being called twice and that the multiMerge method is not implemented in a safe way as other methods intended to be.

Details

This PR is relatively big. It is almost a full rewrite of Windows module code. These are the changes:

  • To ensure that the result callbacks are called exactly once (not more or less than once) we are adding Promise class that invokes the result callback when the Promise is resolved or rejected. To ensure that the callback is called not more than once, it uses m_isCompleted Boolean atomic variable. To ensure that the callback is called at least once, the Reject method is called from destructor.
  • To implement strongly-typed module API, the new KeyValue and Error classes are added. These new types have custom ReadValue and WriteValue function overloads to convert to/from JSON. It allowed to stop using JSValue in favor of the strongly typed classes such as std::vector, std::string, KeyValue, and Error.
  • The Database initialization is moved from constructor to the InitializeStorage method that allows to avoid using exceptions and to report errors the same way as other async task methods. This method is called on demand when a task started.
  • All error reporting was changed to uniformly add errors to the error list associated with the Promise and indicate error result by the std::nullopt value. Most of all functions return std::optional.
  • Convenience macros CHECK and CHECK_SQL_OK are added to simplify code by reducing if-return statements for each function call.
  • UniquePtrSetter is added to allow natural use of std::unique_ptr for SQL object handlers.
  • Since the updated DBTask is not copyable, the task list changed to hold std::unique_ptr<DBTask instances.
  • Implemented MultiMerge method as other methods inside of DBTask. We needed to fix algorithm that matches keys between old and new values to use std::unordered_set instead of relying on their index which may be different and cause incorrect merges.
  • Fixed MergeJsonObjects code to always check value type and that key exists.
  • Cleaned up the Sqlite3Transaction class code.
  • Fixed Microsoft copyright notice to follow the official company guidelines.

Test Plan

The code is manually tested using the example app. The example app is generated using the following command:

yarn install-windows-test-app -p example/windows

Copy link

@asklar asklar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, left a few comments. Thanks for taking the time to rewrite it, looks cleaner now : )

windows/code/DBStorage.h Outdated Show resolved Hide resolved
windows/code/DBStorage.h Show resolved Hide resolved
windows/code/RNCAsyncStorage.h Outdated Show resolved Hide resolved
windows/code/DBStorage.cpp Outdated Show resolved Hide resolved
windows/code/DBStorage.cpp Show resolved Hide resolved
windows/code/DBStorage.cpp Show resolved Hide resolved
Copy link
Member

@tido64 tido64 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krizzu: Do you want to take a look?

windows/code/DBStorage.h Show resolved Hide resolved
windows/code/DBStorage.cpp Outdated Show resolved Hide resolved
windows/code/DBStorage.cpp Show resolved Hide resolved
windows/code/DBStorage.cpp Show resolved Hide resolved
@krizzu
Copy link
Member

krizzu commented Nov 29, 2021

@krizzu: Do you want to take a look?

I'll leave it with you 👍 😄

Shorten type names suggested by @tido64 in PR review

Co-authored-by: Tommy Nguyen <4123478+tido64@users.noreply.github.com>
@tido64 tido64 merged commit 309e252 into react-native-async-storage:master Nov 30, 2021
@krizzu
Copy link
Member

krizzu commented Nov 30, 2021

🎉 This PR is included in version 1.15.14 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants