Skip to content

Commit

Permalink
fix(ios): data race issues when bridge and engine dealloc (#4126)
Browse files Browse the repository at this point in the history
  • Loading branch information
@wwwcg
wwwcg committed Nov 19, 2024
1 parent 52b2aa5 commit 957bfda
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 44 deletions.
4 changes: 4 additions & 0 deletions framework/ios/base/bridge/HippyBridge.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<VFSUriLoader>)createURILoaderIfNeeded {
Expand Down
9 changes: 8 additions & 1 deletion framework/ios/base/executors/HippyJSEnginesMapper.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::shared_ptr<EngineResource>, NSUInteger>;
Expand Down
6 changes: 5 additions & 1 deletion renderer/native/ios/renderer/HippyUIManager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ HIPPY_EXTERN NSString *const HippyUIManagerDidEndBatchNotification;
@interface HippyUIManager : NSObject <HippyInvalidating>

/// HippyBridge instance
@property (nonatomic, weak, readonly) HippyBridge *bridge;
@property (weak, readonly) HippyBridge *bridge;

/// View Registry of all nodes
@property (nonatomic, readonly) HippyComponentMap *viewRegistry;
Expand All @@ -84,6 +84,10 @@ HIPPY_EXTERN NSString *const HippyUIManagerDidEndBatchNotification;
- (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;

Expand Down
106 changes: 64 additions & 42 deletions renderer/native/ios/renderer/HippyUIManager.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,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<int32_t, std::tuple<std::vector<int32_t>, std::vector<int32_t>>>;

constexpr char kVSyncKey[] = "frameupdate";
Expand Down Expand Up @@ -183,9 +176,14 @@ - (void)setUiManager:(HippyUIManager *)uiManager;
NSString *const HippyUIManagerDidEndBatchNotification = @"HippyUIManagerDidEndBatchNotification";

@interface HippyUIManager() {
NSMutableArray<HippyViewManagerUIBlock> *_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
Expand All @@ -197,7 +195,10 @@ @interface HippyUIManager() {
// The implementation here needs to be improved to provide a registration mechanism.
NSHashTable<id<HippyComponent>> *_componentTransactionListeners;

// Lock for render operations
std::mutex _renderQueueLock;
// UIBlocks waiting for executing
NSMutableArray<HippyViewManagerUIBlock> *_pendingUIBlocks;
}

/// All managed ViewManagers
Expand Down Expand Up @@ -230,6 +231,7 @@ - (void)initContext {
_pendingUIBlocks = [NSMutableArray new];
_componentTransactionListeners = [NSHashTable weakObjectsHashTable];
_componentDataByName = [NSMutableDictionary dictionary];
_bridgeLock = OS_UNFAIR_LOCK_INIT;
_componentDataLock = OS_UNFAIR_LOCK_INIT;
HippyScreenScale();
HippyScreenSize();
Expand All @@ -253,6 +255,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 = domManager;
}
Expand Down Expand Up @@ -404,9 +421,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];
}
}
}
Expand Down Expand Up @@ -618,40 +633,40 @@ - (UIView *)createViewRecursiveFromRenderObjectWithNOLock:(HippyShadowView *)sha
}


#pragma mark -
#pragma mark - ShadowView Related

- (HippyShadowView *)createRenderObjectFromNode:(const std::shared_ptr<hippy::DomNode> &)domNode
onRootNode:(std::weak_ptr<RootNode>)rootNode {
- (HippyShadowView *)createShadowViewFromNode:(const std::shared_ptr<hippy::DomNode> &)domNode
onRootNode:(std::weak_ptr<RootNode>)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
Expand All @@ -672,37 +687,47 @@ - (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),
NSStringFromClass([viewManagers objectForKey:viewName]));
[viewManagers setObject:cls forKey:viewName];
}
}

// Second, read the default view manager classes.
HippyBridge *strongBridge = self.bridge;
NSArray<Class> *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;
}
Expand Down Expand Up @@ -787,7 +812,7 @@ - (void)createRenderNodes:(std::vector<std::shared_ptr<DomNode>> &&)nodes
for (const std::shared_ptr<DomNode> &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<int32_t> &subviewTags, const std::vector<int32_t> &subviewIndices) {
Expand Down Expand Up @@ -1110,7 +1135,7 @@ - (void)dispatchFunction:(const std::string &)functionName
id<HippyBridgeMethod> 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];
Expand Down Expand Up @@ -1176,9 +1201,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) {
Expand Down

0 comments on commit 957bfda

Please sign in to comment.