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.

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

EXC_BAD_ACCESS (SIGSEGV) with Storage and Carthage #3750

Closed
supermarin opened this issue Sep 2, 2019 · 16 comments · Fixed by #3786

Comments

@supermarin
Copy link

commented Sep 2, 2019

[REQUIRED] Step 2: Describe your environment

  • Xcode version: 11.0 Beta 6
  • Firebase SDK version: 6.7.0
  • Firebase Component: Storage
  • Component version: 6.7.0

[REQUIRED] Step 3: Describe the problem

Upon completion of uploadTasks, in about 1/3 of cases the app crashes with EXC_BAD_ACCESS (SIGSEGV)

Steps to reproduce:

  1. Initialize a simple app with 1 table view controller
  2. Sign in with Firebase Auth
  3. kick off an upload task (either from a file or Data, 1MB) on a cell press
  4. see it crashing once upload is complete.
    NOTE: it doesn't crash on every single upload, crashes about 1/3 of the time.

Relevant Code:

func tableView(_ tableView: UITableView, didSelectRowAt indexPath: IndexPath) {
        // TODO: Expand cell, add sharing / uploading
        guard let user = Auth.auth().currentUser, let email = user.email else {
            // handle sign in experience
            return
        }

        let item = items[indexPath.row]
        let storageRef = Storage.storage().reference()
        let itemsRef = storageRef.child("items")
        let userBucketRef = itemsRef.child(email)

        let fileName = item.fileURL.lastPathComponent
        let fileRef = userBucketRef.child(fileName)


        let uploadTask = fileRef.putFile(from: item.fileURL, metadata: nil)
        { [weak self] metadata, error in
            guard error == nil else {
                let alert = UIAlertController(title: "Error", message: error?.localizedDescription,
                                              preferredStyle: .alert)
                alert.addAction(UIAlertAction(title: "OK", style: .default))
                self?.show(alert, sender: nil)
                return
            }

            // Happy path
            log(.debug, "Upload done!")
        }
        uploadTask.enqueue()
    }

Gist with full crash log: https://gist.github.com/supermarin/b9fe575cd95efeb905e3b771f0fd883c

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 2, 2019

Thanks for the fast response @paulb777 . Seen the file/method, but haven't spent much time digging into it. Am I missing something obvious?

@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 2, 2019

@supermarin I don't see anything on quick glance, but wanted to leave a pointer for anyone who wants to go deeper, since I'm not able to investigate now. The crash should be debuggable in a CocoaPods installation.

It's likely something exposed/triggered by the Xcode 11 beta, since this code is relatively stable.

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 2, 2019

Just tried with Xcode 10.3, same issue.
This is the Cartfile.resolved:

binary "https://dl.google.com/dl/firebase/ios/carthage/FirebaseAnalyticsBinary.json" "6.7.0"
binary "https://dl.google.com/dl/firebase/ios/carthage/FirebaseAuthBinary.json" "6.7.0"
binary "https://dl.google.com/dl/firebase/ios/carthage/FirebaseDynamicLinksBinary.json" "6.7.0"
binary "https://dl.google.com/dl/firebase/ios/carthage/FirebaseStorageBinary.json" "6.7.0"
github "daltoniam/Starscream" "3.1.0"
github "robbiehanson/CocoaAsyncSocket" "b179ea4013e94e31e6e637955e520ea4fb9d1b13"

If I catch time I'll try to debug with CocoaPods.

@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

A full sample would help us to investigate.

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 4, 2019

@paulb777 can provide tonight when I get home

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

The problem is here, where you call a stored block on a nullable reference without checking if it's still alive.

This fixes the problem:

[strongSelf->_uploadFetcher
        beginFetchWithCompletionHandler:^(NSData *_Nullable data, NSError *_Nullable error) {
          if (weakSelf) {
            weakSelf.fetcherCompletion(data, error);
          }
        }];

Another issue is that this gets called more than once per task completion, and I haven't had time to drill that one down.

If you add logs to both cases, you can observe that the completion block sometimes gets called more than once, when the task dies.

    [strongSelf->_uploadFetcher
        beginFetchWithCompletionHandler:^(NSData *_Nullable data, NSError *_Nullable error) {
          if (weakSelf) {
              NSLog(@"Was not null!");
            weakSelf.fetcherCompletion(data, error);
          } else {
              NSLog(@"Was null!");
          }
        }];
Hello[68954:2361032] [app] Upload tapped
Hello[68954:2361032] Was not null!
Hello[68954:2361032] [app] Upload done!
Hello[68954:2361032] Was null! 		  <--- Problem, shouldn't have been called

Hello[68954:2361032] [app] Upload tapped
Hello[68954:2361032] Was not null!
Hello[68954:2361032] [app] Upload done!

Hello[68954:2361032] [app] Upload tapped
Hello[68954:2361032] Was not null!
Hello[68954:2361032] [app] Upload done!

Hello[68954:2361032] [app] Upload tapped
Hello[68954:2361032] Was not null!
Hello[68954:2361032] [app] Upload done!
@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

@supermarin I'm not completely following. If weakSelf is nil, the fetcherCompletion won't be called since message sends to nil are no-ops.

It does seem like a problem for the completion block to be called twice.

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

If weakSelf is nil, the fetcherCompletion won't be called since message sends to nil are no-ops.

The problem is not with calling methods on a niled out __weak ref, but invoking a block reference that's now a NULL pointer. You can reproduce it quite easily:

#import <Foundation/Foundation.h>

@interface Task: NSObject
@property (nonatomic, copy) bool(^completion)();
@property (nonatomic, strong) NSString *doesntBlowUp;
- (void)doSomething;
@end

@implementation Task
- (void)doSomething {}
@end

Task *createTask() {
    return [Task new];
}

int main(int argc, char * argv[]) {
    __weak Task *task = createTask();
    task.doesntBlowUp = @"nop";
    task.completion = ^bool() {
    	return false;
    };
    NSLog(@"just a proof that the block doesn't crash");
    task.completion();

    task = nil;

    NSLog(@"property access is OK: %@", task.doesntBlowUp);
    NSLog(@"method access is safe too.");
    [task doSomething];

    NSLog(@"invoking a NULL pointer will crash");
    task.completion();
    return 1;
}

Outputs:

$ clang -fobjc-arc main.m
$ ./a.out
2019-09-05 12:44:57.831 a.out[28157:4682243] just a proof that the block doesn't crash
2019-09-05 12:44:57.832 a.out[28157:4682243] property access is OK: (null)
2019-09-05 12:44:57.832 a.out[28157:4682243] method access is safe too.
2019-09-05 12:44:57.832 a.out[28157:4682243] invoking a NULL pointer will crash
zsh: segmentation fault  ./a.out
@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

Thanks for the explanation! That makes sense.

I'm not able to reproduce the double call problem. I'm also not able to reproduce the crash, but I can see how in theory it could happen. I'm testing a fix at #3786.

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 5, 2019

Thanks Paul, I can try it at home later tonight and let you know if it fixed the issue.

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 6, 2019

@paulb777 fix works, and the second completion doesn't get called anymore because of the nil check (it still hits your code occasionally though). In any case it's better than what's currently released in 6.7.0. If you wouldn't mind releasing 6.7.1 after merging, that'd be highly appreciated.

@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@supermarin Thanks for testing. The fix is now merged.

I assume that pushing out a CocoaPods FirebaseStorage update doesn't help you? Our Carthage publishing script is a little rough and in transition, but we can look at getting a 6.8.1 out next week. (6.8.0 released yesterday)

@supermarin

This comment has been minimized.

Copy link
Author

commented Sep 7, 2019

Yep CocoaPods won't help, please do it in 6.8.1 if you can. Thanks a bunch

@paulb777 paulb777 added this to the M55.1 milestone Sep 9, 2019
@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

We released 6.8.1 to CocoaPods today. The Carthage distro should go out tomorrow.

In the meantime, the fixed FirebaseStorage.framework is available in the zip distro.

@paulb777 paulb777 removed the help wanted label Sep 10, 2019
@paulb777

This comment has been minimized.

Copy link
Member

commented Sep 10, 2019

Carthage 6.8.1 is out.

@firebase firebase locked and limited conversation to collaborators Oct 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.