[ios] Fix a race condition issue in the Mapbox metrics class. #15029
Conversation
fabian-guerra
added
iOS
| @@ -65,9 +65,13 @@ - (NSURLSessionConfiguration *)defaultSessionConfiguration { | |||
| } | |||
|
|
|||
| - (void)startDownloadEvent:(NSString *)urlString type:(NSString *)resourceType { | |||
| if (urlString && ![self.events objectForKey:urlString]) { | |||
| [self.events setObject:@{ MGLStartTime: [NSDate date], MGLResourceType: resourceType } forKey:urlString]; | |||
| NSString *retainedUrlString = urlString; | |||
There was a problem hiding this comment.
I don't think this is sufficient. AFAIK urlString should be retained by this method anyway - how about using copy here?
There was a problem hiding this comment.
I consider using copy but an increase in the retain count would do the job better.
| NSString *retainedUrlString = urlString; | ||
| NSString *retainedResourceType = resourceType; | ||
| if (retainedUrlString && !self.events[retainedUrlString]) { | ||
| self.events[retainedUrlString] = @{ MGLStartTime: [NSDate date], MGLResourceType: retainedResourceType }; |
There was a problem hiding this comment.
I think we need to change the events property to be atomic:
@property (atomic, strong) NSMutableDictionary<NSString*, NSDictionary*> *events;
since reading/writing/deleting could happen at the same time.
There was a problem hiding this comment.
This property gets called multiple times. I consider the atomicity but in practice found no reason to make this an atomic property when the crash is raised for a different reason.
| if (urlString && [self.events objectForKey:urlString]) { | ||
| NSDictionary *eventAttributes = [self eventAttributesForURL:response withAction:action]; | ||
| NSDictionary *eventAttributes = [self eventAttributesForURL:retainedResponse withAction:action]; | ||
| [self.metricsDelegate networkConfiguration:self didGenerateMetricEvent:eventAttributes]; |
There was a problem hiding this comment.
We should dispatch this delegate call on the main queue.
fabian-guerra
removed
the
needs changelog
|
|
Closes #14982 |
julianrex
added
the
⚠️ DO NOT MERGE
|
Added #15113 to replace this PR. |
|
Per #15029 (comment), closing to avoid confusion |
We have |
Fixes #14982
Downloading multiple offline packs starts multiple background requests. The metrics controller we were using used a property from a URL object in an autoreleased pool. This request may go away in the middle of setting the starting event, which may have caused the crash.
I think is unnecessary to use a lock for the
urlStringvariable. instead I'm retaining the variable to avoid passing nil to the events dictionary./cc @friedbunny