Extract the OS event filter implementation by brainplot · Pull Request #2422 · keepassxreboot/keepassxc

brainplot · 2018-10-26T16:09:34Z

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]

droidmonkey · 2018-10-26T17:07:55Z

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 · 2018-10-26T17:28:26Z

@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!

droidmonkey · 2018-10-29T03:41:41Z

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

brainplot · 2018-10-29T10:04:19Z

@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.

droidmonkey · 2018-10-29T11:55:40Z

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

brainplot · 2018-10-29T14:02:24Z

@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 · 2018-10-29T14:37:35Z

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

brainplot · 2018-10-29T14:50:15Z

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 · 2018-10-29T15:03:15Z

None of the Qt plugin destructors is called reliably.

- 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]

droidmonkey approved these changes Oct 29, 2018

View reviewed changes

droidmonkey added this to the v2.4.0 milestone Oct 29, 2018

droidmonkey added pr: refactoring Pull request refactors code PR: Ready for Merge labels Oct 29, 2018

Extract the OS event filter implementation

f34e10f

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

brainplot deleted the extract-eventfilter branch October 30, 2018 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Extract the OS event filter implementation#2422

Extract the OS event filter implementation#2422
droidmonkey merged 1 commit intokeepassxreboot:developfrom
brainplot:extract-eventfilter

brainplot commented Oct 26, 2018

Uh oh!

droidmonkey commented Oct 26, 2018

Uh oh!

brainplot commented Oct 26, 2018

Uh oh!

droidmonkey commented Oct 29, 2018

Uh oh!

brainplot commented Oct 29, 2018 •

edited

Loading

Uh oh!

droidmonkey commented Oct 29, 2018

Uh oh!

brainplot commented Oct 29, 2018 •

edited

Loading

Uh oh!

droidmonkey commented Oct 29, 2018

Uh oh!

brainplot commented Oct 29, 2018

Uh oh!

phoerious commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

brainplot commented Oct 26, 2018

Description

Motivation and context

How has this been tested?

Types of changes

Checklist:

Uh oh!

droidmonkey commented Oct 26, 2018

Uh oh!

brainplot commented Oct 26, 2018

Uh oh!

droidmonkey commented Oct 29, 2018

Uh oh!

brainplot commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droidmonkey commented Oct 29, 2018

Uh oh!

brainplot commented Oct 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droidmonkey commented Oct 29, 2018

Uh oh!

brainplot commented Oct 29, 2018

Uh oh!

phoerious commented Oct 29, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brainplot commented Oct 29, 2018 •

edited

Loading

brainplot commented Oct 29, 2018 •

edited

Loading