Skip to content

Commit

Permalink
Work around ancient race condition in ReactInstanceManager
Browse files Browse the repository at this point in the history
Summary:
See T55861104. In rare cases if `removeReactInstanceEventListener` is called right after (like, a small number of CPU instructions later, on a different thread) we allocate the `listeners` array with a certain size, then we could have one or more `null` listeners in the array, which is what we've been seeing in prod, at very low volumes, for several years. Without solving the root of the race condition we can just add a null check here.

Maybe it's also possible that if `addReactInstanceEventListener` is called on another thread in a racey way, that the size will be incremented on the array before we can access the additional member. That seems crazy, but maybe.

While this has been firing for multiple years it seems like a more recent change caused a regression. This diff doesn't address that and only resolves the crash.

Changelog: [Internal]

Reviewed By: ejanzer

Differential Revision: D18192801

fbshipit-source-id: c1000cfcdf6f251b03061d1386eabb9f0617a7d3
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Oct 29, 2019
1 parent bcec128 commit 0bea6a9
Showing 1 changed file with 9 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -1034,6 +1034,8 @@ private void setupReactContext(final ReactApplicationContext reactContext) {
ReactMarker.logMarker(ATTACH_MEASURED_ROOT_VIEWS_END);
}

// There is a race condition here - `finalListeners` can contain null entries
// See usage below for more details.
ReactInstanceEventListener[] listeners =
new ReactInstanceEventListener[mReactInstanceEventListeners.size()];
final ReactInstanceEventListener[] finalListeners =
Expand All @@ -1044,7 +1046,13 @@ private void setupReactContext(final ReactApplicationContext reactContext) {
@Override
public void run() {
for (ReactInstanceEventListener listener : finalListeners) {
listener.onReactContextInitialized(reactContext);
// Sometimes this listener is null - probably due to race
// condition between allocating listeners with a certain
// size, and getting a `final` version of the array on
// the following line.
if (listener != null) {
listener.onReactContextInitialized(reactContext);
}
}
}
});
Expand Down

1 comment on commit 0bea6a9

@jg210
Copy link

@jg210 jg210 commented on 0bea6a9 Mar 2, 2022

Choose a reason for hiding this comment

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

Please sign in to comment.