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

iOS: Common Feedly crash #1481

Closed
vincode-io opened this issue Dec 28, 2019 · 10 comments
Closed

iOS: Common Feedly crash #1481

vincode-io opened this issue Dec 28, 2019 · 10 comments
Assignees
Milestone

Comments

@vincode-io vincode-io added this to the iOS 5.0 Alpha milestone Dec 28, 2019
@vincode-io

This comment has been minimized.

Copy link
Member Author

@vincode-io vincode-io commented Dec 28, 2019

Here is another crashlog that looks like the same problem to me.
2019-12-23_15-59-48.9138_-0800-c707803396d80346acb64082073b4821a443d7fe.crash.zip

@brentsimmons brentsimmons self-assigned this Dec 29, 2019
@brentsimmons

This comment has been minimized.

Copy link
Collaborator

@brentsimmons brentsimmons commented Dec 30, 2019

The critical part looks like this:

1   Foundation                    	0x00000001b742f940 __NSOQSchedule + 108 (NSOperation.m:1762)
2   Foundation                    	0x00000001b742f6d8 +[__NSOperationInternalObserver _observeValueForKeyPath:ofObject:changeKind:oldValue:newValue:ind... + 1920 (NSOperation.m:2368)
3   Foundation                    	0x00000001b7415e78 NSKeyValueNotifyObserver + 136 (NSKeyValueObserving.m:401)
4   Foundation                    	0x00000001b741801c NSKeyValueDidChange + 336 (NSKeyValueObserving.m:532)
5   Foundation                    	0x00000001b7415740 NSKeyValueDidChangeWithPerThreadPendingNotifications + 152 (NSKeyValueObserving.m:1189)
6   libswiftFoundation.dylib      	0x00000001ecb069d0 $s10Foundation27_KeyValueCodingAndObservingPAAE010willChangeC03forys0B4PathCyxqd__Gn_tlFTm + 232
7   Account                       	0x000000010463ec20 FeedlyOperation.didFinish() + 204 (FeedlyOperation.swift:101)

This is why we don’t use KVO. Except, unfortunately, we have to use KVO with Operation subclasses.

My best guess is that an operation is finishing but its OperationQueue is gone. I’m not positive of that. But it looks like an operation is finishing, doing the KVO notification thing, and its observer is gone.

If we can’t figure out the crash, we’ll either switch to DispatchQueue or write our own OperationQueue/Operation implementation that doesn’t use KVO. Both are big enough jobs that I don’t want us to have to do them. Hopefully we can figure out the crash.

@kielgillard

This comment has been minimized.

Copy link
Member

@kielgillard kielgillard commented Dec 30, 2019

My best guess is that an operation is finishing but its OperationQueue is gone. I’m not positive of that. But it looks like an operation is finishing, doing the KVO notification thing, and its observer is gone.

This is my take on the situation, too. I'm looking into it but still less able to contribute until 6 January.

@amrox

This comment has been minimized.

Copy link

@amrox amrox commented Dec 31, 2019

This is a longshot, since I've only glanced at the project but

I've used NSOperations quite a bit over the years. This is the subclass I currently use in production code: https://gist.github.com/amrox/a5c7f10c18d52c725e3291e3983a7c48 (please excuse the ObjC)

The main takeaway is to use a single state variable instead of two booleans for isExecuting and isFinished.

In the FeedlyOperation we have:

isExecutingOperation = false
isFinishedOperation = true

When KVO is triggered by isExecutingOperation = false, the operations hasn't been set to finished yet. Therefore the Operation is not executing, but also not finished, which may be an invalid state from the point of view of the OperationQueue.

In gist I posted, the Operation can never be simultaneously not executing, and not finished. I definitely can't say if it will fix the crash, but it may be a small improvement to the Operation lifecycle state regardless.

Either way, good luck on the bug!

@hborders

This comment has been minimized.

Copy link

@hborders hborders commented Dec 31, 2019

In the concurrency programming guide Apple says to wrap the isExecuting and isFinished ivar mutations in -willChangeValueForKey: and -didChangeValueForKey::

- (void)start {
  // Always check for cancellation before launching the task.
  if ([self isCancelled]) {
    // Must move the operation to the finished state if it is canceled.
    [self willChangeValueForKey:@"isFinished"];
    finished = YES;
    [self didChangeValueForKey:@"isFinished"];
    return;
  }
  // If the operation is not canceled, begin executing the task.
  [self willChangeValueForKey:@"isExecuting"];
  [NSThread detachNewThreadSelector:@selector(main) toTarget:self withObject:nil];
  executing = YES;
  [self didChangeValueForKey:@"isExecuting"];
}
- (void)main {
  @try { 
    // Do the main work of the operation here.
    [self completeOperation];
    } @catch(...) {
    // Do not rethrow exceptions.
    }
}
 
- (void)completeOperation {
  [self willChangeValueForKey:@"isFinished"];
  [self willChangeValueForKey:@"isExecuting"];
 
  executing = NO;
  finished = YES;
 
  [self didChangeValueForKey:@"isExecuting"];
  [self didChangeValueForKey:@"isFinished"];
}

I personally like @amrox's solution better because it eliminates an impossible state, but for those that prefer separate booleans, it seems like -willChangeValueForKey: and -didChangeValueForKey: will handle order and atomicity.

@flexaddicted

This comment has been minimized.

Copy link

@flexaddicted flexaddicted commented Jan 1, 2020

Some thoughts on the crash @brentsimmons @hborders @kielgillard @amrox @vincode-io

First of all, FeedlyOperation is async but you don't override isAsynchronous property like (I've inspected master branch):

override var isAsynchronous: Bool {
    return true
}

Second, I would protect KVO code from race conditions using an approach like:

private var _isExecuting: Bool = false
override private(set) var isExecuting: Bool {
    get {
        return lockQueue.sync { () -> Bool in
            return _isExecuting
        }
    }
    set {
        willChangeValue(forKey: "isExecuting")
        lockQueue.sync(flags: [.barrier]) {
            _isExecuting = newValue
        }
        didChangeValue(forKey: "isExecuting")
    }
}

where lockQueue is defined as

private let lockQueue = DispatchQueue(label: "myQueue", attributes: .concurrent)

Another thing is the usage of cancel. Reading Apple doc, it says

In addition to simply exiting when an operation is cancelled, it is also important that you move a cancelled operation to the appropriate final state. Specifically, if you manage the values for the isFinished and isExecuting properties yourself (perhaps because you are implementing a concurrent operation), you must update those properties accordingly. Specifically, you must change the value returned by isFinished to true and the value returned by isExecuting to false. You must make these changes even if the operation was cancelled before it started executing.

My question is: should cancel call super and then trigger KVO compliance?

You can find additional info below:

@drewmccormack

This comment has been minimized.

Copy link

@drewmccormack drewmccormack commented Jan 4, 2020

As others point out, there are a bunch of issues with the FeedlyOperation class, such as the isAsynchronous override, and not changing to the final state atomically. But I think maybe the biggest problem is the use of Swift key paths. Not sure if that should be supported, but in my testing, it doesn't work. Changing to strings for the key paths fixed things for me.

    /// Fires the KVO needed for the operation queue to know the operation has begun
    private func fireNotificationsForBeginning() {
        willChangeValue(forKey: "isFinished")
        willChangeValue(forKey: "isExecuting")
        self.isFinished = false
        self.isExecuting = true
        didChangeValue(forKey: "isExecuting")
        didChangeValue(forKey: "isFinished")
    }
    
    /// Fires the KVO needed for the operation queue to know operation has finished
    private func fireNotificationsForFinishing() {
        willCompleteBlock?()
        willCompleteBlock = nil // Break retain cycles
        willChangeValue(forKey: "isFinished")
        willChangeValue(forKey: "isExecuting")
        self.isFinished = true
        self.isExecuting = false
        didChangeValue(forKey: "isExecuting")
        didChangeValue(forKey: "isFinished")
    }
@vincode-io

This comment has been minimized.

Copy link
Member Author

@vincode-io vincode-io commented Jan 8, 2020

@brentsimmons

This comment has been minimized.

Copy link
Collaborator

@brentsimmons brentsimmons commented Jan 8, 2020

We’re going to write a replacement for OperationQueue. (Needs new ticket; should be assigned to me.)

@brentsimmons

This comment has been minimized.

Copy link
Collaborator

@brentsimmons brentsimmons commented Jan 16, 2020

Now that we’ve switched from OperationQueue to our own queue, this crash can’t happen any more, since it happens in code that we no longer run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.