Skip to content

Commit b0bdbac

Browse files
committed
Fix crash when importing a device with embedded samples
- The importer streamed the settings file in one pass, so a Sampler tried to load its nahd:// samples before the embedded <Data> blocks (which follow the <Device> element in the file) had been extracted. The unresolved path failed to open and loadSample threw across the QML boundary, crashing the app. This only happened without a project loaded first, since loading a project already populates the extracted-data map. - Extract embedded data before deserializing the device, mirroring the project load path, and guard device deserialization so a failed sample load fails gracefully instead of crashing. Add a regression test with a reader that rejects unresolved nahd:// paths.
1 parent bcd1b6f commit b0bdbac

4 files changed

Lines changed: 92 additions & 3 deletions

File tree

CHANGELOG

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ New features:
4040

4141
Bug fixes:
4242

43+
* Fix crash when importing a device with embedded samples
44+
- Embedded data is now extracted before the device is deserialized, so a
45+
Sampler can resolve its sample paths while loading
46+
- A failed sample load during import now fails gracefully instead of crashing
47+
4348
* Fix rack Delay effect mode selector (Mono/Ping Pong/Tape had no effect)
4449

4550
* Fix rack Delay effect manual (non-sync) time knob clamping at ~11 ms

src/application/service/device_service.cpp

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -596,7 +596,22 @@ bool DeviceService::importDeviceSettings(int slotIndex, const QString & filePath
596596
if (!file.open(QIODevice::ReadOnly)) {
597597
return false;
598598
}
599-
NahdXmlReader reader { file };
599+
const auto xml = QString::fromUtf8(file.readAll());
600+
601+
// Extract embedded data before deserializing the device so that a Sampler can resolve its
602+
// nahd:// sample paths while loading. In the file the <Device> element precedes the <Data>
603+
// blocks, so a single streaming pass would try to load samples before they are extracted.
604+
// extractData() appends to the already-extracted data (it does not clear), so any embedded
605+
// samples of a currently loaded project are preserved.
606+
NahdXmlReader dataReader { xml };
607+
while (!dataReader.atEnd()) {
608+
if (dataReader.isStartElement() && dataReader.name() == Constants::NahdXml::xmlKeyData()) {
609+
m_dataService->extractData(dataReader);
610+
}
611+
dataReader.readNext();
612+
}
613+
614+
NahdXmlReader reader { xml };
600615
return importDeviceSettings(slotIndex, reader);
601616
}
602617

@@ -629,8 +644,16 @@ bool DeviceService::importDeviceSettings(int slotIndex, ProjectReader & reader)
629644
}
630645

631646
if (dev) {
632-
dev->deserializeFromXml(reader);
633-
dev->setId(static_cast<size_t>(slotIndex));
647+
try {
648+
dev->deserializeFromXml(reader);
649+
dev->setId(static_cast<size_t>(slotIndex));
650+
} catch (const std::exception & e) {
651+
// Deserialization can throw (e.g. a Sampler failing to load a sample). This is
652+
// invoked from QML, so swallow the exception here to fail gracefully instead of
653+
// crossing the C++/QML boundary and crashing.
654+
juzzlin::L(TAG).error() << std::format("Failed to import device settings: {}", e.what());
655+
return false;
656+
}
634657
} else {
635658
reader.skipCurrentElement();
636659
}

src/unit_tests/device_service_test/device_service_test.cpp

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -110,6 +110,20 @@ class MockAudioFileReader : public AudioFileReader
110110
QByteArray m_writtenData;
111111
};
112112

113+
// Simulates a real reader (e.g. SndFileReader) that cannot open an unresolved embedded path.
114+
// Used to prove that embedded data is extracted before samples are loaded during import.
115+
class NahdFailingMockAudioFileReader : public MockAudioFileReader
116+
{
117+
public:
118+
bool open(const std::string & path, Mode mode, Info & info) override
119+
{
120+
if (QString::fromStdString(path).startsWith(Constants::NahdXml::embeddedDataPathPrefix())) {
121+
return false;
122+
}
123+
return MockAudioFileReader::open(path, mode, info);
124+
}
125+
};
126+
113127
void DeviceServiceTest::initTestCase()
114128
{
115129
EffectFactory::init();
@@ -300,6 +314,52 @@ void DeviceServiceTest::test_exportImport_withEmbeddedData_shouldWork()
300314
QCOMPARE(resolvedFile.size(), 100 * 2 * sizeof(float));
301315
}
302316

317+
void DeviceServiceTest::test_importDeviceSettings_embeddedData_emptySlot_shouldExtractDataBeforeLoadingSamples()
318+
{
319+
// Export a Sampler with an embedded sample.
320+
QTemporaryFile settingsFile;
321+
QVERIFY(settingsFile.open());
322+
const auto settingsPath = settingsFile.fileName();
323+
settingsFile.close();
324+
325+
{
326+
DeviceService service { std::make_shared<AudioEngine>(), std::make_shared<DataService>() };
327+
auto sampler = std::make_shared<SamplerDevice>("TestSampler", std::make_unique<MockAudioFileReader>());
328+
service.setDevice(0, sampler);
329+
330+
QTemporaryFile sampleFile { "test.wav" };
331+
QVERIFY(sampleFile.open());
332+
const auto samplePath = sampleFile.fileName();
333+
sampleFile.write(QByteArray { 800, 0 });
334+
sampleFile.close();
335+
336+
sampler->loadSample(60, samplePath.toStdString());
337+
sampler->setEmbedWaveData(true);
338+
QVERIFY(service.exportDeviceSettings(0, settingsPath));
339+
}
340+
341+
// Import into a fresh service with an empty device rack (i.e. no project loaded first).
342+
// The reader fails to open unresolved nahd:// paths, so if the embedded data were not
343+
// extracted before the device is deserialized, the sample load would throw and import
344+
// would fail. This reproduces the crash reported when importing without a loaded project.
345+
const auto dataService = std::make_shared<DataService>();
346+
DeviceService service { std::make_shared<AudioEngine>(), dataService };
347+
service.setSamplerAudioFileReaderFactory([]() {
348+
return std::make_unique<NahdFailingMockAudioFileReader>();
349+
});
350+
351+
QVERIFY(!service.device(0)); // Empty slot
352+
QVERIFY(service.importDeviceSettings(0, settingsPath));
353+
354+
const auto importedSampler = std::dynamic_pointer_cast<SamplerDevice>(service.device(0));
355+
QVERIFY(importedSampler);
356+
QVERIFY(importedSampler->sample(60));
357+
358+
const auto importedPath = QString::fromStdString(importedSampler->sample(60)->filePath);
359+
QVERIFY(importedPath.startsWith(Constants::NahdXml::embeddedDataPathPrefix()));
360+
QVERIFY(dataService->resolvePath(importedPath) != importedPath);
361+
}
362+
303363
void DeviceServiceTest::test_peekDeviceTypeInfo_synth_shouldReturnCorrectTypeInfo()
304364
{
305365
const auto audioEngine = std::make_shared<AudioEngine>();

src/unit_tests/device_service_test/device_service_test.hpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ private slots:
3333
void test_importDeviceSettings_shouldReplaceDeviceIfTypeDiffers();
3434
void test_importDeviceSettings_emptySlot_shouldCreateDeviceFromFile();
3535
void test_exportImport_withEmbeddedData_shouldWork();
36+
void test_importDeviceSettings_embeddedData_emptySlot_shouldExtractDataBeforeLoadingSamples();
3637
void test_peekDeviceTypeInfo_synth_shouldReturnCorrectTypeInfo();
3738
void test_peekDeviceTypeInfo_nonexistentFile_shouldReturnEmpty();
3839
void test_reverbSends_shouldSaveAndLoadCorrectly();

0 commit comments

Comments
 (0)