Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extract the OS event filter implementation #2422

Merged
merged 1 commit into from Oct 30, 2018

Conversation

Projects
None yet
3 participants
@brainplot
Copy link
Contributor

brainplot commented Oct 26, 2018

Description

The XcbEventFilter and WinEventFilter classes have been extracted into a separate file and a single class.
The new file has then been added to the build chain.

The new class, called OSEventFilter, implements the inherited nativeEventFilter() method and a preprocessor condition picks the right test for the if statement.

Installing a native event filter has now become this:

#if defined(Q_OS_WIN) || (defined(Q_OS_UNIX) && !defined(Q_OS_MACOS))
    installNativeEventFilter(new OSEventFilter());
#endif

which is oblivious of the actual implementation. The new structure should make it easier to add new event filters for new platforms: you just add a new preprocessor case in the nativeEventFilter() implementation.

Motivation and context

The XcbEventFilter and WinEventFilter classes had basically the same implementation, except for a single line: the condition in the if statement within the nativeEventFilter() method.
This made for a lot of code duplication. In addition to that, the release build generated the following warning:

'XcbEventFilter' has no out-of-line virtual method definitions; its vtable will be emitted in every translation unit.

In order to get rid of the warning, a base class's virtual method must have an out-of-line implementation in one of the derived classes. This will make the compiler place the vtable in that translation unit.
Moreover, Application.cpp felt a bit cluttered in my humble opinion: to reach the constructor implementation I had to scroll quite a few lines down.

How has this been tested?

I performed a dev build and a release build; and ran all the tests.
Tests n.30, n.31 and n.32 failed but they failed before I made any change as well.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING document. [REQUIRED]
  • My code follows the code style of this project. [REQUIRED]
  • I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

@brainplot brainplot force-pushed the brainplot:extract-eventfilter branch from 3b37710 to 7729570 Oct 26, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 26, 2018

Now these are the kind of PRs I wanna see! Great work. Can we follow this architecture for the global hotkey detection as well?

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 26, 2018

@droidmonkey thank you!

I think I've not yet read through the code that handles the global hotkey detection; but I can look into that and see if I can spot something that can be improved!

@brainplot brainplot force-pushed the brainplot:extract-eventfilter branch 3 times, most recently from 6e5f4a3 to 8da2e15 Oct 27, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 29, 2018

Please rebase on develop, not sure what happened to the CI.

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 29, 2018

@droidmonkey sure! I'm gonna do that.

I have a little question though. Does installNativeEventFilter() take care of deleting the memory when we do this?

installNativeEventFilter(new OSEventFilter());

I looked up on the documentation and I haven't found anything related to this. Maybe I'm missing something but if the method doesn't, I think we're introducing a leak. I coded it that way because it's how it was previously coded. We could use a QScopedPointer in the Application class.

@brainplot brainplot force-pushed the brainplot:extract-eventfilter branch from 8da2e15 to ffd9630 Oct 29, 2018

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 29, 2018

The docs don't say the application owns the filter object after calling the function so I agree with a scoped pointer.

@brainplot brainplot force-pushed the brainplot:extract-eventfilter branch from ffd9630 to f34e10f Oct 29, 2018

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 29, 2018

@droidmonkey I added the scoped pointer. However, I think there's something weird going on.
I defined a test destructor like this in the OSEventFilter class:

OSEventFilter::~OSEventFilter()
{
    qWarning("Destructor was invoked.\n");
}

and the message Destructor was invoked was not being printed upon exit.

I spent some time trying to figure out why but I didn't succeed 🤔

@droidmonkey

This comment has been minimized.

Copy link
Member

droidmonkey commented Oct 29, 2018

I wouldn't worry about it since it lives the whole length of the application. Does ASAN report issues?

@brainplot

This comment has been minimized.

Copy link
Contributor Author

brainplot commented Oct 29, 2018

This is the application output:

=================================================================
==21784==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1536 byte(s) in 3 object(s) allocated from:
    #0 0x7fa8f8b87491 in __interceptor_realloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:105
    #1 0x7fa8f02da3f1  (/usr/lib/libfontconfig.so.1+0x213f1)

Direct leak of 136 byte(s) in 3 object(s) allocated from:
    #0 0x7fa8f8b88f19 in operator new[](unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cc:93
    #1 0x7fa8f6a2b398 in qstrdup(char const*) (/usr/lib/libQt5Core.so.5+0xc6398)

Indirect leak of 2336 byte(s) in 73 object(s) allocated from:
    #0 0x7fa8f8b87231 in __interceptor_calloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:95
    #1 0x7fa8f02d9ee5  (/usr/lib/libfontconfig.so.1+0x20ee5)

Indirect leak of 518 byte(s) in 33 object(s) allocated from:
    #0 0x7fa8f8acff01 in __interceptor_strdup /build/gcc/src/gcc/libsanitizer/asan/asan_interceptors.cc:405
    #1 0x7fa8f02d9c25 in FcValueSave (/usr/lib/libfontconfig.so.1+0x20c25)

Indirect leak of 144 byte(s) in 3 object(s) allocated from:
    #0 0x7fa8f8b87019 in __interceptor_malloc /build/gcc/src/gcc/libsanitizer/asan/asan_malloc_linux.cc:86
    #1 0x7fa8f02d3eae in FcLangSetCreate (/usr/lib/libfontconfig.so.1+0x1aeae)

SUMMARY: AddressSanitizer: 4670 byte(s) leaked in 115 allocation(s).
15:47:53: /home/gianluca/Projects/build-keepassxc-Desktop-Debug/src/keepassxc exited with code 1

which is what I've always got since I first started checking out the code.

@phoerious

This comment has been minimized.

Copy link
Member

phoerious commented Oct 29, 2018

None of the Qt plugin destructors is called reliably.

@droidmonkey droidmonkey merged commit 4e1d3bf into keepassxreboot:develop Oct 30, 2018

3 checks passed

CodeFactor No issues found.
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@brainplot brainplot deleted the brainplot:extract-eventfilter branch Oct 30, 2018

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- New Database Wizard [#1952]
- Advanced Search [#1797]
- Automatic update checker [#2648]
- KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739]
- Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439]
- Remove KeePassHttp support [#1752]
- CLI: output info to stderr for easier scripting [#2558]
- CLI: Add --quiet option [#2507]
- CLI: Add create command [#2540]
- CLI: Add recursive listing of entries [#2345]
- CLI: Fix stdin/stdout encoding on Windows [#2425]
- SSH Agent: Support OpenSSH for Windows [#1994]
- macOS: TouchID Quick Unlock [#1851]
- macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583]
- Linux: Prevent Klipper from storing secrets in clipboard [#1969]
- Linux: Use polling based file watching for NFS [#2171]
- Linux: Enable use of browser plugin in Snap build [#2802]
- TOTP QR Code Generator [#1167]
- High-DPI Scaling for 4k screens [#2404]
- Make keyboard shortcuts more consistent [#2431]
- Warn user if deleting referenced entries [#1744]
- Allow toolbar to be hidden and repositioned [#1819, #2357]
- Increase max allowed database timeout to 12 hours [#2173]
- Password generator uses existing password length by default [#2318]
- Improve alert message box button labels [#2376]
- Show message when a database merge makes no changes [#2551]
- Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790]
- Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.