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.

Sign up for GitHub

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 app crash caused by the request operation canceling #48350

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: iOS app crash caused by the request operation canceling #48350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7181c31
fix: iOS app crash caused by canceling request operations
Dec 20, 2024
dc302f3
refactor: use the `RCTExecuteOnMainQueue` instead of its unsafe variant
Dec 20, 2024
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
10 changes: 8 additions & 2 deletions packages/react-native/Libraries/Network/RCTFileRequestHandler.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,14 @@ @implementation RCTFileRequestHandler {

- (void)invalidate
{
[_fileQueue cancelAllOperations];
_fileQueue = nil;
if (_fileQueue) {
for (NSOperation *operation in _fileQueue.operations) {
if ([operation isKindOfClass:[NSOperation class]] && !operation.isCancelled && !operation.isFinished) {
[operation cancel];
}
}
_fileQueue = nil;
}
Comment on lines +28 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not necessary. You can only add NSOperation to an NSOperationQueue and cancellAllOperations runs the same code you are manually writing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cipolleschi Seems that the cancellAllOperations won't do the status checks for the operation, and in the crash report of my iOS app, the stack trace exactly tells me the crash point is just in the cancellAllOperations internal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the official Apple docs for cancelAllOperations.

This method calls the cancel method on all operations currently in the queue.
Canceling the operations does not automatically remove them from the queue or stop those that are currently executing. For operations that are queued and waiting execution, the queue must still attempt to execute the operation before recognizing that it is canceled and moving it to the finished state. For operations that are already executing, the operation object itself must check for cancellation and stop what it is doing so that it can move to the finished state. In both cases, a finished (or canceled) operation is still given a chance to execute its completion block before it is removed from the queue.

And this is the docs of NSOperation cancel.

In any case, calling cancel on an already cancelled or finished operation does not crash the app.

I believe that the crash is happening inside one of the operations that are being cancelled and that's why the crash reporter reports the crash there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that even though we already put assurance for the corresponding code to make it only executed on the main thread or another sole thread, e.g. the JS thread, but because of the nature of object pointer reference, the operation object could still be shared among multiple thread contexts, then the situation you said above happens.

}

- (BOOL)canHandleRequest:(NSURLRequest *)request
Expand Down
130 changes: 66 additions & 64 deletions packages/react-native/React/CxxBridge/RCTCxxBridge.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,86 +1189,88 @@ - (void)dispatchBlock:(dispatch_block_t)block queue:(dispatch_queue_t)queue

- (void)invalidate
{
if (_didInvalidate) {
return;
}
RCTUnsafeExecuteOnMainQueueSync(^{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could potentially deadlock. We should not run the unsafe variant of this method. Can you change it with RCTExecuteOnMainQueue?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

if (_didInvalidate) {
return;
}

RCTAssertMainQueue();
RCTLogInfo(@"Invalidating %@ (parent: %@, executor: %@)", self, _parentBridge, [self executorClass]);
RCTAssertMainQueue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that confuses me is that, in your stacktrace, the crash is happening in Thread 26... but this assert should force the app to be on the main thread, which is not the Thread 26... how's this possible?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cipolleschi Good question, that's because of actually the RCTAssertMainQueue only takes effects in dev build, when in the release build, it does nothing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I added the RCTUnsafeExecuteOnMainQueueSync wrapper to ensure the code to run on the main thread.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image As you can see, when the `NS_BLOCK_ASSERTIONS` is defined, the `RCTAssert` macro is actually empty, and generally the `NS_BLOCK_ASSERTIONS` is defined in release build.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good explanation, but then we should see crashes in development happening because of the assertion. And IIUC, the app does not crash in development, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By looking at the crash log, the JS thread is triggering the invalidation. I think that this is the root of the problem: after the JS thread detect the invalidation, we should jump on the UI thread to invalidate everything...

Copy link
Author

@zhouzh1 zhouzh1 Dec 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't encounter this crash in development, but I am not sure if it happens in it, you know, it's a occasional issue on itself.

RCTLogInfo(@"Invalidating %@ (parent: %@, executor: %@)", self, _parentBridge, [self executorClass]);

if (self->_reactInstance) {
// Do this synchronously on the main thread to fulfil unregisterFromInspector's
// requirements.
self->_reactInstance->unregisterFromInspector();
}
if (self->_reactInstance) {
// Do this synchronously on the main thread to fulfil unregisterFromInspector's
// requirements.
self->_reactInstance->unregisterFromInspector();
}

_loading = NO;
_valid = NO;
_didInvalidate = YES;
_moduleRegistryCreated = NO;
_loading = NO;
_valid = NO;
_didInvalidate = YES;
_moduleRegistryCreated = NO;

if ([RCTBridge currentBridge] == self) {
[RCTBridge setCurrentBridge:nil];
}
if ([RCTBridge currentBridge] == self) {
[RCTBridge setCurrentBridge:nil];
}

// Stop JS instance and message thread
[self ensureOnJavaScriptThread:^{
[self->_displayLink invalidate];
self->_displayLink = nil;
// Stop JS instance and message thread
[self ensureOnJavaScriptThread:^{
[self->_displayLink invalidate];
self->_displayLink = nil;

if (RCTProfileIsProfiling()) {
RCTProfileUnhookModules(self);
}
if (RCTProfileIsProfiling()) {
RCTProfileUnhookModules(self);
}

// Invalidate modules
// Invalidate modules

[[NSNotificationCenter defaultCenter] postNotificationName:RCTBridgeWillInvalidateModulesNotification
object:self->_parentBridge
userInfo:@{@"bridge" : self}];
[[NSNotificationCenter defaultCenter] postNotificationName:RCTBridgeWillInvalidateModulesNotification
object:self->_parentBridge
userInfo:@{@"bridge" : self}];

// We're on the JS thread (which we'll be suspending soon), so no new calls will be made to native modules after
// this completes. We must ensure all previous calls were dispatched before deallocating the instance (and module
// wrappers) or we may have invalid pointers still in flight.
dispatch_group_t moduleInvalidation = dispatch_group_create();
for (RCTModuleData *moduleData in self->_moduleDataByID) {
// Be careful when grabbing an instance here, we don't want to instantiate
// any modules just to invalidate them.
if (![moduleData hasInstance]) {
continue;
}
// We're on the JS thread (which we'll be suspending soon), so no new calls will be made to native modules after
// this completes. We must ensure all previous calls were dispatched before deallocating the instance (and module
// wrappers) or we may have invalid pointers still in flight.
dispatch_group_t moduleInvalidation = dispatch_group_create();
for (RCTModuleData *moduleData in self->_moduleDataByID) {
// Be careful when grabbing an instance here, we don't want to instantiate
// any modules just to invalidate them.
if (![moduleData hasInstance]) {
continue;
}

if ([moduleData.instance respondsToSelector:@selector(invalidate)]) {
dispatch_group_enter(moduleInvalidation);
[self
dispatchBlock:^{
[(id<RCTInvalidating>)moduleData.instance invalidate];
dispatch_group_leave(moduleInvalidation);
}
queue:moduleData.methodQueue];
if ([moduleData.instance respondsToSelector:@selector(invalidate)]) {
dispatch_group_enter(moduleInvalidation);
[self
dispatchBlock:^{
[(id<RCTInvalidating>)moduleData.instance invalidate];
dispatch_group_leave(moduleInvalidation);
}
queue:moduleData.methodQueue];
}
[moduleData invalidate];
}
[moduleData invalidate];
}

if (dispatch_group_wait(moduleInvalidation, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC))) {
RCTLogError(@"Timed out waiting for modules to be invalidated");
}
if (dispatch_group_wait(moduleInvalidation, dispatch_time(DISPATCH_TIME_NOW, 10 * NSEC_PER_SEC))) {
RCTLogError(@"Timed out waiting for modules to be invalidated");
}

[[NSNotificationCenter defaultCenter] postNotificationName:RCTBridgeDidInvalidateModulesNotification
object:self->_parentBridge
userInfo:@{@"bridge" : self}];
[[NSNotificationCenter defaultCenter] postNotificationName:RCTBridgeDidInvalidateModulesNotification
object:self->_parentBridge
userInfo:@{@"bridge" : self}];

self->_reactInstance.reset();
self->_jsMessageThread.reset();
self->_reactInstance.reset();
self->_jsMessageThread.reset();

self->_moduleDataByName = nil;
self->_moduleDataByID = nil;
self->_moduleClassesByID = nil;
self->_pendingCalls = nil;
self->_moduleDataByName = nil;
self->_moduleDataByID = nil;
self->_moduleClassesByID = nil;
self->_pendingCalls = nil;

[self->_jsThread cancel];
self->_jsThread = nil;
CFRunLoopStop(CFRunLoopGetCurrent());
}];
[self->_jsThread cancel];
self->_jsThread = nil;
CFRunLoopStop(CFRunLoopGetCurrent());
}];
});
}

- (void)logMessage:(NSString *)message level:(NSString *)level
Expand Down
Loading