Re: [Bibdesk-develop] More on FVWebViewIcon crashes

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 454-5900

> On Jul 15, 2016, at 20:28, Maxwell, Adam R <ada...@pn...> wrote:
> 
> 
>> On Jul 15, 2016, at 10:13, Christiaan Hofman <cmh...@gm...> wrote:
>> 
>> Interesting, this does seem to be involved. I tried implementing these two methods (well, that should be_tryRetain instead of _tryRelease), and didn’t see the crasher anymore.
>> 
>> I implemented these as follows. First, change the rc by steps of 2 instead of 1 (and bit-shift back in retainCount). When deallocating (why does spell check want defecating?), set the rc to 1 to flag it as deallocating. In _tryRetain, return NO when this flag is set, otherwise increment (by 2) and return YES. As for _isDeallocating, return the “1” bit flag.
> 
> Well I’ll be. Nice job of finding that! Apparently NSObject is in the runtime now, and open source.
> 
> https://opensource.apple.com/source/objc4/objc4-680/runtime/NSObject.mm
> 
>> 
>> So it does seem to be the case that objc messes with memory management behind our backs, using undocumented methods. Very bad behavior. I think weak references use _tryRetain, then thinks it retained, but in fact didn’t, because it wasn’t using our rc ivar but rather the unused default mechanism. So the retain count is now one lower than it should be.
> 
> I wonder if it’s NSNotificationCenter doing something wrong vs. objc itself.

I get more and more convinced that it’s about weak references, so basically objc itself, though NSNotificationCenter in particular uses it.

> I’ve not been reading release notes for the last couple of releases, so missed this:
> 
> https://developer.apple.com/library/mac/releasenotes/MacOSX/WhatsNewInOSX/Articles/MacOSX10_11.html#//apple_ref/doc/uid/TP40016227-SW1
> 
> "NSNotificationCenter and NSDistributedNotificationCenter no longer send notifications to registered observers that may be deallocated. If the observer is able to be stored as a zeroing-weak reference the underlying storage stores the observer as a zeroing weak reference. Alternatively, if the object cannot be stored weakly (because it has a custom retain/release mechanism that would prevent the runtime from being able to store the object weakly) the object is stored as a non-weak zeroing reference. This means that observers are not required to un-register in their deallocation method.”
> 
> I’m really disappointed in Apple if this is broken. I don’t care about inline refcounts that much, but it seems like they’re building a really fragile house here. 
> 

And it really seems to be the case. Indeed, making something as fundamental as memory management dependent on undocumented methods is worse than fragile, it’s asking for trouble. 

> Looking at the zombie FVWebViewIcon and some other FVWebViewIcon, the history is pretty different, so it’s likely taking a completely different code path that Apple hasn’t really tested: there's no retain/release from NSNotificationCenter when FVObject’s implementation is commented out. 
> 
> *** History with FVObject retain/release ***
> 
> #	Event Type	∆ RefCt	RefCt	Timestamp	Responsible Library	Responsible Caller
> 0	Malloc	+1	1	00:07.490.521	FileView	+[FVIcon allocWithZone:]
> 1	Retain	+1	2	00:07.490.543	FileView	-[_FVController _cachedIconForURL:]
> 2	Release	-1	1	00:07.490.545	FileView	-[_FVController _cachedIconForURL:]
> 3	Retain	+1	2	00:07.490.545	FileView	-[_FVController reload]
> 4	Retain	+1	3	00:07.511.173	FileView	-[_FVController _recacheIconsInBackgroundIfNeeded]
> 5	Release	-1	2	00:07.540.340	AppKit	loadNib
> 6	Retain	+1	3	00:08.112.437	FileView	-[_FVController iconsAtIndexes:]
> 7	Retain	+1	4	00:08.112.595	FileView	-[FVIconOperation initWithIcon:view:]
> 8	Retain	+1	5	00:08.123.952	Foundation	-[NSObject(NSThreadPerformAdditions) performSelector:onThread:withObject:waitUntilDone:modes:]
> 9	Release	-1	4	00:09.883.125	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 10	Release	-1	3	00:11.705.736	Foundation	__NSThreadPerformPerform
> 11	Retain	+1	4	00:11.911.026	FileView	-[_FVController iconsAtIndexes:]
> 12	Release	-1	3	00:11.915.088	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 13	Retain	+1	4	00:12.200.528	FileView	-[_FVController iconsAtIndexes:]
> 14	Release	-1	3	00:12.226.420	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 15	Retain	+1	4	00:12.232.692	Foundation	-[NSNotificationCenter postNotificationName:object:userInfo:]
> 16	Release	-1	2	00:12.293.897	Foundation	-[NSNotificationCenter postNotificationName:object:userInfo:]
> 17	Retain	+1	3	00:12.318.457	Foundation	-[NSNotificationCenter postNotificationName:object:userInfo:]
> 18	Release	-1	1	00:12.334.116	Foundation	-[NSNotificationCenter postNotificationName:object:userInfo:]
> 19	Retain	+1	2	00:12.340.832	Foundation	-[NSNotificationCenter postNotificationName:object:userInfo:]
> 20	Retain	+1	2	00:12.351.941	WebKitLegacy	WebDocumentLoaderMac::setDataSource(WebDataSource*, WebView*)
> 21	Release	-1	1	00:12.358.489	Foundation	-[NSNotificationCenter postNotificationName:object:userInfo:]
> 22	Release	-1	0	00:12.445.285	WebKitLegacy	WebDocumentLoaderMac::decreaseLoadCount(unsigned long)
> 23	Retain	+1	2	00:12.445.480	Foundation	-[NSObject(NSThreadPerformAdditions) performSelector:onThread:withObject:waitUntilDone:modes:]
> 24	Retain	+1	2	00:12.445.915	FileView	-[FVIconOperation initWithIcon:view:]
> 25	Release	-1	1	00:12.446.475	FileView	-[FVIconOperation dealloc]
> 26	Zombie		-1	00:12.511.403	FileView	-[FVIconOperation hash]
> 
> *** History without FVObject retain/release ***
> 
> #	Event Type	∆ RefCt	RefCt	Timestamp	Responsible Library	Responsible Caller
> 0	Malloc	+1	1	00:04.309.172	FileView	+[FVIcon allocWithZone:]
> 1	Retain	+1	2	00:04.311.198	FileView	-[_FVController _cachedIconForURL:]
> 2	Release	-1	1	00:04.311.200	FileView	-[_FVController _cachedIconForURL:]
> 3	Retain	+1	2	00:04.311.201	FileView	-[_FVController reload]
> 4	Retain	+1	3	00:04.334.238	FileView	-[_FVController _recacheIconsInBackgroundIfNeeded]
> 5	Release	-1	2	00:04.361.338	AppKit	loadNib
> 6	Retain	+1	3	00:04.565.832	FileView	-[_FVController iconsAtIndexes:]
> 7	Retain	+1	4	00:04.565.909	FileView	-[FVIconOperation initWithIcon:view:]
> 8	Retain	+1	5	00:04.575.665	Foundation	-[NSObject(NSThreadPerformAdditions) performSelector:onThread:withObject:waitUntilDone:modes:]
> 9	Release	-1	4	00:05.250.591	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 10	Retain	+1	5	00:05.993.302	WebKitLegacy	WebDocumentLoaderMac::setDataSource(WebDataSource*, WebView*)
> 11	Release	-1	4	00:06.000.769	Foundation	__NSThreadPerformPerform
> 12	Retain	+1	5	00:07.187.582	FileView	-[_FVController iconsAtIndexes:]
> 13	Release	-1	4	00:07.190.859	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 14	Release	-1	3	00:07.904.926	WebKitLegacy	WebDocumentLoaderMac::decreaseLoadCount(unsigned long)
> 15	Retain	+1	4	00:07.930.562	FileView	-[FVIconOperation initWithIcon:view:]
> 16	Release	-1	3	00:07.932.780	FileView	-[FVIconOperation dealloc]
> 17	Retain	+1	4	00:07.957.346	FileView	-[_FVController iconsAtIndexes:]
> 18	Release	-1	3	00:07.980.658	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 19	Retain	+1	4	00:08.274.495	FileView	-[_FVController iconsAtIndexes:]
> 20	Release	-1	3	00:08.287.621	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 21	Release	-1	2	00:08.289.721	FileView	-[FVIconOperation dealloc]
> 22	Retain	+1	3	00:08.301.759	FileView	-[_FVController iconsAtIndexes:]
> 23	Release	-1	2	00:08.333.803	QuartzCore	CA::Transaction::observer_callback(__CFRunLoopObserver*, unsigned long, void*)
> 
> 
>> 
>> BTW, do you know the __sync_fetch_and_add and related built-in functions? Are they available on 10.6? Or perhaps they are compiler specific?
> 
> Those look like gcc builtins. The OSAtomic functions might use them under the hood?
> 
> — adam
> 

It may well be, they basically do the same, but with variable types, and a different order for the arguments.

I ask this because an Apple engineer passed me this implementation from the objc runtime:

/// Excerpted from the objc runtime

// API to only be called by classes that provide their own reference count storage

OBJC_EXPORT
void
_objc_deallocOnMainThreadHelper(void *context)
    OBJC_AVAILABLE(10.7, 5.0, 9.0, 1.0);

// On async versus sync deallocation and the _dealloc2main flag
//
// Theory:
//
// If order matters, then code must always: [self dealloc].
// If order doesn't matter, then always async should be safe.
//
// Practice:
//
// The _dealloc2main bit is set for GUI objects that may be retained by other
// threads. Once deallocation begins on the main thread, doing more async
// deallocation will at best cause extra UI latency and at worst cause
// use-after-free bugs in unretained delegate style patterns. Yes, this is
// extremely fragile. Yes, in the long run, developers should switch to weak
// references.
//
// Note is NOT safe to do any equality check against the result of
// dispatch_get_current_queue(). The main thread can and does drain more than
// one dispatch queue. That is why we call pthread_main_np().
//

typedef enum {
    _OBJC_RESURRECT_OBJECT = -1,        /* _logicBlock has called -retain, and scheduled a -release for later. */
    _OBJC_DEALLOC_OBJECT_NOW = 1,       /* call [self dealloc] immediately. */
    _OBJC_DEALLOC_OBJECT_LATER = 2      /* call [self dealloc] on the main queue. */
} _objc_object_disposition_t;

#define _OBJC_SUPPORTED_INLINE_REFCNT_LOGIC_BLOCK(_rc_ivar, _logicBlock)        \
    -(id)retain {                                                               \
        /* this will fail to compile if _rc_ivar is an unsigned type */         \
        int _retain_count_ivar_must_not_be_unsigned[0L - (__typeof__(_rc_ivar))-1] __attribute__((unused)); \
        __typeof__(_rc_ivar) _prev = __sync_fetch_and_add(&_rc_ivar, 2);        \
        if (_prev < -2) { /* specifically allow resurrection from logical 0. */ \
            __builtin_trap(); /* BUG: retain of over-released ref */            \
        }                                                                       \
        return self;                                                            \
    }                                                                           \
    -(oneway void)release {                                                     \
        __typeof__(_rc_ivar) _prev = __sync_fetch_and_sub(&_rc_ivar, 2);        \
        if (_prev > 0) {                                                        \
            return;                                                             \
        } else if (_prev < 0) {                                                 \
            __builtin_trap(); /* BUG: over-release */                           \
        }                                                                       \
        _objc_object_disposition_t fate = _logicBlock(self);                    \
        if (fate == _OBJC_RESURRECT_OBJECT) {                                   \
            return;                                                             \
        }                                                                       \
        /* mark the object as deallocating. */                                  \
        if (!__sync_bool_compare_and_swap(&_rc_ivar, -2, 1)) {                  \
            __builtin_trap(); /* BUG: dangling ref did a retain */              \
        }                                                                       \
        if (fate == _OBJC_DEALLOC_OBJECT_NOW) {                                 \
            [self dealloc];                                                     \
        } else if (fate == _OBJC_DEALLOC_OBJECT_LATER) {                        \
            dispatch_barrier_async_f(dispatch_get_main_queue(), self,           \
                _objc_deallocOnMainThreadHelper);                               \
        } else {                                                                \
            __builtin_trap(); /* BUG: bogus fate value */                       \
        }                                                                       \
    }                                                                           \
    -(NSUInteger)retainCount {                                                  \
        return (_rc_ivar + 2) >> 1;                                             \
    }                                                                           \
    -(BOOL)_tryRetain {                                                         \
        __typeof__(_rc_ivar) _prev;                                             \
        do {                                                                    \
            _prev = _rc_ivar;                                                   \
            if (_prev & 1) {                                                    \
                return 0;                                                       \
            } else if (_prev == -2) {                                           \
                return 0;                                                       \
            } else if (_prev < -2) {                                            \
                __builtin_trap(); /* BUG: over-release elsewhere */             \
            }                                                                   \
        } while ( ! __sync_bool_compare_and_swap(&_rc_ivar, _prev, _prev + 2)); \
        return 1;                                                               \
    }                                                                           \
    -(BOOL)_isDeallocating {                                                    \
        if (_rc_ivar == -2) {                                                   \
            return 1;                                                           \
        } else if (_rc_ivar < -2) {                                             \
            __builtin_trap(); /* BUG: over-release elsewhere */                 \
        }                                                                       \
        return _rc_ivar & 1;                                                    \
    }

#define _OBJC_SUPPORTED_INLINE_REFCNT_LOGIC(_rc_ivar, _dealloc2main)            \
    _OBJC_SUPPORTED_INLINE_REFCNT_LOGIC_BLOCK(_rc_ivar, (^(id _self_ __attribute__((unused))) { \
        if (_dealloc2main && !pthread_main_np()) {                              \
            return _OBJC_DEALLOC_OBJECT_LATER;                                  \
        } else {                                                                \
            return _OBJC_DEALLOC_OBJECT_NOW;                                    \
        }                                                                       \
    }))

#define _OBJC_SUPPORTED_INLINE_REFCNT(_rc_ivar) _OBJC_SUPPORTED_INLINE_REFCNT_LOGIC(_rc_ivar, 0)
#define _OBJC_SUPPORTED_INLINE_REFCNT_WITH_DEALLOC2MAIN(_rc_ivar) _OBJC_SUPPORTED_INLINE_REFCNT_LOGIC(_rc_ivar, 1)

What would you think the best way to go would be? Either remove FVObject, or reimplement it with something like the above?

Christiaan

Re: [Bibdesk-develop] More on FVWebViewIcon crashes

Bibliography manager for Mac OS X

Re: [Bibdesk-develop] More on FVWebViewIcon crashes