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.

Sign up for GitHub

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

Jump to bottom

fetcherCompletion is assign #1565

Closed
FindCrt opened this issue Jul 20, 2018 · 6 comments · Fixed by #1721
Closed

fetcherCompletion is assign #1565

FindCrt opened this issue Jul 20, 2018 · 6 comments · Fixed by #1721
Assignees
@morganchen12
Labels
api: storage
Milestone
5.7.0

Comments

@FindCrt
Copy link

FindCrt commented Jul 20, 2018 •
edited

@property(readonly) GTMSessionFetcherCompletionHandler fetcherCompletion;

This property is using default assign, so fetcherCompletion will may be released to case a wild pointer and EXC_BAD_ACCESS crash.

In the file FIRStorageDownloadTask.m, there is a assignment for fetcherCompletion :

_fetcherCompletion = ^(NSData *data, NSError *error) {
      ......
};

And at the end of this scope, there is a fetch:

[self.fetcher beginFetchWithCompletionHandler:^(NSData *data, NSError *error) {
    weakSelf.fetcherCompletion(data, error);
  }];

Because this fetch is async, when weakSelf.fetcherCompletion calls, the temporary scope in which fetcherCompletion is assigned has gone. fetcherCompletion become a bad memory.
@paulb777
Copy link
Member

@ToFind1991 Sorry for the delayed response.

Are you seeing a demonstrable issue with this code?

My understanding is that the weakSelf takes care of the issue that you're describing. When fetcherCompletion is deallocated along with its containing object, weakSelf will be set to nil and the function call will be a no-op.

@morganchen12
Copy link
Contributor

To clarify the object being deallocated here isn't self, it's the fetcherCompletion closure. This should be copy and not assign.

@paulb777
Copy link
Member

This stackoverflow answer seems to indicate that while copy is recommended for documentation, it's not functionally necessary when built with ARC.

@morganchen12
Copy link
Contributor

strong will do too, but assign is hazardous since it doesn't provide any guarantees for ownership or deallocation.

@morganchen12 morganchen12 self-assigned this Aug 20, 2018
@morganchen12 morganchen12 mentioned this issue Aug 20, 2018
Merged
@paulb777 paulb777 added this to the M32 milestone Aug 21, 2018
paulb777 added a commit that referenced this issue Aug 24, 2018
@paulb777
Minimal fix for #1565
d0440e6
@paulb777 paulb777 mentioned this issue Aug 24, 2018
Merged
paulb777 added a commit that referenced this issue Aug 24, 2018
@paulb777
Minimal fix for #1565 (#1747)
317db5c
paulb777 added a commit that referenced this issue Aug 24, 2018
@paulb777
Minimal fix for #1565 (#1747)
2507c7a
@paulb777 paulb777 mentioned this issue Aug 24, 2018
Merged
@andrewehab

This comment has been minimized.

Sign in to view
@BlakeBoxberger
Copy link

BlakeBoxberger commented Aug 20, 2019 •
edited

It looks like this issue has somehow re-manifested itself. Even with the copy attribute, I'm getting EXC_BAD_ACCESS on this exact line.

EDIT: This can be avoided by using .observe() instead of .enqueue(). It should still be properly fixed, but I hope this suggestion helps anyone desperately searching for a fix like I was.

@firebase firebase locked and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@morganchen12 morganchen12

Labels
api: storage
Projects
None yet
Milestone
5.7.0
Development

Successfully merging a pull request may close this issue.

Fix assign default on fetcherCompletion morganchen12/firebase-ios-sdk
5 participants
@paulb777 @morganchen12 @FindCrt @BlakeBoxberger @andrewehab