Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ios): data race issues when bridge and engine dealloc #4126

Merged
merged 1 commit into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions framework/ios/base/bridge/HippyBridge.mm
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
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
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ HIPPY_EXTERN NSString *const HippyFontChangeTriggerNotification;
@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 @@ -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;

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

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

@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 @@ -200,7 +198,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 @@ -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();
Expand Down Expand Up @@ -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 = domManager;
}
Expand Down Expand Up @@ -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];
}
}
}
Expand Down Expand Up @@ -625,40 +640,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 @@ -679,37 +694,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 @@ -794,7 +819,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 @@ -1117,7 +1142,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 @@ -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) {
Expand Down
Loading