google / gtm-session-fetcher

Google Toolbox for Mac - Session Fetcher
Apache License 2.0
238 stars 147 forks source link

Crash in `GTMSessionFetcher stopFetchReleasingCallbacks` #401

Open 0xpablo opened 1 month ago

0xpablo commented 1 month ago

Hi there, we are using gtm-session-fetcher 3.3.2. We noticed a crash in GTMSessionFetcher stopFetchReleasingCallbacks. Here's the stack trace in case it helps. I checked more recent releases but I didn't see anything specific on that stack trace. Wondering if there's any missing nullability annotations needed. If you think this is fixed on a more recent release I'm happy to try updating.

Thanks in advance!

0  Foundation                     0x177afc static URLRequest._unconditionallyBridgeFromObjectiveC(_:) + 264
1  App                      0x39082d0 -[GTMSessionFetcher stopFetchReleasingCallbacks:] + 2151 (GTMSessionFetcher.m:2151)
2  App                      0x3915bd0 -[GTMSessionUploadFetcher stopFetchReleasingCallbacks:] + 1162 (GTMSessionUploadFetcher.m:1162)
3  App                      0x3917ddc -[GTMSessionUploadFetcher stopFetching] + 1865 (GTMSessionUploadFetcher.m:1865)
4  App                      0x393dd78 -[GTLRServiceTicket cancelTicket] + 2607 (GTLRService.m:2607)
thomasvl commented 1 month ago

There's isn't re really enough here to go on to say what might be causing the problem or even why you think it might be a missing nullability issue. All you seem to have provide was limited stack info and not actually what caused the failure.

Those line numbers also don't seem to line up with the 3.3.2 release, do you have local edits or are you using a different release?

0xpablo commented 1 month ago

Thank you for your quick reply. I have confirmed we are on 3.3.2. I wish I could give you more info but we haven't been able to reproduce it. We have about 50K crashes coming from this in the last 90d so it seems to not be super rare. About the stack trace not matching, this is an issue that sometimes happens with Firebase. I'll have a look in the Xcode organizer in case there's more reliable info there. I was wondering if Apple's using Swift in parts of foundation and since things are being bridged from ObjC, depending on the nullability annotations of the ObjC side, that might cause a crash but I'm not certain at all about this.

0xpablo commented 1 month ago

I was able to find the crash on the organizer, it points to this line:

  [_authorizer stopAuthorizationForRequest:request];

I think that makes sense. The request is probably null and I suppose we are implementing that protocol in Swift (I'm not familiar with this part of the code in our app). I guess the protocol should be annotated to reflect that the request is nullable or make sure stopFetchReleasingCallbacks cannot produce a null request.

Thread 24 name:
Thread 24 Crashed:
0   Foundation                      0x0000000198544afc static URLRequest._unconditionallyBridgeFromObjectiveC(_:) + 264 (URLRequest.swift:358)
1   App                         0x00000001061ac2d0 -[GTMSessionFetcher stopFetchReleasingCallbacks:] + 524 (GTMSessionFetcher.m:2149)
2   App                         0x00000001061b9bd0 -[GTMSessionUploadFetcher stopFetchReleasingCallbacks:] + 104 (GTMSessionUploadFetcher.m:1160)
3   App                         0x00000001061bbddc -[GTMSessionUploadFetcher stopFetching] + 228 (GTMSessionUploadFetcher.m:1864)
4   App                         0x00000001061e1d78 -[GTLRServiceTicket cancelTicket] + 56 (GTLRService.m:2605)
thomasvl commented 1 month ago

The header uses NS_ASSUME_NONNULL_BEGIN/NS_ASSUME_NONNULL_END, so it is annotated.

The request also comes out an an ivar, so the better question is how are you managing to get to that point without a request or what is clearing the request out from under the library.

0xpablo commented 1 month ago

The header uses NS_ASSUME_NONNULL_BEGIN/NS_ASSUME_NONNULL_END, so it is annotated.

The request also comes out an an ivar, so the better question is how are you managing to get to that point without a request or what is clearing the request out from under the library.

I see. It's annotated as non-nullable but GTMSessionFetcher is violating that contract as the request property of GTMSessionFetcher is nullable, and the request can be null when calling that delegate. We are not interacting with GTMSessionFetcher directly, so I'm not sure what could we be doing wrong, if I look a bit up in the stack I see we are using GTLRDriveService.executeQuery, which is what calls GTLRServiceTicket cancelTicket.

thomasvl commented 1 month ago

The ivar can be nil during the full flow, but normally you don't get to the cancel case with that happening, that's why I was saying there's more info needed to understand how you are getting into that state. Simply adding an if would just be papering over the situation; so understand how you get into the state is just as important as stopping a potential nil call there.