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

KillTimer needs protection "if HandleAllocated then" #35

Open
User4martin opened this issue Feb 17, 2025 · 3 comments
Open

KillTimer needs protection "if HandleAllocated then" #35

User4martin opened this issue Feb 17, 2025 · 3 comments

Comments

@User4martin
Copy link

KillTimer requires a handle.

However it is called in places that may be triggered even if there is no handle yet (and the VTV or one of its parents may not even have a parent themself, and handle creation may be impossible)

Example

procedure TBaseVirtualTree.DoChange(Node: PVirtualNode);

begin
  KillTimer(Handle, ChangeTimer);

Create a VTV in code, but do not set a parent yet, then trigger a change.


I propose to protect all KillTimer calls. Introducing a new Method KillTime(ID: ...). Checking for HandleAllocated can then be done within this, and the call with the handle can be done if safe.


For reference, the issue was discovered here https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/41430

I have not tested it with this VTV here, but a glance at the code made me believe that it can happen with this VTV too.


For reference / if interested in syncing with related other VTV
https://github.com/TurboPack/VirtualTreeView/blob/master/Source/VirtualTrees.pas#L7039-L7044
has StopTimer doing exactly this.

@User4martin
Copy link
Author

Not entirely sure, but it may also apply to some "SetTimer" calls

@User4martin
Copy link
Author

User4martin commented Feb 17, 2025

And maybe also calls to Handle in

  • TBaseVirtualTree.DoSetOffsetXY
  • InvalidateColumn (and similar)
  • ToggleNode
  • ClearSelection
  • UpdateVerticalScrollBar (called eg from AddChild)

Just from a quick scan. There may be others


Also DestroyHandle kills two of the many timers, but not the others? Intention? Does the OS not kill those timers?

User4martin pushed a commit to User4martin/VirtualTreeView-Lazarus that referenced this issue Feb 17, 2025
…arent window is not visible (or no parent present) then the handle should not be requested. Issue blikblum#35
@User4martin
Copy link
Author

User4martin commented Feb 17, 2025

I made a PR #36 for the KillTimer part. The remainder needs to be addressed separately.

I don't have any apps of my own using this VTV. So I can't do much testing beyond compilation and review. I am testing the same kind of changeset against the Lazarus included VTV, and so far no issues there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant