ITunesXMLImporterTest: Fix sporadic crash due to a not initalized reference.

[daschuer]

Use a function call with a null check instead.

The failing test has happened on Launchpad:
https://launchpadlibrarian.net/672913729/buildlog_ubuntu-focal-amd64.mixxx_2.4~beta~22~ge7bb02f7a5-1~focal_BUILDING.txt.gz

[daschuer]

Finally rebased and all green. Sorry for the noise.
We should merge this soon, because our beta ppa keeps failing.

[ronso0]

because our beta ppa keeps failing

Just the Focal amd64 build fails, so to have a package ASAP for this, too, how about disabling the test just for this build? (if that's possible) To not merge this in a rush..

[Swiftb0y]

humbly requesting @fwcd opinion on this refactoring.

[fwcd]

Some quick notes from glancing over the changes, I'll see if I can do a more thorough review over the next few days

[fwcd]

I haven't dug into the test log, so I am not sure if I understand the problem this fixes yet, but if there is some unsafe reference in there, could we perhaps fix the bug in isolation before refactoring it?

[daschuer]

because our beta ppa keeps failing

Just the Focal amd64 build fails, so to have a package ASAP for this, too, how about disabling the test just for this build? (if that's possible) To not merge this in a rush..

OK, I went a head and disabled the test in a18b0b9 as a direct 2.4 commit.

Now all PPA builds are succeeding: https://launchpadlibrarian.net/673323201/buildlog_ubuntu-focal-amd64.mixxx_2.4~beta~26~ga18b0b9269-1~focal_BUILDING.txt.gz

[daschuer]

if there is some unsafe reference in there, could we perhaps fix the bug in isolation before refactoring it?

OK, due to disabling the test the pressure is gone and we may go for a refactoring.

The original root cause for the crash was that the const std::atomic<bool>& m_cancelImport; had random values.
During the fix I have read the suggestion for refactoring and tried to follow it make m_cancelImport a value type in ITunesImport. This however did not work good, because it is used in ImporterImpl keeping the cyclic dependency.
In addition the cyclic dependency is used anyway in TreeItem::newRoot(m_pParentFeature);

IMHO this solution is now on a pragmatic middle ground with a function call utilizing the already existing pointer after a null check.

[fwcd]

the const std::atomic<bool>& m_cancelImport; had random values.

How does this happen? Do members in C++ not initialize (deterministically) before methods can be called or does the test framework do some magic there that breaks this?

mixxx/src/test/itunesxmlimportertest.cpp

Line 30 in a5ae80a

std::atomic<bool> cancelImport;

My understanding was that this would (safely) initialize cancelImport to false.

[daschuer]

I have also no explanation why it is not initialized during tests.

[Swiftb0y]

I have also no explanation why it is not initialized during tests.

Apparently thats legal, even though many compilers still initialized it. https://stackoverflow.com/questions/36320008/whats-the-default-value-for-a-stdatomic

[daschuer]

Done

[Swiftb0y]

Thank you. Should be good now after the build issues are fixed.