Add a recursion blocker in Pointer/TouchInputRedirection::update
Needs RevisionPublic

Authored by graesslin on Feb 4 2018, 12:39 PM.

Details

Reviewers
romangg
Group Reviewers
KWin
Plasma
Summary

It was possible that the update method was called recursively. This
could result in unpredicted behavior and in the worst case a crash.
This was exposed by adding tooltips to the window decoration (master).
There we have the following sequence:

  1. pointer above an internal window (m_internalWindow is not null)
  2. updateDecoration is called
  3. triggers a hover enter on a decoration button
  4. this creates a Tooltip
  5. which means a new internal window is added
  6. update is called again
  7. m_internalWindow gets reset
  8. back to previous update, now m_internalWindow is null although the if before evaluated to not null

-> crash

To prevent this a recursion blocker RAII class is introduced which
calls update again if the method got entered a second time.

Thus the update method can finish operation without the assumptions
being changed while executed. The changes which are required due to
the second call will be applied once we leave the method.

Targeting 5.12 as this is a general problem, although the bug is
only in master.

BUG: 389350
FIXED-IN: 5.12.1

Test Plan

No more crash when hovering close button on DebugConsole

Diff Detail

Repository
R108 KWin
Branch
input-update-blocker-5.12
Lint
No Linters Available
Unit
No Unit Test Coverage
graesslin created this revision.Feb 4 2018, 12:39 PM
Restricted Application added a project: KWin. · View Herald TranscriptFeb 4 2018, 12:39 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
graesslin requested review of this revision.Feb 4 2018, 12:39 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 4 2018, 12:39 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 21 2018, 5:09 PM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptFeb 25 2018, 9:27 AM

Note that it gave me some error, when I tried to compile this branch.

UpdateRecursionBlocker has a generic name, but it is dependent on receiving as T one of the input classes with input as an instance and having a member function update. I would make it fully generic by adding a std::function type argument to the constructer (that in our current application encapsulates input->update()) and that gets called in the destructor just like it is now. The template parameter T would then be the template argument of the std::function argument and m_input would be replaced by a member variable m_fct storing the std::function

The class does not directly block recursions, but delays additional calls to the end of the first call (and then does the call only once). Therefore I would name it differently. Something like BlockAndDelayCallsToOne?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptFeb 25 2018, 12:04 PM
mart added a subscriber: mart.Mar 23 2018, 2:53 PM

janitor message: review process on this should start again (or abandoned if doesn't make sense anymore)

romangg requested changes to this revision.Jun 1 2018, 11:12 AM

When you have time please address my comment. I have to say though that I can't reproduce the crash when hovering over close button in Debug console.

This revision now requires changes to proceed.Jun 1 2018, 11:12 AM