karelia / BSManagedDocument

Brings UIManagedDocument's design to OS X
Other
55 stars 25 forks source link

crashes upon auto-save in NSManagedObject property validation, OS-X 10.10.5 #44

Open Motti-Shneor opened 8 years ago

Motti-Shneor commented 8 years ago

Inconsistent, but happens a lot in my app. Stack is always the same: (See at the end). The code that crashes is here: My entity has a "catalogID" attribute, that I need to verify for uniqueness. This is from the managedObject subclass.

pragma mark - CoreData KVC Validation (per-property)

// Validate the catalogID for uniqueness. This implementation works fine in the following scenarios: // * end-editing, // * saving a new species, // * updating existing species. -(BOOL)validateCatalogID:(id )ioValue error:(NSError * __autoreleasing )outError {

if (*ioValue == nil)
    return YES; // Let CoreData validate for empty/missing values

// Lazily prepare static request and predicate to fetch OTHER species catalogID's. We'll reuse these every time we validate the edited species catalogID.
static NSFetchRequest *otherSpeciesIDsFetchRequest = nil;
static NSPredicate *otherSpeciesCatalogIDsPredicate = nil;
static dispatch_once_t onceToken;
dispatch_once(&onceToken, ^{
    otherSpeciesIDsFetchRequest = [NSFetchRequest fetchRequestWithEntityName:[[self entity] name]];
    [otherSpeciesIDsFetchRequest setIncludesSubentities:NO];
    [otherSpeciesIDsFetchRequest setIncludesPropertyValues:NO];
    [otherSpeciesIDsFetchRequest setFetchLimit:3];
    otherSpeciesCatalogIDsPredicate = [NSPredicate predicateWithFormat:@"(catalogID == $ID) AND (self != $THIS_SPECIES)"];
});

@synchronized(otherSpeciesIDsFetchRequest) {
    // Configure the predicate with the current catalogID value to validate.
    otherSpeciesIDsFetchRequest.predicate = [otherSpeciesCatalogIDsPredicate predicateWithSubstitutionVariables:@{@"ID":*ioValue, @"THIS_SPECIES":self}];

    // instead of fectching entity data, we only COUNT matching items.
    NSError *error = nil;
    NSInteger count = [self.managedObjectContext countForFetchRequest:otherSpeciesIDsFetchRequest error:&error];
//    DBGLog(@"Check Species ID:%@ uniqueness, found %ld", *ioValue, (long)count);
    if (count > 0) {
        [self addValidationError:outError code:ePMXexistingCatalogID
                         message:[NSString stringWithFormat:NSLocalizedString(@"ID '%@' is already used for another species. Use a unique ID", NULL), *ioValue]
                            ];
        return NO;
    }
    else {
        if (error!=nil) {
            [self addValidationError:outError code:ePMXexistingCatalogID
                             message:[NSString stringWithFormat:NSLocalizedString(@"Species ID '%@' cannot be validated. error %@", NULL), *ioValue, error]];
            return NO;
        }
    }
    return YES; // this catalogID was not found in any other species entity - unique - valid catalogID.
}

}

Crash info: always in the main thread, I do not set up any other threads myself in the application, neither I setup any operation queue of my own - all other threads are system/GCD normal threads, and they don't seem to do anything interesting at that time.

Crashed Thread: 0 Dispatch queue: com.apple.main-thread

Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000018

VM Regions Near 0x18: --> __TEXT 00000001067f1000-00000001068e8000 [ 988K] r-x/rwx SM=COW /Applications/PlanktoMEtrix-II V1.0b20/PlanktoMetrix-II.app/Contents/MacOS/PlanktoMetrix-II

Application Specific Information: objc_msgSend() selector name: subentitiesByName

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 libobjc.A.dylib 0x00007fff894eb0dd objc_msgSend + 29 1 com.IOLR.PlanktoMetrix-II 0x00000001068003fc -[PMSpecies(PMExtensions) validateCatalogID:error:] + 732 2 com.apple.CoreData 0x00007fff94ff8554 -[NSManagedObject(_NSInternalMethods) _validateValue:forProperty:andKey:withIndex:error:] + 228 3 com.apple.CoreData 0x00007fff94ff834d -[NSManagedObject(_NSInternalMethods) _validatePropertiesWithError:] + 317 4 com.apple.CoreData 0x00007fff94ff81cb -[NSManagedObject(_NSInternalMethods) _validateForSave:] + 123 5 com.apple.CoreData 0x00007fff94ff810c -[NSManagedObject validateForUpdate:] + 76 6 com.IOLR.PlanktoMetrix-II 0x0000000106801ce9 -[PMSpecies(PMExtensions) validateForUpdate:] + 73 7 com.apple.CoreData 0x00007fff94ff75f1 -[NSManagedObjectContext(_NSInternalAdditions) _validateObjects:forOperation:error:exhaustive:forSave:] + 1425 8 com.apple.CoreData 0x00007fff94ff700d -[NSManagedObjectContext(_NSInternalAdditions) _validateChangesForSave:] + 413 9 com.apple.CoreData 0x00007fff94ff6c86 -[NSManagedObjectContext(_NSInternalChangeProcessing) _prepareForPushChanges:] + 214 10 com.apple.CoreData 0x00007fff94ff493a -[NSManagedObjectContext save:] + 250 11 com.IOLR.PlanktoMetrix-II 0x00000001067faca6 -[BSManagedDocument contentsForURL:ofType:saveOperation:error:] + 918 12 com.IOLR.PlanktoMetrix-II 0x00000001067fc2ab 73-[BSManagedDocument saveToURL:ofType:forSaveOperation:completionHandler:]_block_invoke + 411 13 com.apple.AppKit 0x00007fff8e9fa681 -[NSDocument performAsynchronousFileAccessUsingBlock:] + 62 14 com.IOLR.PlanktoMetrix-II 0x00000001067fc072 -[BSManagedDocument saveToURL:ofType:forSaveOperation:completionHandler:] + 370 15 com.apple.AppKit 0x00007fff8eb43e6c 67-[NSDocument autosaveWithImplicitCancellability:completionHandler:]_block_invoke + 1318 16 com.apple.AppKit 0x00007fff8e9fac89 -[NSDocument continueFileAccessUsingBlock:] + 222 17 com.apple.AppKit 0x00007fff8ed211a4 54-[NSDocument performAsynchronousFileAccessUsingBlock:]_block_invoke729 + 125 18 com.apple.AppKit 0x00007fff8ed65aa2 62-[NSDocumentController(NSInternal) _onMainThreadInvokeWorker:]_block_invoke1969 + 175 19 com.apple.CoreFoundation 0x00007fff8e2868ec CFRUNLOOP_IS_CALLING_OUT_TO_A_BLOCK + 12 20 com.apple.CoreFoundation 0x00007fff8e2789f5 CFRunLoopDoBlocks + 341 21 com.apple.CoreFoundation 0x00007fff8e278536 CFRunLoopRun + 1814 22 com.apple.CoreFoundation 0x00007fff8e277bd8 CFRunLoopRunSpecific + 296 23 com.apple.HIToolbox 0x00007fff8b7a556f RunCurrentEventLoopInMode + 235 24 com.apple.HIToolbox 0x00007fff8b7a52ea ReceiveNextEventCommon + 431 25 com.apple.HIToolbox 0x00007fff8b7a512b _BlockUntilNextEventMatchingListInModeWithFilter + 71 26 com.apple.AppKit 0x00007fff8e86e8ab _DPSNextEvent + 978 27 com.apple.AppKit 0x00007fff8e86de58 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 346 28 com.apple.AppKit 0x00007fff8e863af3 -[NSApplication run] + 594 29 com.apple.AppKit 0x00007fff8e7e0244 NSApplicationMain + 1832 30 com.IOLR.PlanktoMetrix-II 0x000000010680dae2 main + 34 31 libdyld.dylib 0x00007fff928bc5c9 start + 1

mikeabdullah commented 8 years ago

Hi Motti, I don't have any immediate insight as to why this might be a problem. Worth checking, you're using ARC for this code, yes?

Can you reproduce the crash yourself, or do you only ever get in external reports of it? What I find a bit weird is the complaint of the selector subentitiesByName, but your posted code never actually calls that itself.

Motti-Shneor commented 8 years ago

On Dec 19, 2015, at 21:20, Mike Abdullah notifications@github.com wrote:

Hi Motti, I don't have any immediate insight as to why this might be a problem. Worth checking, you're using ARC for this code, yes?

64bit, ARC, Xcode 6.4, OS-X SDK 10.10, Deployment 10.9.

Can you reproduce the crash yourself, or do you only ever get in external reports of it?

I cannot reproduce it myself. I only got this from clients, but it happens to them in identifiable scenario, most every day (I got about 25 similar crash reports, from 3 different machines, running 10.9.5 and 10.10.5). What I find a bit weird is the complaint of the selector subentitiesByName, but your posted code never actually calls that itself.

Yup. I imagine fetchRequestWithEntityName:[[self entity] name] - before includesSubentities = NO will gather all subentities of the entity provided - using that method. This also means the crash happens the very first time I enter that dispatch_once {} clause.

Now I have advanced a little in my research about this crash.

I started wondering why that saving operation involved validation at all — after all, the MOC there is BSManagedDocument’s own background context, used only for deferring writes/reads to persistent store to other threads — no user changes ever reach that context that justify validation. It should be enough to validate the contexts where user introduces changes.

So… for the time being, I introduced a few changes to my code:

  1. I no longer reuse the fetch-request, but recreate it for each validation, in case something is wrong with my dispatch_once and the later synchronized modification of the predicate, rendering my code non-thread-safe.
  2. I avoid the validation - return YES, if the context’s parentContext is nil.

This crash is still worth looking at, but for the time being, I may be able to save my clients from it.

My validation method now looks like this:

pragma mark - CoreData KVC Validation (per-property)

// Validate the catalogID for uniqueness. This implementation works fine in the following scenarios: // * end-editing, // * saving a new species, // * updating existing species. -(BOOL)validateCatalogID:(id )ioValue error:(NSError * __autoreleasing )outError {

if (*ioValue == nil)
    return YES; // Let CoreData validate for empty/missing values

if (!self.managedObjectContext.parentContext)    // this validation is done while saving via BSManagedDocument's parent background context. No verification.
    return YES;

NSFetchRequest *otherSpeciesIDsFetchRequest = [NSFetchRequest fetchRequestWithEntityName:[[self entity] name]];
otherSpeciesIDsFetchRequest.includesSubentities = NO;
otherSpeciesIDsFetchRequest.includesPropertyValues =NO;
otherSpeciesIDsFetchRequest.fetchLimit = 3;
otherSpeciesIDsFetchRequest.predicate = [NSPredicate predicateWithFormat:@"(catalogID == %@) AND (self != %@)", *ioValue, self];

// instead of fectching entity data, we only COUNT matching items.
NSError *error = nil;
NSInteger count = [self.managedObjectContext countForFetchRequest:otherSpeciesIDsFetchRequest error:&error];
//    DBGLog(@"Validated Species ID:%@ for uniqueness, found %ld", *ioValue, (long)count);
if (count > 0) {
    NSString *errorMessage = [NSString stringWithFormat:NSLocalizedString(@"ID '%@' is already used for another species. Use a unique ID", NULL), *ioValue];
    [self addValidationError:outError code:ePMXexistingCatalogID message:errorMessage];
    return NO;
}
else {
    if (error!=nil) {
        NSString *errorMessage = [NSString stringWithFormat:NSLocalizedString(@"Species ID '%@' cannot be validated. error %@", NULL), *ioValue, error];
        [self addValidationError:outError code:ePMXexistingCatalogID message:errorMessage];
        return NO;
    }
}
return YES; // this catalogID was not found in any other species entity - unique - valid catalogID.

}

— Reply to this email directly or view it on GitHub https://github.com/karelia/BSManagedDocument/issues/44#issuecomment-166017262.

Motti Shneor e-mail: motti.shneor@gmail.com phone: +972-8-9267730

mobile: +972-54-3136621

Ceterum censeo Microsoftinem delendam esse

adib commented 8 years ago

I wonder why do you need to synchronize the fetch (@synchronized(otherSpeciesIDsFetchRequest)). Needing to synchronize while accessing Core Data objects always smell trouble.

Why not enclose the entire fetch inside performBlock: or performBlockAndWait: instead?

In any case, your crash shows it happened at Thread 0 – which is the main thread. Then you shouldn't need to synchronize here.

adib commented 8 years ago

PS: NSFetchRequest objects are designed to be single-shot. It's not a good idea to reuse them and you should never share them between threads.

Motti-Shneor commented 8 years ago

Nowhere in the documentation I saw any hint of an NSFetchRequest being “one shot” object. It has no MOC, and is designed to be used across different contexts - which means - at times - in different threads. By synchronizing around my fetch-request object, I prevent concurrent use of my fetch-request in multiple threads.

Beyond all this, this technique has worked for me in many projects for a good few years now.

The reason I took the trouble to lazily create then reuse my fetch-request, is that it is being called very frequently, and great many times, because it plays part in my Core-Data validation scheme - which is out of my control.

I now reverted my code to recreate the fetch-request on every validation, AND to avoid all validations on the background-only-saving-context of BSManagedDocument, still — these crashes may hint of an issue in the BSManagedDocument saving implementation, or at least incompleteness of the documentation (how BSManagedDocument should be used, how is it NOT meant to be used, etc.)

On 20 בדצמ׳ 2015, at 17:37, Sasmito Adibowo notifications@github.com wrote:

PS: NSFetchRequest objects are designed to be single-shot. It's not a good idea to reuse them and you should never share them between threads.

— Reply to this email directly or view it on GitHub https://github.com/karelia/BSManagedDocument/issues/44#issuecomment-166129760.