Skip to content

Commit

Permalink
Fixed rare crash when deallocating object graphs involving cross pers…
Browse files Browse the repository at this point in the history 
…istent root relationships

I observed the issue with CoreObject version tagged as placeboard-1.0, I haven't figured out yet how to reproduce the issue in a test, but it seems the issue could still happen with the latest CoreObject master.

For the record, here is the crash stack trace with placeboard-1.0 tag and the corresponding EtoileFoundation commit:

#0	0x20cdadd8 in objc_exception_throw ()
#1	0x2142b8f0 in -[__NSSetM addObject:] ()
#2	0x0072f9a0 in -[CORelationshipCache referringObjects] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/CORelationshipCache.m:122
#3	0x006a2310 in -[COObjectGraphContext replaceObject:withObject:] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COObjectGraphContext.m:883
#4	0x006c5ab8 in -[COObject willDiscard] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COObject.m:1726
#5	0x006a0ccc in -[COObjectGraphContext discardObjectsWithUUIDs:] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COObjectGraphContext.m:697
#6	0x0069bd34 in -[COObjectGraphContext dealloc] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COObjectGraphContext.m:121
#7	0x20cf4f8a in objc_object::sidetable_release(bool) ()
#8	0x0066e470 in -[COPersistentRoot .cxx_destruct] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COPersistentRoot.m:33
#9	0x20cd9f3c in object_cxxDestructFromClass(objc_object*, objc_class*) ()
#10	0x20ce3e4a in objc_destructInstance ()
#11	0x20ce3e6e in object_dispose ()
#12	0x20cf4f8a in objc_object::sidetable_release(bool) ()
#13	0x2142a788 in -[__NSDictionaryM dealloc] ()
#14	0x20cf4f8a in objc_object::sidetable_release(bool) ()
#15	0x006826b6 in -[COEditingContext .cxx_destruct] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COEditingContext.m:32
#16	0x20cd9f3c in object_cxxDestructFromClass(objc_object*, objc_class*) ()
#17	0x20ce3e4a in objc_destructInstance ()
#18	0x20ce3e6e in object_dispose ()
#19	0x0067b13e in -[COEditingContext dealloc] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Core/COEditingContext.m:115
#20	0x20cf4f8a in objc_object::sidetable_release(bool) ()
#21	0x00725016 in -[COUndoTrack .cxx_destruct] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Undo/COUndoTrack.m:34
#22	0x20cd9f3c in object_cxxDestructFromClass(objc_object*, objc_class*) ()
#23	0x20ce3e4a in objc_destructInstance ()
#24	0x20ce3e6e in object_dispose ()
#25	0x0071cc20 in -[COUndoTrack dealloc] at /Users/qmathe/reps/Etoile/Frameworks/CoreObject/Undo/COUndoTrack.m:83
#26	0x20cf4f8a in objc_object::sidetable_release(bool) ()
  • Loading branch information
@qmathe
qmathe committed Apr 26, 2016
1 parent abedc4b commit 068d782
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions Core/CORelationshipCache.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ - (NSSet *) referringObjects
NSMutableSet *result = [NSMutableSet set];
for (COCachedRelationship *entry in _cachedRelationships)
{
/* When deallocating an object graph and replacing references to its
inner objects with -[COPath brokenPath], some of them might be
already deallocated. */
if (entry->_sourceObject == nil)
continue;

// N.B.: Don't filter by !isSourceObjectTrackingSpecificBranch as the other methods do
[result addObject: entry->_sourceObject];
}
Expand Down

4 comments on commit 068d782

@ericwa
Copy link
Member

@ericwa ericwa commented on 068d782 Apr 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this will hurt anything, but it does seem that the relationship cache is in an invalid state if this happens, because -[COObject willDiscard] should have removed that entry before the weak reference entry->_sourceObject was zeroed.

Maybe add an assert that (entry->_sourceObject != nil) before the if; AFAIK the assertion won't be compiled in release builds right?

@qmathe
Copy link
Member Author

@qmathe qmathe commented on 068d782 Apr 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this. If NDEBUG isn't defined, assert() shouldn't be compiled in release builds.

However this makes me realize that after merging the lazy-loading branch, we should review our assertions.

We use either assert() or ETAssert(). ETAssert() is always turned on unlike assert(). We have several issues:

  • some assertions have side-effects and will break the code when they are disabled
  • many assert() assertions should be kept turned on during a release build… imo it's better to crash early and often even in a release build, than to have to figure out weird crash reports because the crashes were delayed

In some performance critical parts e.g. binary serialization, expansive assertions should be turned off in release builds. EtoileFoundation provides ETDebugAssert() for this kind of use.

NSAssert() and their variations bring useless complexity and features. So we could change ETAssert() and ETDebugAssert() to use assert(), in this way they could be easily used everywhere whether it's a C function or and ObjC method. Or we could use assert() and introduce something like slowassert() or debugassert() with a flag to only enable it in debug builds.

What's your take on this?

@ericwa
Copy link
Member

@ericwa ericwa commented on 068d782 Apr 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some assertions have side-effects and will break the code when they are disabled

I think those are bugs, and it should be trivial to fix them (change assert([self doSomething]); to bool ok = [self doSomething]; assert(ok);

But, anyway, I agree with you on both points, I prefer to keep assertions in release builds, (except for very costly checks), and NSAssert seems bloated. We use NSParameterAssert a fair bit, looks like 50-60 uses in CO.

A problem with names like ETAssert, NSAssert, FooAssert, assert, is the inconsistency in behaviour across libraries and languages. It's never clear whether they are active during release builds or debug only. I remember getting tricked in Java once because assert() failures were ignored by default, and only reported if you pass a special command-line option to the JVM, lol.

I would be tempted to give ETAssert a more verbose name like ETReleaseAssert or ETAlwaysAssert, but the verbosity could be annoying, not sure.

Anyway since we want most assertions to be enabled in release builds, C's assert is no use, so standardizing on ETAssert + ETDebugAssert for costly checks may be the best?
BTW I don't see a need for NSParameterAssert being a separate function, imo these can just be turned into ETAssert..?

@qmathe
Copy link
Member Author

@qmathe qmathe commented on 068d782 Apr 27, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those are bugs, and it should be trivial to fix them (change assert([self doSomething]); to bool ok = [self doSomething]; assert(ok);

Agreed.

I would be tempted to give ETAssert a more verbose name like ETReleaseAssert or ETAlwaysAssert, but the verbosity could be annoying, not sure.

It's tempting to have more explicit names I agree.

Standardizing on ETAssert + ETDebugAssert sounds good to me. ETAssert can be renamed ETReleaseAssert later on. Turning NSParameterAssert into ETAssert everywhere sounds good too.

If we do this, ETAssert must be changed to something like:

#define ETAssert(x) assert(x)

It's fine to do this, since NDEBUG doesn't control anything else beside assert() in C. This means we can safely define NDEBUG in all our build configurations. If we don'tdo this, we won't be able to use ETAssert in C functions.

Please sign in to comment.