Bug 333759

Created attachment 98282 [details]
Fix variable toolview not sync with framestack toolview

Hi, I'm looking into this bug right now. It seems the bug still exists in the latest version.

The exact steps to reproduce is as follow:
1. Debug some application
2. Pause the debugger
3. Collapse the variable list or hide the variables tool view
4. Switch to another frame, i.e. frame 1
5. Expand the variable list or show the variables tool view
6. *Important*: switch back to the same frame as in step 2

Actual Results:
Variables for frame 1 are shown

Expected Results:
Variables of the currently selected frame should be shown


The reason for this is that internally `IVariableController` keeps track of the current active frame/thread and only updates variables when active frame/thread changes. However, when variables list is hidden, the active frame/thread is not updated. Thus if the user changes frame in this case, the active frame/thread value remains what it was before the variables list is hidden. Then after making variables list visible and changing the frame again to the previous frame, `IVariableController` thinks there is not change at all and skips the update.


The attached patch fixes this issue by removing the whole tracking active frame/thread logic. This logic was introduced in 57b2d64f to make sure locals are updated only once. However, with the code evolving, other situations where update might be called with frame/thread unchanged are gone and the above is the only one left, which in fact also doesn't require such logic. Given this tracking logic makes no benefits but bugs only, I removed it in the patch which also fixes the bug.

@Aetf: I'll have a look at the patch. Could you, in the meantime, post the patch to Phabricator, properly formatted including a commit message?

https://community.kde.org/Infrastructure/Phabricator#Posting_a_Patch

I believe the patch can be simpler, without removing the "only update when active frame/thread changes" -- what about just clearing the active frame/thread in IVariableController::setAutoUpdate? Please have a look if this fixes the issue.

Posted on Phabricator now: https://phabricator.kde.org/D1351

Well, yes, it can be done by just make IVariableController::handleEvent correctly updates m_active{Frame,Thread} even if IVariableController::autoUpdate is UpdateNone. No need to touch IVariableController::setAutoUpdate though.

But I think there is duplication. Maybe my wording isn't clear, but the patch isn't removing the "only update when active frame/thread chagnes" functionality, as we only call IVariableController::update when handling IDebugSession::thread_or_frame_changed event. So no need to do the duplicate check (The update in IVariableController::setAutoUpdate handles different situations and is irrelevant here. However, I do wonder if it is needed all the time)

Besides, m_active{Frame,Thread} only mirror the same value in FrameStackModel::current{Frame,Thread} and it requires extra effort to keep them synced. So in my opinion we should remove m_active{Frame,Thread} to simplify the logic.

I understand removing them breaks binary compatibility. If that's what you are concerned, then I'm OK to change to the simpler workaround.

@Niko: Can you remember why exactly you introduced this code in 57b2d64f?

@Aetf: Please go for the simpler change, try if it works.

Note regarding "duplication": Yes, we might have a copy of thread/frame in IVariableController, but there's a reason for it: Updating the variables is expensive (lots of communication with GDB required), so we need to reduce the number of updates as much as possible. Thus we "cache" thread/frame here, not "duplicate".

(In reply to Kevin Funk from comment #7)
I am still not quite convinced (see below) but maybe the removal deserves it own patch. So, simpler fix submitted to Phabricator. 

For the "cache/duplicate" thing: I understand that we should avoid updating variables as much as possible due to communication costs. And that's what exactly the *thread_or_frame_changed* event handler for. Is there such a case that this handler is called without an actual change in active thread/frame? Given we now only check cached active thread/frame in this event handler, it looks duplicate to me.