Skip to content

[gui] reenable fit panel testing and fix nullptr access#21882

Open
ferdymercury wants to merge 4 commits intoroot-project:masterfrom
ferdymercury:patch-20
Open

[gui] reenable fit panel testing and fix nullptr access#21882
ferdymercury wants to merge 4 commits intoroot-project:masterfrom
ferdymercury:patch-20

Conversation

@ferdymercury
Copy link
Copy Markdown
Collaborator

This Pull request:

Changes or fixes:

initialize to null instead

Fixes #21881

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

@ferdymercury ferdymercury changed the title [asimage] prevent pointing to invalid memory [gui,asimage] prevent pointing to invalid memory Apr 10, 2026
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 10, 2026

Test Results

    22 files      22 suites   3d 10h 11m 32s ⏱️
 3 834 tests  3 831 ✅  1 💤  2 ❌
75 673 runs  75 634 ✅ 18 💤 21 ❌

For more details on these failures, see this check.

Results for commit 522f4c3.

♻️ This comment has been updated with latest results.

if (values) {
fValues = *values;
fContext = gVirtualX->CreateGC(gVirtualX->GetDefaultRootWindow(), values);
if (gVirtualX)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking of gVirtualX does not make big sense - it always present, also in batch mode

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was null when i checked with the debugger. Note that TVirtualX::Instance can still return nullptr when on batch

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For changes in GUI classes we need ok from Bertrand @bellenot.
Can we combine this PR with enabling of gui test: #21891
Then we directly can see if our changes works/fails?

And finally we will be able to decide if such extra checks in GUI classes allows to use them in batch GUI tests.

@ferdymercury ferdymercury marked this pull request as ready for review April 13, 2026 08:55
@linev
Copy link
Copy Markdown
Member

linev commented Apr 13, 2026

I checkout your branch and try to run test.
There are many issues with normal GUI mode.

  1. Last two tests TestTree2D and TestTreeND do not work for me - while fit panel does not have entries like "gaus2d" or "gausND".
  2. Also running test in batch possible only with root -b --web=off UnitTesting.cxx. Produced test executable crashed much earlier because TGClient::Instance() returns nullptr
  3. When run inside root, one sees lot of errors about missing images.

@ferdymercury
Copy link
Copy Markdown
Collaborator Author

  • Last two tests TestTree2D and TestTreeND do not work for me - while fit panel does not have entries like "gaus2d" or "gausND".

Yes, same here, I had deactivated (commented) locally those old tests for the moment and was focusing on whether it was running at all without crashing.

  • Also running test in batch possible only with root -b --web=off UnitTesting.cxx. Produced test executable crashed much earlier because TGClient::Instance() returns nullptr

Weird, where? I see a different behavior on Linux. It just crashes when deleting the TGTooltip at shutdown for me.

  • When run inside root, one sees lot of errors about missing images.

Same here, that's what I would say "expected", a lot of errors in batch mode of not being able to load icons, but no crashes.

@linev
Copy link
Copy Markdown
Member

linev commented Apr 14, 2026

Weird, where? I see a different behavior on Linux. It just crashes when deleting the TGTooltip at shutdown for me.

I mean when I run compiled executable.

  • When run inside root, one sees lot of errors about missing images.

Same here, that's what I would say "expected", a lot of errors in batch mode of not being able to load icons, but no crashes.

This is most challenging part - try to understand if there double deletion or just wrong pointer released.
Because of lot of errors I cannot exclude incomplete initialization of some GUI classes

@linev
Copy link
Copy Markdown
Member

linev commented Apr 14, 2026

@ferdymercury @bellenot

I probably found the reason why it crashes.
Seems to be it is wrongly set flag TGLabel::fHasOwnFont.
In batch it is incorrect and TGLabel destructor tries to free TGGC object which does not belong to the label.

Also side problem is that in TGGC::TGGC(GCValues_t *values) constructor ref counter not always set correctly.

Fixing this two problems makes test running for me

@linev
Copy link
Copy Markdown
Member

linev commented Apr 14, 2026

First step is #21907

@ferdymercury
Can you extract your changes in TASImage to separate PR.

After such two steps we will be able to enable this and may be other GUI tests in CI

@ferdymercury ferdymercury changed the title [gui,asimage] prevent pointing to invalid memory [gui] reenable fit panel testing and fix nullptr access Apr 14, 2026
Comment on lines +83 to +84
if (gVirtualX)
gVirtualX->ChangeWindowAttributes(fId, &wattr);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably these changes are no longer needed since I was trying out different things and you already found the culprit in TGGC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[gui,asimage] crash when opening xpm icon in batch mode

2 participants