From cb19621dfe51879407337a1cc19ef27355a451aa Mon Sep 17 00:00:00 2001 From: Kevin Gozali Date: Fri, 15 Jun 2018 10:56:40 -0700 Subject: [PATCH] Implementation of JS reload without crashing Summary: On JS reload the FabricUIManager and EventDispatcher didn't get release due to a retain cycle. This breaks the cycle. In addition, force release the Scheduler on reload so that the stale classes get cleaned up properly, avoiding crashes. Also the surface now remounts the content correctly Reviewed By: shergin Differential Revision: D8414916 fbshipit-source-id: 4b14031f29b3bc9987d7aa765dc0d930a7de2b1e --- React/Fabric/RCTSurfacePresenter.mm | 28 +++++++++++++ React/Fabric/Surface/RCTFabricSurface.mm | 40 +++++++++++++++++++ ReactCommon/fabric/uimanager/Scheduler.cpp | 1 + .../uimanager/SchedulerEventDispatcher.cpp | 2 +- .../uimanager/SchedulerEventDispatcher.h | 5 ++- 5 files changed, 73 insertions(+), 3 deletions(-) diff --git a/React/Fabric/RCTSurfacePresenter.mm b/React/Fabric/RCTSurfacePresenter.mm index 19042989764..9c9c15955bb 100644 --- a/React/Fabric/RCTSurfacePresenter.mm +++ b/React/Fabric/RCTSurfacePresenter.mm @@ -47,6 +47,15 @@ using namespace facebook::react; _surfaceRegistry = [[RCTSurfaceRegistry alloc] init]; _mountingManager = [[RCTMountingManager alloc] init]; _mountingManager.delegate = self; + + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(handleBridgeWillReloadNotification:) + name:RCTBridgeWillReloadNotification + object:_bridge]; + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(handleJavaScriptDidLoadNotification:) + name:RCTJavaScriptDidLoadNotification + object:_bridge]; } return self; @@ -153,6 +162,25 @@ using namespace facebook::react; surface.view.rootView = (RCTSurfaceRootView *)rootComponentView; } +#pragma mark - Bridge events + +- (void)handleBridgeWillReloadNotification:(NSNotification *)notification +{ + // TODO: Define a lifecycle contract for the pieces involved here including the scheduler, mounting manager, and + // the surface registry. For now simply recreate the scheduler on reload. + // The goal is to deallocate the Scheduler and its underlying references before the JS runtime is destroyed. + _scheduler = [[RCTScheduler alloc] init]; + _scheduler.delegate = self; +} + +- (void)handleJavaScriptDidLoadNotification:(NSNotification *)notification +{ + RCTBridge *bridge = notification.userInfo[@"bridge"]; + if (bridge != _batchedBridge) { + _batchedBridge = bridge; + } +} + @end @implementation RCTSurfacePresenter (Deprecated) diff --git a/React/Fabric/Surface/RCTFabricSurface.mm b/React/Fabric/Surface/RCTFabricSurface.mm index 2e06d361d01..267ef61c025 100644 --- a/React/Fabric/Surface/RCTFabricSurface.mm +++ b/React/Fabric/Surface/RCTFabricSurface.mm @@ -74,6 +74,15 @@ _touchHandler = [RCTSurfaceTouchHandler new]; [self _run]; + + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(handleJavaScriptWillStartLoadingNotification:) + name:RCTJavaScriptWillStartLoadingNotification + object:_bridge]; + [[NSNotificationCenter defaultCenter] addObserver:self + selector:@selector(handleJavaScriptDidLoadNotification:) + name:RCTJavaScriptDidLoadNotification + object:_bridge]; } return self; @@ -268,4 +277,35 @@ return NO; } +#pragma mark - Bridge events + +- (void)handleJavaScriptWillStartLoadingNotification:(NSNotification *)notification +{ + // TODO: Move the bridge lifecycle handling up to the RCTSurfacePresenter. + + RCTAssertMainQueue(); + + // Reset states because the bridge is reloading. This is similar to initialization phase. + _stage = RCTSurfaceStageSurfaceDidInitialize; + _view = nil; + _touchHandler = [RCTSurfaceTouchHandler new]; + [self _setStage:RCTSurfaceStageBridgeDidLoad]; +} + +- (void)handleJavaScriptDidLoadNotification:(NSNotification *)notification +{ + // TODO: Move the bridge lifecycle handling up to the RCTSurfacePresenter. + + // Note: this covers only JS reloads. + [self _setStage:RCTSurfaceStageModuleDidLoad]; + [self _run]; + + // After a reload surfacePresenter needs to know the last min/max size for this surface, because the surface hosting + // view was already attached to the ViewController's view. + // TODO: Find a better automatic way. + [_surfacePresenter setMinimumSize:_minimumSize + maximumSize:_maximumSize + surface:self]; +} + @end diff --git a/ReactCommon/fabric/uimanager/Scheduler.cpp b/ReactCommon/fabric/uimanager/Scheduler.cpp index 5a488f4da47..bcb89f0ad8b 100644 --- a/ReactCommon/fabric/uimanager/Scheduler.cpp +++ b/ReactCommon/fabric/uimanager/Scheduler.cpp @@ -35,6 +35,7 @@ Scheduler::Scheduler() { Scheduler::~Scheduler() { uiManager_->setDelegate(nullptr); + eventDispatcher_->setUIManager(nullptr); } void Scheduler::registerRootTag(Tag rootTag) { diff --git a/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.cpp b/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.cpp index 0dce7c6b0c2..11d40c1d32e 100644 --- a/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.cpp +++ b/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.cpp @@ -22,7 +22,7 @@ static std::string normalizeEventType(const std::string &type) { return prefixedType; } -void SchedulerEventDispatcher::setUIManager(std::shared_ptr uiManager) { +void SchedulerEventDispatcher::setUIManager(std::shared_ptr uiManager) const { uiManager_ = uiManager; } diff --git a/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.h b/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.h index 313d3b2ee18..bb6fd12de35 100644 --- a/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.h +++ b/ReactCommon/fabric/uimanager/SchedulerEventDispatcher.h @@ -27,7 +27,7 @@ class SchedulerEventDispatcher final: public: - void setUIManager(std::shared_ptr uiManager); + void setUIManager(std::shared_ptr uiManager) const; #pragma mark - EventDispatcher @@ -42,7 +42,8 @@ public: private: - std::shared_ptr uiManager_; + // TODO: consider using std::weak_ptr<> instead for better memory management. + mutable std::shared_ptr uiManager_; }; } // namespace react