Bug 111561

Created attachment 135350 [details]
Screenshot of Instruments.app showing memory leaks

A screenshot of the number of leaks occurring is enclosed

From what I understand, this relates to :

https://opensource.apple.com/source/IOHIDFamily/IOHIDFamily-700/IOHIDLib/IOHIDDeviceClass.cpp.auto.html


which is Apple's code.

From 

https://github.com/st3fan/osx-10.9/blob/master/IOHIDFamily-503.1.13/IOHIDLib/IOHIDDeviceClass.cpp


it appears to be this section of code:

void IOHIDDeviceClass::setElementDictIntValue(CFMutableDictionaryRef element, CFStringRef key, uint32_t value)
{
    CFNumberRef number = CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &value);
    
    if ( !number )
        return;

    CFDictionarySetValue(element, key, number);
    CFRelease(number);
}

and here :

CFTypeRef IOHIDDeviceClass::createElement(CFDataRef data, IOHIDElementStruct * element, uint32_t index, CFTypeRef parentElement, CFMutableDictionaryRef elementCache, bool * isElementCached, IOOptionBits options)
{
    CFTypeRef   type = 0;
    CFNumberRef key = 0;
    uint32_t    cookie = element->cookieMin + index;
    
    if ( elementCache )
    {
        key = CFNumberCreate(kCFAllocatorDefault, kCFNumberIntType, &cookie);
        
        if ( key )
        {
            type = CFDictionaryGetValue(elementCache, key);
            
            if ( type )
            {
                if (isElementCached)
                    *isElementCached = true;
                    
                CFRetain(type);
                CFRelease(key);
                    
                return type;
            }
        }
}

@Julien : not sure if you can do anything about this one ?

I don't know what can be done here since it's Apple code.
Moreover, I'm not sure there's a leak in this part of code.
Perhaps it just shows that fonts are loaded in memory when scrolling and it's kept in cache.
To test, you can scroll once the whole dropdown font list.
Then you scroll again to see if consumed memory increases.

BTW, would it be possible to have a bt of something similar to know where it's called in LO?

Perhaps this is linked to my input devices on this machine, I honestly don't know, as IOHIDDeviceClass is supposed to relate to USB and other connected hardware devices ? Whether this is due to a keyboard input (I use an Apple wireless keyboard on my test machine), or the USB mouse, or the display refresh ? No idea unfortunately.

(In reply to Julien Nabet from comment #6)


> BTW, would it be possible to have a bt of something similar to know where
> it's called in LO?

I'll have a look.

Here's the code pointer:
https://opengrok.libreoffice.org/xref/core/apple_remote/source/HIDRemoteControlDevice.m#423
success = (*handle)->copyMatchingElements(handle, NULL, (CFArrayRef*)&elements);

It seems that the problems are indeed linked to AppleRemote handling code :

	success = (*handle)->copyMatchingElements(handle, NULL, (CFArrayRef*)&elements);


line 423 of HIDRemoteCotnrolDevice.m

Created attachment 135642 [details]
Leak cycle graph for CFArray management

I am enclosing a leak cycle graph for the CFArray management code cycle

Trace of calls leading to leak :



   9 libAppleRemotelo.dylib -[HIDRemoteControlDevice(IOKitMethods) initializeCookies] /Users/Shared/LO/core/apple_remote/source/HIDRemoteControlDevice.m:423
  10 libAppleRemotelo.dylib -[HIDRemoteControlDevice startListening:] /Users/Shared/LO/core/apple_remote/source/HIDRemoteControlDevice.m:180
  11 libAppleRemotelo.dylib -[RemoteControlContainer startListening:] /Users/Shared/LO/core/apple_remote/source/RemoteControlContainer.m:114
  12 libvcllo.dylib -[VCL_NSApplication applicationWillBecomeActive:] /Users/Shared/LO/core/vcl/osx/vclnsapp.mm:504

See also lines 109-115 of RemoteControlContainer.m :

- (void) startListening: (id) sender {
#ifdef DEBUG
    NSLog(@"Apple Remote: start listening to events... ");
#endif
    for(NSUInteger i=0; i < [remoteControls count]; i++) {
        [[remoteControls objectAtIndex: i] startListening: sender];
    }

where the line :

        [[remoteControls objectAtIndex: i] startListening: sender];


is identified as a leak point.

That points to :

- (void) startListening: (id) sender {
    (void)sender;
	if ([self isListeningToRemote]) return;


at lines 147-149 in HIDRemoteControlDevice.m

I'm noticing a different leak with the the given steps (with a USB device attached): 
[NSCFTimer initWithFireDate:interval:target:selector:userInfo:repeats:]

Version: 6.0.0.0.alpha0+
Build ID: 2b78b9feab9521e614b9edae17709cb6e2001292
CPU threads: 4; OS: Mac OS X 10.12.6; UI render: default; 
TinderBox: MacOSX-x86_64@49-TDF, Branch:master, Time: 2017-08-25_05:47:15
Locale: nl-NL (nl_NL.UTF-8); Calc: group

However, I think I have seen this one too ... some while ago. I will retest it with a USB device connected..

I gave a try locally with https://gerrit.libreoffice.org/#/c/43329/
I don't reproduce the leaks with it and have no crash.
However, perhaps I missed something so first, I'll wait for MacOs Jenkins build.

Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=47039a11dae0a614213cae136fe705d32f9efa1c

tdf#111561: try to fix leak in HIDRemoteControlDevice (apple_remote)

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

Julien Nabet committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=b8a6998342fbbeb77bca354df306dd94d94d57e9

Following tdf#111561: no need to autorelease "elements" (apple_remote)

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.

(In reply to Commit Notification from comment #17)


> 
> Affected users are encouraged to test the fix and report feedback.

Hmm, doesn't seem to change anything on my locally built master:

Version: 6.0.0.0.alpha0+
Build ID: 643e9001bff137b6e5a8784d9e1f25a51e0d1644
CPU threads: 4; OS: Mac OS X 10.13; UI render: default; 
Locale: fr-FR (fr_FR.UTF-8); Calc: group

Thank you for your feedback Alex.
No idea then :-(

So, what I'm currently seeing is :

	success = (*handle)->copyMatchingElements(handle, NULL, (CFArrayRef*)&elements);

			

at line 423 of HIDRemoteControlDevice.m

(In reply to Alex Thurgood from comment #21)
> So, what I'm currently seeing is :
> 
> 	success = (*handle)->copyMatchingElements(handle, NULL,
> (CFArrayRef*)&elements);
> 
> 			
> 
> at line 423 of HIDRemoteControlDevice.m

So the bit before the above line is :

- (BOOL) initializeCookies {
	IOHIDDeviceInterface122** handle = (IOHIDDeviceInterface122**)hidDeviceInterface;
	IOHIDElementCookie		cookie;
	long					usage;
	long					usagePage;
	id						object;
	NSArray*				elements = nil;
	NSDictionary*			element;
	IOReturn success;

	if (!handle || !(*handle)) return NO;

	// Copy all elements, since we're grabbing most of the elements
	// for this device anyway, and thus, it's faster to iterate them
	// ourselves. When grabbing only one or two elements, a matching
	// dictionary should be passed in here instead of NULL.

and according to Apple documentation :

https://developer.apple.com/documentation/iokit/iohiddevicedeviceinterface/1395733-copymatchingelements?language=objc

the example that Apple gives appears to be slightly different to that used in our code ?

Alex: I don't understand what you mean
Apple:
IOReturn (*copyMatchingElements)(void *self, CFDictionaryRef matchingDict, CFArrayRef *pElements, IOOptionBits options);

LO code:
success = (*handle)->copyMatchingElements(handle, NULL, (CFArrayRef*)&elements);


from LO code "IOReturn success;" => OK

handle corresponds to *self => OK

NULL corresponds to matchingDict and there's none here
comments in the code:
"419 	// Copy all elements, since we're grabbing most of the elements
420 	// for this device anyway, and thus, it's faster to iterate them
421 	// ourselves. When grabbing only one or two elements, a matching
422 	// dictionary should be passed in here instead of NULL."
=> OK

NSArray* elements = nil;
but code shows "&elements" cast with (CFArrayRef*) => OK

about "options", Apple website indicates:
"Reserved for future use. Ignored in current implementation. Set to zero."
=> OK

Did I miss something?

(In reply to Julien Nabet from comment #23)


> Did I miss something?

No you didn't miss anything and thanks for the explanation - my observation was due to my lack of understanding in C++

Seeing as I couldn't bring this any further, and it looks like it is Apple's own code that might be the root cause, let's close this as NOTOURBUG.