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): potential thread race in Vsync manager #4185

Merged
merged 1 commit into from
Feb 13, 2025
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
8 changes: 4 additions & 4 deletions renderer/native/ios/renderer/HippyUIManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
#import "HippyRenderUtils.h"
#import "HippyView.h"
#import "HippyViewManager.h"
#import "RenderVsyncManager.h"
#import "HippyVsyncManager.h"
#import "UIView+DomEvent.h"
#import "UIView+Hippy.h"
#import "HippyBridgeModule.h"
Expand Down Expand Up @@ -1203,7 +1203,7 @@ - (void)addEventName:(const std::string &)name
NSString *vsyncKey = [NSString stringWithFormat:@"%p-%d", strongSelf, node_id];
auto event = std::make_shared<hippy::DomEvent>(name_, node);
std::weak_ptr<DomNode> weakNode = node;
[[RenderVsyncManager sharedInstance] registerVsyncObserver:^{
[[HippyVsyncManager sharedInstance] registerVsyncObserver:^{
__strong __typeof(weakSelf)strongSelf = weakSelf;
HippyBridge *bridge = strongSelf.bridge;
[bridge.javaScriptExecutor executeAsyncBlockOnJavaScriptQueue:^{
Expand Down Expand Up @@ -1407,7 +1407,7 @@ - (void)removeEventName:(const std::string &)eventName
if (node) {
//for kVSyncKey event, node is rootnode
NSString *vsyncKey = [NSString stringWithFormat:@"%p-%d", self, node_id];
[[RenderVsyncManager sharedInstance] unregisterVsyncObserverForKey:vsyncKey];
[[HippyVsyncManager sharedInstance] unregisterVsyncObserverForKey:vsyncKey];
}
}];
} else {
Expand All @@ -1421,7 +1421,7 @@ - (void)removeEventName:(const std::string &)eventName

- (void)removeVSyncEventOnRootNode:(std::weak_ptr<hippy::RootNode>)rootNode {
NSString *vsyncKey = [NSString stringWithFormat:@"%p-%d", self, static_cast<int>(rootNode.lock()->GetId())];
[[RenderVsyncManager sharedInstance] unregisterVsyncObserverForKey:vsyncKey];
[[HippyVsyncManager sharedInstance] unregisterVsyncObserverForKey:vsyncKey];
}

- (void)addPropertyEvent:(const std::string &)name
Expand Down
88 changes: 88 additions & 0 deletions renderer/native/ios/renderer/HippyVsyncManager.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
/*!
* iOS SDK
*
* Tencent is pleased to support the open source community by making
* Hippy available.
*
* Copyright (C) 2019 THL A29 Limited, a Tencent company.
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#import <Foundation/Foundation.h>

NS_ASSUME_NONNULL_BEGIN

/**
A manager for coordinating V-sync synchronized callbacks.
Provides thread-safe registration of display link observers with configurable refresh rates.
*/
@interface HippyVsyncManager : NSObject

///-------------------
/// @name Singleton
///-------------------

/**
Returns the shared vsync manager instance.

@discussion Use this singleton instance to coordinate vsync observers across the application.
*/
+ (instancetype)sharedInstance;

///-------------------
/// @name Registration
///-------------------

/**
Registers a vsync observer with default 60Hz refresh rate.

@param observer The block to execute on each vsync callback
@param key A unique identifier for the observer

@discussion Re-registering with the same key will replace the existing observer.
*/
- (void)registerVsyncObserver:(dispatch_block_t)observer forKey:(NSString *)key;

/**
Registers a vsync observer with custom refresh rate.

@param observer The block to execute on each vsync callback
@param rate The desired refresh rate in Hz (1-120)
@param key A unique identifier for the observer

@discussion On iOS versions prior to 15, rates above 60Hz will be capped to 60Hz.
*/
- (void)registerVsyncObserver:(dispatch_block_t)observer rate:(float)rate forKey:(NSString *)key;

/**
Unregisters and invalidates the vsync observer for the specified key.

@param key The identifier used during registration

@discussion Safe to call even if no observer exists for the key.
*/
- (void)unregisterVsyncObserverForKey:(NSString *)key;

///--------------------
/// @name Restrictions
///--------------------

- (instancetype)init NS_UNAVAILABLE;
+ (instancetype)new NS_UNAVAILABLE;

@end

NS_ASSUME_NONNULL_END

151 changes: 151 additions & 0 deletions renderer/native/ios/renderer/HippyVsyncManager.m
Original file line number Diff line number Diff line change
@@ -0,0 +1,151 @@
/*!
* iOS SDK
*
* Tencent is pleased to support the open source community by making
* Hippy available.
*
* Copyright (C) 2019 THL A29 Limited, a Tencent company.
* All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#import <QuartzCore/CADisplayLink.h>
#import "HippyVsyncManager.h"
#import "HippyAssert.h"
#import <objc/runtime.h>

// MARK: - CADisplayLink Category for Vsync Handling
@interface CADisplayLink (Vsync)

/// Associated block to be executed on vsync
@property(nonatomic, copy) dispatch_block_t block;

/// Applies refresh rate with validation
/// @param rate Target refresh rate (1-120 Hz)
- (void)applyRefreshRate:(float)rate;

@end

@implementation CADisplayLink (Vsync)

- (void)setBlock:(dispatch_block_t)block {
// Use COPY association to maintain block ownership
objc_setAssociatedObject(self, @selector(block), block, OBJC_ASSOCIATION_COPY);
}

- (dispatch_block_t)block {
return objc_getAssociatedObject(self, _cmd);
}

- (void)applyRefreshRate:(float)rate {
// Validate refresh rate boundaries
HippyAssert(rate >= 1 && rate <= 120, @"VSync refresh rate must be between 1 and 120 Hz");

if (@available(iOS 15.0, *)) {
CAFrameRateRange rateRange = CAFrameRateRangeMake(rate, rate, rate);
self.preferredFrameRateRange = rateRange;
} else {
// Cap to 60 FPS for devices below iOS 15
self.preferredFramesPerSecond = MIN(rate, 60);
}
}

@end

// MARK: - Vsync Manager Implementation
@interface HippyVsyncManager () {
NSMutableDictionary<NSString *, CADisplayLink *> *_observers;
dispatch_semaphore_t _semaphore;
}

@end

@implementation HippyVsyncManager

// MARK: - Singleton Pattern
+ (instancetype)sharedInstance {
static dispatch_once_t onceToken;
static HippyVsyncManager *instance;
dispatch_once(&onceToken, ^{
instance = [[HippyVsyncManager alloc] init];
});
return instance;
}

- (instancetype)init {
self = [super init];
if (self) {
_observers = [NSMutableDictionary dictionary];
_semaphore = dispatch_semaphore_create(1);
}
return self;
}

// MARK: - Vsync Callback Handling
- (void)vsyncSignalInvoked:(CADisplayLink *)displayLink {
// Execute block if exists
dispatch_block_t block = displayLink.block;
if (block) {
block();
}
}

// MARK: - Public Interface
- (void)registerVsyncObserver:(dispatch_block_t)observer forKey:(NSString *)key {
// Default to 60Hz refresh rate
[self registerVsyncObserver:observer rate:60.0f forKey:key];
}

- (void)registerVsyncObserver:(dispatch_block_t)observer rate:(float)rate forKey:(NSString *)key {
if (!observer || !key) {
HippyAssert(NO, @"Invalid parameters for observer registration");
return;
}

// Remove existing observer for key
[self unregisterVsyncObserverForKey:key];

// Create and configure display link
CADisplayLink *vsync = [CADisplayLink displayLinkWithTarget:self selector:@selector(vsyncSignalInvoked:)];
[vsync applyRefreshRate:rate];
[vsync addToRunLoop:[NSRunLoop mainRunLoop] forMode:NSRunLoopCommonModes];
vsync.block = observer;

// Thread-safe dictionary update
dispatch_semaphore_wait(_semaphore, DISPATCH_TIME_FOREVER);
_observers[key] = vsync;
dispatch_semaphore_signal(_semaphore);
}

- (void)unregisterVsyncObserverForKey:(NSString *)key {
if (!key) {
HippyAssert(NO, @"Attempted to unregister with nil key");
return;
}

CADisplayLink *vsync = nil;

// Thread-safe dictionary access
dispatch_semaphore_wait(_semaphore, DISPATCH_TIME_FOREVER);
vsync = _observers[key];
if (vsync) {
[_observers removeObjectForKey:key];
}
dispatch_semaphore_signal(_semaphore);

// Invalidate outside lock to prevent deadlocks
[vsync invalidate];
}

@end
2 changes: 1 addition & 1 deletion renderer/native/ios/renderer/NativeRenderManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
#import "HippyUIManager+Private.h"
#import "NativeRenderManager.h"
#import "HippyShadowText.h"
#import "RenderVsyncManager.h"
#import "HippyVsyncManager.h"
#import "HippyAssert.h"
#include "dom/dom_manager.h"
#include "dom/layout_node.h"
Expand Down
35 changes: 0 additions & 35 deletions renderer/native/ios/renderer/RenderVsyncManager.h

This file was deleted.

Loading
Loading