Bug 111561 - Apple Remote code in HIDRemoteControlDevice.m causes memory leak in IOHIDDeviceClass::setElementDictIntValue()
Summary: Apple Remote code in HIDRemoteControlDevice.m causes memory leak in IOHIDDevi...
Status: RESOLVED NOTOURBUG
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: framework (show other bugs)
Version:
(earliest affected)
6.0.0.0.alpha0+
Hardware: x86-64 (AMD64) Mac OS X (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-09 15:33 UTC by Alex Thurgood
Modified: 2018-10-24 06:32 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Screenshot of Instruments.app showing memory leaks (83.77 KB, image/png)
2017-08-09 15:34 UTC, Alex Thurgood
Details
Leak cycle graph for CFArray management (93.44 KB, image/png)
2017-08-18 08:30 UTC, Alex Thurgood
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Thurgood 2017-08-09 15:33:06 UTC
Description:
When invoked

IOHIDDeviceClass::setElementDictIntValue() 

leaks memory quite substantially, as does :

IOHIDDeviceClass::createElement()

and these are invoked multiple times.


Steps to Reproduce:
1. Start XCode, then Instruments.app
2. Choose Memory Leak profile tool
3. Select LibreOffice.app in instdir as target process
4. Click on the record button, LODev is started by the profiling tool
5. Wait for the StartCenter to load - note the occurrences of memory leaks as they occur.
6. Open a new Writer document.
7. When the document has loaded, click on the dropdown font list. Scroll up and down the list with the Up/Down cursor arrows
8. Stop recording.
9. Analyse the profile trace.

Actual Results:  
It leaks memory

Expected Results:
It shouldn't leak memory


Reproducible: Always

User Profile Reset: No

Additional Info:
The responsible code seems to be here :


+0x09	pushq               %rax
+0x0a	movq                %rdx, %r14
+0x0d	movq                %rsi, %r15
+0x10	movl                %ecx, -28(%rbp)
+0x13	movq                37794(%rip), %rax
+0x1a	movq                (%rax), %rdi
+0x1d	leaq                -28(%rbp), %rdx
+0x21	movl                $9, %esi
+0x26	callq               "DYLD-STUB$$CFNumberCreate"
+0x2b	movq                %rax, %rbx
+0x2e	testq               %rbx, %rbx
+0x31	je                  "IOHIDDeviceClass::setElementDictIntValue(__CFDictionary*, __CFString const*, unsigned int)+0x49"
+0x33	movq                %r15, %rdi
+0x36	movq                %r14, %rsi
+0x39	movq                %rbx, %rdx
+0x3c	callq               "DYLD-STUB$$CFDictionarySetValue"
+0x41	movq                %rbx, %rdi
+0x44	callq               "DYLD-STUB$$CFRelease"
+0x49	addq                $8, %rsp
+0x4d	popq                %rbx
+0x4e	popq                %r14
+0x50	popq                %r15
+0x52	popq                %rbp
+0x53	retq



User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:54.0) Gecko/20100101 Firefox/54.0
Comment 1 Alex Thurgood 2017-08-09 15:34:42 UTC
Created attachment 135350 [details]
Screenshot of Instruments.app showing memory leaks

A screenshot of the number of leaks occurring is enclosed
Comment 2 Alex Thurgood 2017-08-09 15:38:43 UTC
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.
Comment 3 Alex Thurgood 2017-08-09 15:41:37 UTC
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);
}
Comment 4 Alex Thurgood 2017-08-09 15:47:13 UTC
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;
            }
        }
}
Comment 5 Alex Thurgood 2017-08-09 16:09:56 UTC
@Julien : not sure if you can do anything about this one ?
Comment 6 Julien Nabet 2017-08-09 17:13:26 UTC
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?
Comment 7 Alex Thurgood 2017-08-10 07:04:16 UTC
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.
Comment 8 Alex Thurgood 2017-08-10 07:05:57 UTC
(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.
Comment 9 Julien Nabet 2017-08-12 11:16:25 UTC
Here's the code pointer:
https://opengrok.libreoffice.org/xref/core/apple_remote/source/HIDRemoteControlDevice.m#423
success = (*handle)->copyMatchingElements(handle, NULL, (CFArrayRef*)&elements);
Comment 10 Alex Thurgood 2017-08-18 08:25:52 UTC
It seems that the problems are indeed linked to AppleRemote handling code :

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


line 423 of HIDRemoteCotnrolDevice.m
Comment 11 Alex Thurgood 2017-08-18 08:30:39 UTC
Created attachment 135642 [details]
Leak cycle graph for CFArray management

I am enclosing a leak cycle graph for the CFArray management code cycle
Comment 12 Alex Thurgood 2017-08-18 08:38:48 UTC
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
Comment 13 Alex Thurgood 2017-08-18 08:42:26 UTC
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.
Comment 14 Alex Thurgood 2017-08-18 08:44:45 UTC
That points to :

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


at lines 147-149 in HIDRemoteControlDevice.m
Comment 15 Telesto 2017-08-29 07:16:11 UTC
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..
Comment 16 Julien Nabet 2017-10-11 17:13:30 UTC
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.
Comment 17 Commit Notification 2017-10-11 18:09:57 UTC
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.
Comment 18 Commit Notification 2017-10-12 05:42:52 UTC
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.
Comment 19 Alex Thurgood 2017-10-17 12:40:39 UTC
(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
Comment 20 Julien Nabet 2017-10-17 12:42:06 UTC
Thank you for your feedback Alex.
No idea then :-(
Comment 21 Alex Thurgood 2017-10-17 13:31:44 UTC
So, what I'm currently seeing is :

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

			

at line 423 of HIDRemoteControlDevice.m
Comment 22 Alex Thurgood 2017-10-17 13:46:28 UTC
(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 ?
Comment 23 Julien Nabet 2017-10-17 19:24:03 UTC
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?
Comment 24 Alex Thurgood 2017-10-23 12:59:59 UTC
(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++
Comment 25 QA Administrators 2018-10-24 02:56:09 UTC Comment hidden (obsolete)
Comment 26 Alex Thurgood 2018-10-24 06:32:53 UTC
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.