[Differential] [Requested Changes To] D2033: Clear idle timeouts when session becomes inactive

[oliverhenshaw (Oliver Henshaw)]

oliverhenshaw requested changes to this revision.
oliverhenshaw added a reviewer: oliverhenshaw.
oliverhenshaw added a comment.
This revision now requires changes to proceed.


  I may not have much pull, but I'd rather these changes were revised again before being landed. Sorry!
  
  Note that although this is the right thing to do, the bug reporter found this change did not fix their problem - https://bugs.kde.org/show_bug.cgi?id=354250#c25 . I haven't tested it myself but it makes sense that the call to erase idle timeouts will eventually make xlib calls, which will block if your xserver is blocked (which it seems to for some users when switched to another VT)

INLINE COMMENTS

> powerdevilcore.cpp:141-143
> +    // Bug 354250: Simulate user activity when session becomes inactive,
> +    // this keeps us from sending the computer to sleep when switching to an idle session.
> +    // (this is just being lazy as it will result in us clearing everything

This comment should reflect that this is actively a good choice and not just a workaround for a corner case. IIRC the policyagent will forbid actions when the session is not active so not scheduling idle timeouts is just good efficiency.

> oliverhenshaw wrote in powerdevilcore.cpp:151
> Like you said, other kidletime users might not care whether the session is active or not. This will remove all timeouts set by other kded services, right?
> 
> Just off the top of my head, Is ActionPool::unloadAllActiveActions() better here, or is that not appropriate? It's too late in the day here for me to look into that today.

Again, removeAllIdleTimeouts is too big a hammer, when  arbitrary other kded services use the same kidletime instance.

> powerdevilcore.cpp:777-780
> +    for (auto it = m_pendingResumeFromIdleActions.constBegin(), end = m_pendingResumeFromIdleActions.constEnd(); it != end; ++it) {
> +        (*it)->onWakeupFromIdle();
>      }
> +    m_pendingResumeFromIdleActions.clear();

It would be better for this set of changes to have its own review and own commit message.

Also how can this solve a crash bug? I'm speculating wildly here, but: As far as I can see, using the iterator returned by erase() should be safe. Unless there's not really an implicit null pointer check in the while loop and the compiler optimises it away? Or perhaps onWakeFromIdle() for some action ends up invalidating the iterator somehow, possibly if it re-enters another instance of onResumingFromIdle()? If so, clearing m_pendingResumeFromIdleActions is definitely wrong because there could well have been a nested call to onKIdleTimeoutReached()

Either way, the real bug is somewhere else.

REPOSITORY
  rPOWERDEVIL Powerdevil

REVISION DETAIL
  https://phabricator.kde.org/D2033

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, sebas, oliverhenshaw
Cc: sebas, oliverhenshaw, graesslin, plasma-devel, jensreuterberg, abetts
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160713/465265e7/attachment-0001.html>