-
Notifications
You must be signed in to change notification settings - Fork 130
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
Prevent potential crash during app teardown #308
Conversation
If the session API client is garbage collected while sessions are enqueued to be delivered, there can be a crash when accessing self.config.apiKey from the operation queue.
I feel like we might want to call |
@kattrali is Simon's suggestion something we might want to do, in addition to or in place of the fix that was originally proposed? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with Simon here, in dealloc
, all operations should be cancelled to avoid starting a new operation while garbage collection is occurring.
@@ -19,8 +19,11 @@ - (NSOperation *)deliveryOperation { | |||
} | |||
|
|||
- (void)deliverSessionsInStore:(BugsnagSessionFileStore *)store { | |||
NSString *apiKey = self.config.apiKey; | |||
NSURL *sessionURL = self.config.sessionURL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these values should be copied
Goal
If the session API client is garbage collected while sessions are enqueued to be delivered, there
can be a crash when accessing self.config.apiKey from the operation queue. This changeset fixes that by copying the API key/session URL accessed from within the operation block.
Changeset
Copy the
sessionURL
andapiKey
values so thatself
is not accessed from the operation block.Tests
Existing unit tests/mazerunner.
Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for: