Attachments added during onCrashedLastRun aren't transmitted

[mrRay]

Platform

macOS

Installed

Manually

Version

8.0.0

Steps to Reproduce

Our app uploads additional contextual crash data to Sentry as an attachment (the size and nature of the data makes it inappropriate to transmit either as structured contextual data or breadcrumbs). Historically, we've accomplished this via the 'onCrashedLastRun' handler to generate the attachment and attach it to the current scope, which looks something like this:

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification {
    NSDictionary * infoDict = [[NSBundle mainBundle] infoDictionary];
    NSString * vers = [infoDict objectForKey:@"CFBundleVersion"];
    
    [SentrySDK startWithConfigureOptions:^(SentryOptions *opts) {
        //  configure some basic options
        opts.dsn = @"YOUR_DSN_GOES_HERE";
        opts.releaseName = vers;
        opts.debug = YES;
        
        //  on relaunching after a crash, we want to collect some info about the crash and then attach it to the 
        //  crash log somehow. this data is generated on demand, and will not exist until we determine that a 
        //  crash has occurred and create it for upload.
        opts.onCrashedLastRun = ^(SentryEvent *crashEvent)  {
            //  assemble ancillary info describing the crash (string with the date of the event in this example)
            NSString * logFileString = NSUUID.UUID.UUIDString;
            NSData * logFileData = [logFileString dataUsingEncoding:NSUTF8StringEncoding];
            NSString * logFileName = [logFileString stringByAppendingPathExtension:@"txt"];
            //  create an attachment containing the ancillary information
            SentryAttachment * attachment = [[SentryAttachment alloc] initWithData:logFileData filename:logFileName contentType:@"text/plain"];
            
            //  add the attachment to the current scope (which, at this time, should be the scope that was active when the "onCrashedLastRUn" handler was invoked, correct?)
            [SentrySDK configureScope:^(SentryScope * scope)    {
                [scope addAttachment:attachment];
            }];
            
            //  we only want the attachment to be associated with the crash log- not with subsequent events- so clear the attachment when we're done!
            dispatch_async(dispatch_get_main_queue(), ^{
                [SentrySDK configureScope:^(SentryScope * scope)    {
                    [scope clearAttachments];
                }];
            });
        };
    }];
}

...at some point, this stopped working. Specifically, it looks like the -[SentryCrashReportSink handleConvertedEvent: report: sentReports:] method makes a copy of the current hub's scope, and uses that copy to handle the crash event. Presumably, this is why attempting to attach objects to the hub's current scope inside the 'onCrashedLastRun' handler is no longer effective- the attachments are being attached to the hub's current scope, but the hub's current scope is no longer being used to send the crash data, I think?

Possible Fix
The following patch will allow attachments made to the curent scope during the "onCrashedLastRun" handler to be submitted with the crash log- it doesn't cause any of the built-in tests to fail, but I'm largely unfamiliar with this codebase and am unsure as to whether or not there are any other regressions it may trip or design philosophies it may run counter to.

diff --git a/Sources/Sentry/SentryCrashReportSink.m b/Sources/Sentry/SentryCrashReportSink.m
index c40e350b..92627239 100644
--- a/Sources/Sentry/SentryCrashReportSink.m
+++ b/Sources/Sentry/SentryCrashReportSink.m
@@ -90,7 +90,7 @@ SentryCrashReportSink ()
                  sentReports:(NSMutableArray *)sentReports
 {
     [sentReports addObject:report];
-    SentryScope *scope = [[SentryScope alloc] initWithScope:SentrySDK.currentHub.scope];
+    SentryScope *scope = SentrySDK.currentHub.scope;
 
     if (report[SENTRYCRASH_REPORT_ATTACHMENTS_ITEM]) {
         for (NSString *ssPath in report[SENTRYCRASH_REPORT_ATTACHMENTS_ITEM]) {

Expected Result

I expected that the objects I attached to the current scope in the "onCrashedLastRun" handler would be applied to the crash log's scope.

Actual Result

The attachments added in the "onCrashedLastRun" handler are not transmitted with the crash log!

Are you willing to submit a PR?

Sure

[philipphofmann]

I'm surprised that your workaround worked at some point. If our SDK would support hints #2325, you could add your attachments in beforeSend, but we don't support hints yet.

I think calling flush with a bit of a delay before you remove the attachment could work for you. Currently, the client most likely isn't fast enough to copy the attachments from the scope before you clear the attachments. But also, this solution is hacky and not guaranteed to work all the time.

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification {
    NSDictionary * infoDict = [[NSBundle mainBundle] infoDictionary];
    NSString * vers = [infoDict objectForKey:@"CFBundleVersion"];
    
    [SentrySDK startWithConfigureOptions:^(SentryOptions *opts) {
        //  configure some basic options
        opts.dsn = @"YOUR_DSN_GOES_HERE";
        opts.releaseName = vers;
        opts.debug = YES;
        
        //  on relaunching after a crash, we want to collect some info about the crash and then attach it to the 
        //  crash log somehow. this data is generated on demand, and will not exist until we determine that a 
        //  crash has occurred and create it for upload.
        opts.onCrashedLastRun = ^(SentryEvent *crashEvent)  {
            //  assemble ancillary info describing the crash (string with the date of the event in this example)
            NSString * logFileString = NSUUID.UUID.UUIDString;
            NSData * logFileData = [logFileString dataUsingEncoding:NSUTF8StringEncoding];
            NSString * logFileName = [logFileString stringByAppendingPathExtension:@"txt"];
            //  create an attachment containing the ancillary information
            SentryAttachment * attachment = [[SentryAttachment alloc] initWithData:logFileData filename:logFileName contentType:@"text/plain"];
            
            //  add the attachment to the current scope (which, at this time, should be the scope that was active when the "onCrashedLastRUn" handler was invoked, correct?)
            [SentrySDK configureScope:^(SentryScope * scope)    {
                [scope addAttachment:attachment];
            }];
            

            //  we only want the attachment to be associated with the crash log- not with subsequent events- so clear the attachment when we're done!
            dispatch_after(small_delay ,dispatch_get_main_queue(), ^{
                // Please choose a proper value
                [SentrySDK flush:5.0];
                [SentrySDK configureScope:^(SentryScope * scope)    {
                    [scope clearAttachments];
                }];
            });
        };
    }];
}

[mrRay]

"Currently, the client most likely isn't fast enough to copy the attachments from the scope before you clear the attachments."

That doesn't appear to be the case- if you remove the call to "clearAttachments" entirely, the attachments are still dropped.

The patch I posted fixes this issue- is there a reason it's not acceptable, or would you like me to file a PR with it?

[philipphofmann]

No, the patch doesn't work cause we need a copy from the scope as we are adding other attachments to it, which we don't want to end up on the global scope. Sadly, we only have a proper way to achieve your use case once we implement #2325.

If you are not using view hierarchy or screenshots, you could keep a fork with the patch until we implement hints. It should be easy to maintain, as it's only one line of code, and we are not touching that part of the code often.

[mrRay]

Awesome, thanks very much for taking a look at it and letting me know where the potential pitfalls are!

[hubertdeng123]

Removing this from Untriaged and onto the Backlog to avoid it getting tracked for triage SLO's since it looks like it will not be immediately implemented

[brustolin]

When we add support for hints(#2325), this will be solved.
Closing this as duplicate.

[m1entus]

@philipphofmann any ETA on implementing hints ?

[philipphofmann]

@m1entus, no not yet sorry.