Skip to content

Commit

Permalink
Fabric: Attempt to recover from invariant violation in ~Sheduler
Browse files Browse the repository at this point in the history
Summary:
We see crashes that happen in the destructor of `EventEmitterWrapper` class which indicates that `EventEmitter`s were not "disabled" before `Runtime` was deallocated. Assuming that `Scheduler` *was* deallocated before the `Runtime` (there is no proof for that), we can suppose that `stopSurface` was not called properly on time. Again, assuming that the moment of Scheduler deallocation is fine, we can try to recover from this situation by forced unmounting of all outstanding `ShadowTree` (that will make future destroying of EventEmitter don't crash).

Why do we believe that `Scheduler` *was* deallocated before `Runtime`?
We are not sure but that's possibly a case or that might be true for some portion of cases.
At least, we will know for sure what exactly went wrong. There is a possibility that all crashes that would happen after will happen exactly here.
And, frankly, from the probability standpoint, earlier we destroy JSI objects, lower chances are that it will be after the death of Runtime.

Changelog: [Internal] Fabric-specific internal change.

Reviewed By: sammy-SC

Differential Revision: D18465408

fbshipit-source-id: 76338ed6fce0621cf11ce3178b4d1be7e0c73ccf
  • Loading branch information
shergin authored and facebook-github-bot committed Nov 17, 2019
1 parent 59e8bd9 commit db4a7ae
Showing 1 changed file with 32 additions and 15 deletions.
47 changes: 32 additions & 15 deletions ReactCommon/fabric/uimanager/Scheduler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -85,27 +85,44 @@ Scheduler::Scheduler(
}

Scheduler::~Scheduler() {
#ifndef NDEBUG
/*
* This requirement must be satisfied to make concurrent deallocation of
* `Scheduler` and calling on it from `UIManager` side thread-safe.
* Conceptually, `UIManager` is allowed to call `Scheduler` only if the
* corresponding `ShadowTree` instance exists (which is not fully enforced
* yet).
*/
auto hasRunningSurfaces = false;
// All Surfaces must be explicitly stopped before destroying `Scheduler`.
// The idea is that `UIManager` is allowed to call `Scheduler` only if the
// corresponding `ShadowTree` instance exists.

// The thread-safety of this operation is guaranteed by this requirement.
uiManager_->setDelegate(nullptr);

// Then, let's verify that the requirement was satisfied.
auto surfaceIds = std::vector<SurfaceId>{};
uiManager_->getShadowTreeRegistry().enumerate(
[&](ShadowTree const &shadowTree, bool &stop) {
stop = true;
hasRunningSurfaces = true;
surfaceIds.push_back(shadowTree.getSurfaceId());
});

assert(
!hasRunningSurfaces &&
"Scheduler was destroyed with sill running Surfaces.");
#endif
surfaceIds.size() == 0 &&
"Scheduler was destroyed with outstanding Surfaces.");

uiManager_->setDelegate(nullptr);
if (surfaceIds.size() == 0) {
return;
}

LOG(FATAL) << "Scheduler was destroyed with outstanding Surfaces.";

// If we are here, that means assert didn't fire which indicates that we in
// production.

// Now we have still-running surfaces, which is no good, no good.
// That's indeed a sign of a severe issue on the application layer.
// At this point, we don't have much to lose, so we are trying to unmount all
// outstanding `ShadowTree`s to prevent all stored JSI entities from
// overliving the `Scheduler`. (Unmounting `ShadowNode`s disables
// `EventEmitter`s which destroys JSI objects.)
for (auto surfaceId : surfaceIds) {
uiManager_->getShadowTreeRegistry().visit(
surfaceId,
[](ShadowTree const &shadowTree) { shadowTree.commitEmptyTree(); });
}
}

void Scheduler::startSurface(
Expand Down

0 comments on commit db4a7ae

Please sign in to comment.