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

Getting crashes on DataTransferObject since updating #245

Closed
mfernandez94 opened this issue Feb 6, 2020 · 25 comments
Closed

Getting crashes on DataTransferObject since updating #245

mfernandez94 opened this issue Feb 6, 2020 · 25 comments
Assignees

Comments

@mfernandez94
Copy link

mfernandez94 commented Feb 6, 2020

Started to see a rise in crashes for users since updating, stack trace seems to show its an issue in

[DataTransferObject(Protected) initWithDictionary:] + 41

Screen Shot 2020-02-06 at 2 31 59 PM

and

-[DataTransferObject initWithDictionary:] + 335

Screen Shot 2020-02-06 at 3 26 01 PM

side note I noticed we were getting crashes when sending a URL type in the data dictionary, so transformed them into strings, but still getting a good amount of crashes. Could it be possible other invalid JSON is getting through? Insights say heap corruption but not sure if that's the case.

@akornich akornich self-assigned this Feb 7, 2020
@akornich akornich added the bug label Feb 7, 2020
@akornich
Copy link
Contributor

akornich commented Feb 7, 2020

@mfernandez94 , are you using any custom settings for the server data in your RollbarConfig object? If yes, can you post a code snippet (of what and how you set it) here?

@mfernandez94
Copy link
Author

mfernandez94 commented Feb 8, 2020

@akornich I don't believe so, but we are setting this on init of rollbar, is this what you mean?

Screen Shot 2020-02-07 at 4 51 35 PM

Screen Shot 2020-02-07 at 4 53 24 PM

@akornich
Copy link
Contributor

akornich commented Feb 8, 2020

@mfernandez94 , yes, that is what i wanted to see. thanks!
it doesn't look like you are attempting to preset any server config parameters.
i'll keep looking deeper.
just a general note to keep in mind when adding custom objects to a payload: we are using NSJSONSerialization class to turn a payload into a JSON. That places some restrictions on how a payload object should look like:

A Foundation object that may be converted to JSON must have the following properties:
The top level object is an NSArray or NSDictionary.
All objects are instances of NSString, NSNumber, NSArray, NSDictionary, or NSNull.
All dictionary keys are instances of NSString.
Numbers are not NaN or infinity.
Other rules may apply. Calling isValidJSONObject: or attempting a conversion are the definitive ways to tell if a given object can be converted to JSON data.

@mfernandez94
Copy link
Author

@akornich Ok, thank you! I added some valid JSON checks, and I'll update if the issue is resolved

@akornich
Copy link
Contributor

@mfernandez94 , do you have any update on the issue? can we close it?

@mfernandez94
Copy link
Author

@akornich So just created a fix for it checking if the data is valid before sending, and seems like we are still getting the crash issue

@akornich
Copy link
Contributor

@mfernandez94 , can you, please, confirm - these crashes happen only on some of the payload instances and not on all, correct?

@mfernandez94
Copy link
Author

@akornich yes only on some, also it seems like they have stopped crashing on the ones with data (for the most part), and according to the crash logs they happen mostly on log ErrorWithMessage without any data attached just the message

@eb-smith-affirm
Copy link

Bump. This causing a lot of crashes for us.

@akornich
Copy link
Contributor

@eb-smith-affirm , what version of the SDK are you using? Also, could you, please, post here the trace stack of the crash?

@okalentiev
Copy link

@akornich I would like to follow up on this since we still experiencing the issue. The crashes are not consistent and happen in a few different places.

The first one is in the DataTransferObject.m - Line 335.

DataTransferObject.m

Crashed: log
SIGSEGV 0x0000000000000020

0  libobjc.A.dylib                0x1afb07c70 objc_retain + 16
1  CoreFoundation                 0x1afd3a404 __NSSingleObjectArrayI_new + 88
2  CoreFoundation                 0x1afcad0ec -[NSDictionary allKeys] + 316
3  Rollbar                        0x105e37430 -[DataTransferObject initWithDictionary:] + 335 (DataTransferObject.m:335)
4  Rollbar                        0x105e83194 -[RollbarConfig server] + 147 (RollbarConfig.m:147)
5  Rollbar                        0x105e8d808 -[RollbarNotifier buildRollbarServer] + 312 (RollbarNotifier.m:312)
6  Rollbar                        0x105e8e6cc -[RollbarNotifier buildRollbarPayloadWithLevel:message:exception:error:extra:crashReport:context:] + 500 (RollbarNotifier.m:500)
7  Rollbar                        0x105e8d160 -[RollbarNotifier log:message:exception:data:context:] + 197 (RollbarNotifier.m:197)
8  Rollbar                        0x105e895fc +[Rollbar log:message:exception:data:context:] + 104 (RollbarFacade.m:104)
9  Rollbar                        0x105e89530 +[Rollbar log:message:exception:data:] + 95 (RollbarFacade.m:95)
10 Rollbar                        0x105e894c0 +[Rollbar log:message:exception:] + 87 (RollbarFacade.m:87)
11 ****                    
12 ****                   
13 libdispatch.dylib              0x1afa76ec4 _dispatch_call_block_and_release + 32
14 libdispatch.dylib              0x1afa7833c _dispatch_client_callout + 20
15 libdispatch.dylib              0x1afa7aaf8 _dispatch_continuation_pop + 408
16 libdispatch.dylib              0x1afa7a258 _dispatch_async_redirect_invoke + 588
17 libdispatch.dylib              0x1afa875c0 _dispatch_root_queue_drain + 348
18 libdispatch.dylib              0x1afa87d9c _dispatch_worker_thread2 + 116
19 libsystem_pthread.dylib        0x1afadf6d8 _pthread_wqthread + 216
20 libsystem_pthread.dylib        0x1afae59c8 start_wqthread + 8

The next two crashes happen when we try to call Rollbar.log(.error, message: message). This method is called from a concurrent DispatchQueue, not sure if that could cause any issues.

RollbarPayload.m – line 45.

RollbarPayload.m

Fatal Exception: NSInvalidArgumentException
*** -[__NSPlaceholderDictionary initWithObjects:forKeys:count:]: attempt to insert nil object from objects[1]

0  CoreFoundation                 0x182560300 __exceptionPreprocess
1  libobjc.A.dylib                0x182274c1c objc_exception_throw
2  CoreFoundation                 0x1825b95a8 -[__NSCFString characterAtIndex:].cold.1
3  CoreFoundation                 0x1825c3520 -[__NSPlaceholderDictionary initWithObjects:forKeys:count:].cold.5
4  CoreFoundation                 0x182447280 -[__NSPlaceholderDictionary initWithObjects:forKeys:count:]
5  CoreFoundation                 0x182438abc +[NSDictionary dictionaryWithObjects:forKeys:count:]
6  Rollbar                        0x10336e000 -[RollbarPayload initWithAccessToken:data:] + 45 (RollbarPayload.m:45)
7  Rollbar                        0x10336a9c0 -[RollbarNotifier buildRollbarPayloadWithLevel:message:exception:error:extra:crashReport:context:] + 521 (RollbarNotifier.m:521)
8  Rollbar                        0x103369160 -[RollbarNotifier log:message:exception:data:context:] + 197 (RollbarNotifier.m:197)
9  Rollbar                        0x1033655fc +[Rollbar log:message:exception:data:context:] + 104 (RollbarFacade.m:104)
10 Rollbar                        0x103365530 +[Rollbar log:message:exception:data:] + 95 (RollbarFacade.m:95)
11 Rollbar                        0x1033654c0 +[Rollbar log:message:exception:] + 87 (RollbarFacade.m:87)
12 ****
13 ****
14 libdispatch.dylib              0x1821feec4 _dispatch_call_block_and_release
15 libdispatch.dylib              0x18220033c _dispatch_client_callout
16 libdispatch.dylib              0x182202af8 _dispatch_continuation_pop
17 libdispatch.dylib              0x182202258 _dispatch_async_redirect_invoke
18 libdispatch.dylib              0x18220f5c0 _dispatch_root_queue_drain
19 libdispatch.dylib              0x18220fd9c _dispatch_worker_thread2
20 libsystem_pthread.dylib        0x1822676d8 _pthread_wqthread
21 libsystem_pthread.dylib        0x18226d9c8 start_wqthread

And another one is in NSJSONSerialization+Rollbar.m line 34.

NSJSONSerialization+Rollbar.m

Fatal Exception: NSInvalidArgumentException
*** +[NSJSONSerialization dataWithJSONObject:options:error:]: value parameter is nil

0  CoreFoundation                 0x1b8274300 __exceptionPreprocess
1  libobjc.A.dylib                0x1b7f88c1c objc_exception_throw
2  Foundation                     0x1b859b920 -[NSISObjectiveLinearExpression addVar:priority:times:processVarNewToReceiver:processVarDroppedFromReceiver:]
3  Rollbar                        0x103edc354 +[NSJSONSerialization(Rollbar) dataWithJSONObject:options:error:safe:] + 34 (NSJSONSerialization+Rollbar.m:34)
4  Rollbar                        0x103eeb6fc -[RollbarNotifier queuePayload_OnlyCallOnThread:] + 645 (RollbarNotifier.m:645)
5  Foundation                     0x1b866cd6c __NSThreadPerformPerform
6  CoreFoundation                 0x1b81efaf4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__
7  CoreFoundation                 0x1b81efa48 __CFRunLoopDoSource0
8  CoreFoundation                 0x1b81ef198 __CFRunLoopDoSources0
9  CoreFoundation                 0x1b81e9f38 __CFRunLoopRun
10 CoreFoundation                 0x1b81e98f4 CFRunLoopRunSpecific
11 Foundation                     0x1b8532b18 -[NSRunLoop(NSRunLoop) runMode:beforeDate:]
12 Rollbar                        0x103ef49f4 -[RollbarThread run] + 65 (RollbarThread.m:65)
13 Foundation                     0x1b866cc10 __NSThread__start__
14 libsystem_pthread.dylib        0x1b7f798fc _pthread_start
15 libsystem_pthread.dylib        0x1b7f819d4 thread_start

Unfortunately, I couldn't reproduce any of them but they all seem related. My guess is that we are passing nil at some stage, but I see no traces of that. Any hints about how to pinpoint this issue would be very helpful.

Our Rollbar version is 1.12.8.

@akornich
Copy link
Contributor

akornich commented Aug 3, 2020

@okalentiev, thanks a lot for the provided traces! While I can not see any obvious cause for the exceptions by looking at the traces briefly, I'll spend some time in a few days experimenting with the code along the paths outlined here...

@akornich
Copy link
Contributor

akornich commented Aug 3, 2020

@okalentiev , btw, could it be that when you make some Rollbar logging calls while providing the custom data dictionary parameter, some of the dictionary values happen to be nils? I don't think we are gating or clearing such values from the custom data dictionary at the moment. Any nil value would break the JSON serialization. You have to either make sure key-values that have nil as a value are all removed from the data dictionary, or use NSNull instead of the nils.
A very similar problem could happen if the RollbarConfiguration's customData dictionary has nil values...

@okalentiev
Copy link

@akornich Thanks for the swift response and clarification! The last two crashes were happening from log calls without a custom data dictionary. But I just checked and in certain cases, we have nil values in RollbarConfiguration's customData🤦‍♂️ So I guess that we found the source of the problem.

Shall we close this one or you want me to get back to you once the fix is released on our side?

@akornich
Copy link
Contributor

akornich commented Aug 5, 2020

@okalentiev , thanks for the update, much appreciated! Let's close it after you confirm that fixing nils in customData resolves all the crashes.
Best regards!

@keagan
Copy link

keagan commented Sep 26, 2020

@akornich I saw this crash in a different app, and have put together a sample app to help you guys reproduce the race condition. Just create a fresh empty app and use the following app delegate code (updating access token accordingly):

import Rollbar
import UIKit

@main
class AppDelegate: UIResponder, UIApplicationDelegate {
    func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]?) -> Bool {

        let rollbarConfig = RollbarConfiguration()
        rollbarConfig.telemetryEnabled = true
        rollbarConfig.captureIp = .full
        Rollbar.initWithAccessToken(ACCESS_TOKEN_HERE, configuration: rollbarConfig)

        for _ in 0..<20 {
            DispatchQueue.global(qos: .utility).async {
                for _ in 0..<20 {
                    Rollbar.error("error", exception: nil, data: nil)
                }
            }
        }

        return true
    }
}

Note that it's a race condition, so may not hit every single time, but it's pretty close for me.

Here's what I see in a debugger:
image

@akornich
Copy link
Contributor

@keagan , thanks a lot for the repro and the screenshot! i think it is never a good idea to keep modifying a collection (dictionary) while iterating through it. i'll plan to fix it on Monday and have a new alpha release right away.

@akornich
Copy link
Contributor

@keagan , i take it back. there is nothing wrong with iterating a dictionary via keys while removing some elements from the dictionary. it looks like something else messes up the memory. i am looking into it...

@akornich
Copy link
Contributor

akornich commented Oct 6, 2020

@keagan, I think I narrowed down to what could be causing crashes in your test scenario. Our current use of RollbarConfig structure within the shared instance of RollbarLogger is not thread-safe at the moment. Until we properly fix it (which is a significant effort), we would recommend the following workaround:
in multithreaded environments, please, use a dedicated RollbarLogger instance per additional execution thread to report payloads.

For example, somewhere closer to the startup of the main application/process thread, initialize the shared instance of Rollbar:

        [Rollbar initWithAccessToken:@"NNNN"];
        Rollbar.currentConfiguration.destination.accessToken = @"NNNN";
        Rollbar.currentConfiguration.destination.environment = @"unit-tests";
        Rollbar.currentConfiguration.developerOptions.transmit = YES;
        Rollbar.currentConfiguration.developerOptions.logPayload = YES;
        Rollbar.currentConfiguration.loggingOptions.maximumReportsPerMinute = 5000;
        Rollbar.currentConfiguration.telemetry.enabled = YES;
        Rollbar.currentConfiguration.loggingOptions.captureIp = RollbarCaptureIpType_Full;
        //etc...

You can use Rollbar's shared logging methods anywhere within that main thread.

However, later for every new thread use a dedicated RollbarLogger instance, configured either fully or partially based on the shared instance configuration, to do the work:

    for( int i = 0; i < 20; i++) {
        dispatch_async(dispatch_get_global_queue(QOS_CLASS_UTILITY,0), ^(){
            RollbarLogger *logger = [[RollbarLogger alloc] initWithAccessToken:Rollbar.currentConfiguration.destination.accessToken];
            for (int j = 0; j < 20; j++) {
                [logger log:RollbarLevel_Error
                    message:@"error"
                       data:nil
                    context:[NSString stringWithFormat:@"%d-%d", i, j]
                 ];
                //DO NOT USE THE SHARED INSTANCE: [Rollbar errorMessage:@"error"];
            }
        });
    }

@keagan
Copy link

keagan commented Oct 6, 2020

got it, @akornich. we'll look into it and follow your guidance, thanks.

@okalentiev
Copy link

@akornich Thanks a lot for the provided steps! Can you please clarify what is the RollbarLogger in this context? I found RollbarNotifier in SDK that does similar things to what you described, but I'm not sure if that's correct.

@akornich
Copy link
Contributor

akornich commented Oct 6, 2020

@okalentiev, sorry for the confusion. You are absolutely correct! I based my code snippet on our v2-alpha codebase of the SDK where RollbarNotifier was renamed into RollbarLogger.

@akornich
Copy link
Contributor

@keagan , i just wanted to check with you if my recommendation worked for you...

@akornich
Copy link
Contributor

closing it. if anyone experiences this type of issue - feel free to reopen it.

@keagan
Copy link

keagan commented Oct 27, 2020

@keagan , i just wanted to check with you if my recommendation worked for you...

@akornich yeah we appear to be good at this point, thanks for the assistance

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants