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

[Config] Mark two RCNConfigSettings properties atomic #13925

Merged
merged 3 commits into from
Oct 18, 2024
Merged

[Config] Mark two RCNConfigSettings properties atomic #13925

merged 3 commits into from
Oct 18, 2024

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Oct 17, 2024

The crashing frame is on objc_release_x0 so I'm thinking that that the instance is being over-released.

Thread 24 Queue 295: com.google.GoogleConfigService.FIRRemoteConfig (serial) [Crashed]:

0    libobjc.A.dylib                          0x1897da370     objc_release_x0 + 16
1    CoreFoundation                           0x18c495394     __CFBasicHashReplaceValue + 415
2    CoreFoundation                           0x18c493d04     CFDictionarySetValue + 207
3    CFNetwork                                0x18d9409bc     HTTPHeaderDict::setValue(HTTPHeaderKeyMixedValue const&, HTTPHeaderValueMixedValue const&) + 143
4    CFNetwork                                0x18d94082c     HTTPMessage::setHeaderFieldStringValue(__CFString const*, __CFString const*) + 99
5    CFNetwork                                0x18d93fbcc     CFURLRequestSetHTTPHeaderFieldValue + 127
6    FlexNetworkingProduction                 0x101729cb4     __53-[RCNConfigRealtime createRequestBodyWithCompletion:]_block_invoke + 183
7    libdispatch.dylib                        0x194164370     _dispatch_call_block_and_release + 31
8    libdispatch.dylib                        0x1941660d0     _dispatch_client_callout + 19
9    libdispatch.dylib                        0x19416d6d8     _dispatch_lane_serial_drain + 743
10   libdispatch.dylib                        0x19416e1e0     _dispatch_lane_invoke + 379
11   libdispatch.dylib                        0x194179258     _dispatch_root_queue_drain_deferred_wlh + 287
12   libdispatch.dylib                        0x194178aa4     _dispatch_workloop_worker_thread + 539
13   libsystem_pthread.dylib                  0x213857c7c     _pthread_wqthread + 287
14   libsystem_pthread.dylib                  0x213854488     start_wqthread + 7

These properties are never sent in RCNConfigSettings's initialization but rather on one of two different queues:

// Dispatch to the RC serial queue to update settings on the queue.
dispatch_async(strongSelf->_lockQueue, ^{
RCNConfigFetch *strongSelfQueue = weakSelf;
if (strongSelfQueue == nil) {
return;
}
// Update config settings with the IID and token.
strongSelfQueue->_settings.configInstallationsToken = tokenResult.authToken;
strongSelfQueue->_settings.configInstallationsIdentifier = identifier;

// Dispatch to the RC serial queue to update settings on the queue.
dispatch_async(strongSelf->_realtimeLockQueue, ^{
RCNConfigRealtime *strongSelfQueue = weakSelf;
if (strongSelfQueue == nil) {
return;
}
/// Update config settings with the IID and token.
strongSelfQueue->_settings.configInstallationsToken = tokenResult.authToken;
strongSelfQueue->_settings.configInstallationsIdentifier = identifier;

Screenshot 2024-10-17 at 4 19 08 PM Screenshot 2024-10-17 at 4 19 25 PM

They are currently marked nonatomic, so my hypothesis is that concurrent sets lead to an object with an invalid retain count, and when the property is later accessed here. It causes a crash when trying to over-release the invalid instance.

I did some searching and atomic seemed to be an acceptable solution. Others were to add a queue inside RCNConfigSettings which seemed too heavyweight, as well as overwriting the getter and setter to use @synchronized which seemed reasonable but adding atomic was less LOC.

Fix #13898

@ncooke3 ncooke3 changed the title [Config] Mark two 'RCNConfigSettings' properties atomic [Config] Mark two RCNConfigSettings properties atomic Oct 17, 2024
@ncooke3 ncooke3 added this to the 11.5.0 - M156 milestone Oct 17, 2024
@ncooke3 ncooke3 merged commit 9f2f41f into main Oct 18, 2024
43 checks passed
@ncooke3 ncooke3 deleted the nc/13898 branch October 18, 2024 21:24
@firebase firebase locked and limited conversation to collaborators Nov 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RemoteConfig crashes on __53-[RCNConfigRealtime createRequestBodyWithCompletion:]_block_invoke
4 participants