From 7638e0285ef5dc236d121b332c863e077db74ccd Mon Sep 17 00:00:00 2001 From: wwwcg Date: Tue, 19 Nov 2024 12:04:59 +0800 Subject: [PATCH] fix(ios): data race issues when bridge and engine dealloc --- framework/ios/base/bridge/HippyBridge.mm | 4 + .../base/executors/HippyJSEnginesMapper.mm | 9 +- renderer/native/ios/renderer/HippyUIManager.h | 6 +- .../native/ios/renderer/HippyUIManager.mm | 106 +++++++++++------- 4 files changed, 81 insertions(+), 44 deletions(-) diff --git a/framework/ios/base/bridge/HippyBridge.mm b/framework/ios/base/bridge/HippyBridge.mm index 18a34b57bef..af83b0a4d70 100644 --- a/framework/ios/base/bridge/HippyBridge.mm +++ b/framework/ios/base/bridge/HippyBridge.mm @@ -271,6 +271,10 @@ - (void)dealloc { if (_rootNode) { _rootNode->ReleaseResources(); } + if (self.uiManager) { + // Prevents multi-threading from accessing weak properties + [self.uiManager setBridge:nil]; + } } - (std::shared_ptr)createURILoaderIfNeeded { diff --git a/framework/ios/base/executors/HippyJSEnginesMapper.mm b/framework/ios/base/executors/HippyJSEnginesMapper.mm index ef6fffaee13..8e3fdd622f1 100644 --- a/framework/ios/base/executors/HippyJSEnginesMapper.mm +++ b/framework/ios/base/executors/HippyJSEnginesMapper.mm @@ -60,7 +60,14 @@ } EngineResource::~EngineResource() { - dom_worker_->Terminate(); + auto runner = engine_->GetJsTaskRunner(); + if (footstone::Worker::IsTaskRunning() && runner == footstone::runner::TaskRunner::GetCurrentTaskRunner()) { + dispatch_async(dispatch_get_main_queue(), ^{ + dom_worker_->Terminate(); + }); + } else { + dom_worker_->Terminate(); + } } using EngineRef = std::pair, NSUInteger>; diff --git a/renderer/native/ios/renderer/HippyUIManager.h b/renderer/native/ios/renderer/HippyUIManager.h index 51d600d9002..519abc4e5a4 100644 --- a/renderer/native/ios/renderer/HippyUIManager.h +++ b/renderer/native/ios/renderer/HippyUIManager.h @@ -77,7 +77,7 @@ HIPPY_EXTERN NSString *const HippyFontChangeTriggerNotification; @interface HippyUIManager : NSObject /// HippyBridge instance -@property (nonatomic, weak, readonly) HippyBridge *bridge; +@property (weak, readonly) HippyBridge *bridge; /// View Registry of all nodes @property (nonatomic, readonly) HippyComponentMap *viewRegistry; @@ -93,6 +93,10 @@ HIPPY_EXTERN NSString *const HippyFontChangeTriggerNotification; - (instancetype)init NS_UNAVAILABLE; + (instancetype)new NS_UNAVAILABLE; +/// Set hippyBridge for UIManager +/// - Parameter bridge: HippyBridge instance +- (void)setBridge:(HippyBridge * _Nullable)bridge; + /// Get all rootView - (NSArray<__kindof UIView *> *)rootViews; diff --git a/renderer/native/ios/renderer/HippyUIManager.mm b/renderer/native/ios/renderer/HippyUIManager.mm index e23a2ab5123..674ac7d0eb1 100644 --- a/renderer/native/ios/renderer/HippyUIManager.mm +++ b/renderer/native/ios/renderer/HippyUIManager.mm @@ -85,13 +85,6 @@ return HippyViewManagerClasses; } -static NSString *viewNameFromViewManagerClass(Class cls) { - HippyAssert([cls respondsToSelector:@selector(moduleName)], - @"%@ must respond to selector moduleName", NSStringFromClass(cls)); - NSString *viewName = [cls performSelector:@selector(moduleName)]; - return viewName; -} - using HPViewBinding = std::map, std::vector>>; constexpr char kVSyncKey[] = "frameupdate"; @@ -186,9 +179,14 @@ - (void)setUiManager:(HippyUIManager *)uiManager; NSString *const HippyFontChangeTriggerNotification = @"HippyFontChangeTriggerNotification"; @interface HippyUIManager() { - NSMutableArray *_pendingUIBlocks; - + // lock for weak bridge access + os_unfair_lock _bridgeLock; + // HippyBridge's weak reference + __weak HippyBridge *_bridge; + + // View Registry HippyComponentMap *_viewRegistry; + // ShadowView Registry HippyComponentMap *_shadowViewRegistry; // lock for componentDataByName @@ -200,7 +198,10 @@ @interface HippyUIManager() { // The implementation here needs to be improved to provide a registration mechanism. NSHashTable> *_componentTransactionListeners; + // Lock for render operations std::mutex _renderQueueLock; + // UIBlocks waiting for executing + NSMutableArray *_pendingUIBlocks; } /// All managed ViewManagers @@ -233,6 +234,7 @@ - (void)initContext { _pendingUIBlocks = [NSMutableArray new]; _componentTransactionListeners = [NSHashTable weakObjectsHashTable]; _componentDataByName = [NSMutableDictionary dictionary]; + _bridgeLock = OS_UNFAIR_LOCK_INIT; _componentDataLock = OS_UNFAIR_LOCK_INIT; HippyScreenScale(); HippyScreenSize(); @@ -260,6 +262,21 @@ - (void)dealloc { #pragma mark Setter & Getter +- (HippyBridge *)bridge { + os_unfair_lock_lock(&_bridgeLock); + HippyBridge *bridge = _bridge; + os_unfair_lock_unlock(&_bridgeLock); + return bridge; +} + +- (void)setBridge:(HippyBridge *)bridge { + // Called when HippyBridge dealloc + // Avoid multi-threading accessing the weak bridge by setting it to nil. + os_unfair_lock_lock(&_bridgeLock); + _bridge = bridge; + os_unfair_lock_unlock(&_bridgeLock); +} + - (void)setDomManager:(std::weak_ptr)domManager { _domManager = domManager; } @@ -411,9 +428,7 @@ - (void)observeValueForKeyPath:(NSString *)keyPath domManager->PostTask(hippy::Scene({func})); HippyBridge *bridge = self.bridge; - if (bridge) { - [bridge sendEvent:@(hippyOnSizeChangedKey) params:params]; - } + [bridge sendEvent:@(hippyOnSizeChangedKey) params:params]; } } } @@ -625,40 +640,40 @@ - (UIView *)createViewRecursiveFromRenderObjectWithNOLock:(HippyShadowView *)sha } -#pragma mark - +#pragma mark - ShadowView Related -- (HippyShadowView *)createRenderObjectFromNode:(const std::shared_ptr &)domNode - onRootNode:(std::weak_ptr)rootNode { +- (HippyShadowView *)createShadowViewFromNode:(const std::shared_ptr &)domNode + onRootNode:(std::weak_ptr)rootNode { auto strongRootNode = rootNode.lock(); if (!strongRootNode || !domNode) { return nil; } - int32_t root_id = strongRootNode->GetId(); - NSNumber *rootTag = @(root_id); + int32_t rootId = strongRootNode->GetId(); + NSNumber *rootTag = @(rootId); NSNumber *componentTag = @(domNode->GetId()); NSString *viewName = [NSString stringWithUTF8String:domNode->GetViewName().c_str()]; NSString *tagName = [NSString stringWithUTF8String:domNode->GetTagName().c_str()]; NSMutableDictionary *props = [StylesFromDomNode(domNode) mutableCopy]; HippyComponentData *componentData = [self componentDataForViewName:viewName]; - HippyShadowView *renderObject = [componentData createShadowViewWithTag:componentTag]; - renderObject.rootNode = rootNode; - NSAssert(componentData && renderObject, @"componentData and renderObject must not be nil"); + HippyShadowView *shadowView = [componentData createShadowViewWithTag:componentTag]; + shadowView.rootNode = rootNode; + NSAssert(componentData && shadowView, @"componentData and renderObject must not be nil"); [props setValue: rootTag forKey: @"rootTag"]; // Register shadow view - if (renderObject) { - renderObject.hippyTag = componentTag; - renderObject.rootTag = rootTag; - renderObject.viewName = viewName; - renderObject.tagName = tagName; - renderObject.props = props; - renderObject.domNode = domNode; - renderObject.rootNode = rootNode; - renderObject.domManager = _domManager; - renderObject.nodeLayoutResult = domNode->GetLayoutResult(); - renderObject.frame = CGRectMakeFromLayoutResult(domNode->GetLayoutResult()); - [componentData setProps:props forShadowView:renderObject]; + if (shadowView) { + shadowView.hippyTag = componentTag; + shadowView.rootTag = rootTag; + shadowView.viewName = viewName; + shadowView.tagName = tagName; + shadowView.props = props; + shadowView.domNode = domNode; + shadowView.rootNode = rootNode; + shadowView.domManager = _domManager; + shadowView.nodeLayoutResult = domNode->GetLayoutResult(); + shadowView.frame = CGRectMakeFromLayoutResult(domNode->GetLayoutResult()); + [componentData setProps:props forShadowView:shadowView]; } - return renderObject; + return shadowView; } - (void)updateView:(nonnull NSNumber *)componentTag @@ -679,12 +694,14 @@ - (void)updateView:(nonnull NSNumber *)componentTag } - (__kindof HippyViewManager *)viewManagerForViewName:(NSString *)viewName { - HippyBridge *strongBridge = self.bridge; if (!self.viewManagers) { NSMutableDictionary *viewManagers = [NSMutableDictionary dictionary]; + // First, read all extra components. if (self.extraComponents) { for (Class cls in self.extraComponents) { - NSString *viewName = viewNameFromViewManagerClass(cls); + HippyAssert([cls respondsToSelector:@selector(moduleName)], + @"%@ must respond to selector moduleName", NSStringFromClass(cls)); + NSString *viewName = [cls performSelector:@selector(moduleName)]; HippyAssert(![viewManagers objectForKey:viewName], @"duplicated component %@ for class %@ and %@", viewName, NSStringFromClass(cls), @@ -692,24 +709,32 @@ - (__kindof HippyViewManager *)viewManagerForViewName:(NSString *)viewName { [viewManagers setObject:cls forKey:viewName]; } } + + // Second, read the default view manager classes. + HippyBridge *strongBridge = self.bridge; NSArray *classes = HippyGetViewManagerClasses(strongBridge); NSMutableDictionary *defaultViewManagerClasses = [NSMutableDictionary dictionaryWithCapacity:[classes count]]; for (Class cls in classes) { - NSString *viewName = viewNameFromViewManagerClass(cls); + HippyAssert([cls respondsToSelector:@selector(moduleName)], + @"%@ must respond to selector moduleName", NSStringFromClass(cls)); + NSString *viewName = [cls performSelector:@selector(moduleName)]; if ([viewManagers objectForKey:viewName]) { continue; } [defaultViewManagerClasses setObject:cls forKey:viewName]; } + [viewManagers addEntriesFromDictionary:defaultViewManagerClasses]; self.viewManagers = viewManagers; } + // Get and instantiate the class id object = [self.viewManagers objectForKey:viewName]; if (object_isClass(object)) { HippyViewManager *viewManager = [object new]; + HippyBridge *strongBridge = self.bridge; viewManager.bridge = strongBridge; - NSAssert([viewManager isKindOfClass:[HippyViewManager class]], @"Must be a HippyViewManager instance"); + HippyAssert([viewManager isKindOfClass:[HippyViewManager class]], @"Must be a HippyViewManager instance"); [self.viewManagers setObject:viewManager forKey:viewName]; object = viewManager; } @@ -794,7 +819,7 @@ - (void)createRenderNodes:(std::vector> &&)nodes for (const std::shared_ptr &node : nodes) { const auto& render_info = node->GetRenderInfo(); [manager addViewTag:render_info.id forSuperViewTag:render_info.pid atIndex:render_info.index]; - HippyShadowView *shadowView = [self createRenderObjectFromNode:node onRootNode:rootNode]; + HippyShadowView *shadowView = [self createShadowViewFromNode:node onRootNode:rootNode]; [_shadowViewRegistry addComponent:shadowView forRootTag:shadowView.rootTag]; } [manager enumerateViewsHierarchy:^(int32_t tag, const std::vector &subviewTags, const std::vector &subviewIndices) { @@ -1117,7 +1142,7 @@ - (void)dispatchFunction:(const std::string &)functionName id method = moduleData.methodsByName[name]; if (method) { @try { - [method invokeWithBridge:_bridge module:componentData.manager arguments:finalParams]; + [method invokeWithBridge:self.bridge module:componentData.manager arguments:finalParams]; } @catch (NSException *exception) { NSString *errMsg = [NSString stringWithFormat:@"Exception '%@' was thrown while invoking %@ on component %@ with params %@", exception, name, nativeModuleName, finalParams]; @@ -1183,9 +1208,6 @@ - (void)addEventName:(const std::string &)name [[RenderVsyncManager sharedInstance] registerVsyncObserver:^{ __strong __typeof(weakSelf)strongSelf = weakSelf; HippyBridge *bridge = strongSelf.bridge; - if (!bridge) { - return; - } [bridge.javaScriptExecutor executeAsyncBlockOnJavaScriptQueue:^{ auto strongNode = weakNode.lock(); if (strongNode) {