From ce1756e36f8e70e4f1c10df0f5735be22eb6259c Mon Sep 17 00:00:00 2001 From: Zach Nelson Date: Thu, 16 Apr 2026 19:42:05 -0500 Subject: [PATCH] refactor: Added shared XML parser teardown helper (#1438) ## Summary **What is the goal of this PR?** Added `destroyXmlParser()` helper to replace the repeated 4-line parser cleanup block (stop, clear callbacks, free, null) that was copyied across 6 XML parser files. --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? _**PARTIALLY**_ --- .../Epub/parsers/ChapterHtmlSlimParser.cpp | 29 +++++++------------ lib/Epub/Epub/parsers/ContainerParser.cpp | 12 +++----- lib/Epub/Epub/parsers/ContentOpfParser.cpp | 21 +++----------- lib/Epub/Epub/parsers/TocNavParser.cpp | 23 +++------------ lib/Epub/Epub/parsers/TocNcxParser.cpp | 23 +++------------ lib/OpdsParser/OpdsParser.cpp | 21 +++++--------- lib/XmlParserUtils/XmlParserUtils.h | 13 +++++++++ 7 files changed, 48 insertions(+), 94 deletions(-) create mode 100644 lib/XmlParserUtils/XmlParserUtils.h diff --git a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp index 6245d2ae4..9a957d616 100644 --- a/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp +++ b/lib/Epub/Epub/parsers/ChapterHtmlSlimParser.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include "../../Epub.h" @@ -996,7 +997,7 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() { paragraphAlignmentBlockStyle.alignment = align; startNewTextBlock(paragraphAlignmentBlockStyle); - const XML_Parser parser = XML_ParserCreate(nullptr); + XML_Parser parser = XML_ParserCreate(nullptr); int done; if (!parser) { @@ -1010,7 +1011,7 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() { FsFile file; if (!Storage.openFileForRead("EHP", filepath, file)) { - XML_ParserFree(parser); + destroyXmlParser(parser); return false; } @@ -1029,10 +1030,8 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() { void* const buf = XML_GetBuffer(parser, PARSE_BUFFER_SIZE); if (!buf) { LOG_ERR("EHP", "Couldn't allocate memory for buffer"); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); + destroyXmlParser(parser); + file.close(); return false; } @@ -1040,10 +1039,8 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() { if (len == 0 && file.available() > 0) { LOG_ERR("EHP", "File read error"); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); + destroyXmlParser(parser); + file.close(); return false; } @@ -1052,19 +1049,15 @@ bool ChapterHtmlSlimParser::parseAndBuildPages() { if (XML_ParseBuffer(parser, static_cast(len), done) == XML_STATUS_ERROR) { LOG_ERR("EHP", "Parse error at line %lu:\n%s", XML_GetCurrentLineNumber(parser), XML_ErrorString(XML_GetErrorCode(parser))); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); + destroyXmlParser(parser); + file.close(); return false; } } while (!done); LOG_DBG("EHP", "Time to parse and build pages: %lu ms", millis() - chapterStartTime); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); + destroyXmlParser(parser); + file.close(); // Process last page if there is still text if (currentTextBlock) { diff --git a/lib/Epub/Epub/parsers/ContainerParser.cpp b/lib/Epub/Epub/parsers/ContainerParser.cpp index aa7a92a0b..0ce90a21b 100644 --- a/lib/Epub/Epub/parsers/ContainerParser.cpp +++ b/lib/Epub/Epub/parsers/ContainerParser.cpp @@ -1,6 +1,7 @@ #include "ContainerParser.h" #include +#include bool ContainerParser::setup() { parser = XML_ParserCreate(nullptr); @@ -14,14 +15,7 @@ bool ContainerParser::setup() { return true; } -ContainerParser::~ContainerParser() { - if (parser) { - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_ParserFree(parser); - parser = nullptr; - } -} +ContainerParser::~ContainerParser() { destroyXmlParser(parser); } size_t ContainerParser::write(const uint8_t data) { return write(&data, 1); } @@ -35,6 +29,7 @@ size_t ContainerParser::write(const uint8_t* buffer, const size_t size) { void* const buf = XML_GetBuffer(parser, 1024); if (!buf) { LOG_DBG("CTR", "Couldn't allocate buffer"); + destroyXmlParser(parser); return 0; } @@ -43,6 +38,7 @@ size_t ContainerParser::write(const uint8_t* buffer, const size_t size) { if (XML_ParseBuffer(parser, static_cast(toRead), remainingSize == toRead) == XML_STATUS_ERROR) { LOG_ERR("CTR", "Parse error: %s", XML_ErrorString(XML_GetErrorCode(parser))); + destroyXmlParser(parser); return 0; } diff --git a/lib/Epub/Epub/parsers/ContentOpfParser.cpp b/lib/Epub/Epub/parsers/ContentOpfParser.cpp index 33518bc65..f75fd2d0c 100644 --- a/lib/Epub/Epub/parsers/ContentOpfParser.cpp +++ b/lib/Epub/Epub/parsers/ContentOpfParser.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include "../BookMetadataCache.h" @@ -26,13 +27,7 @@ bool ContentOpfParser::setup() { } ContentOpfParser::~ContentOpfParser() { - if (parser) { - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; - } + destroyXmlParser(parser); if (tempItemStore) { tempItemStore.close(); } @@ -55,11 +50,7 @@ size_t ContentOpfParser::write(const uint8_t* buffer, const size_t size) { if (!buf) { LOG_ERR("COF", "Couldn't allocate memory for buffer"); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); return 0; } @@ -69,11 +60,7 @@ size_t ContentOpfParser::write(const uint8_t* buffer, const size_t size) { if (XML_ParseBuffer(parser, static_cast(toRead), remainingSize == toRead) == XML_STATUS_ERROR) { LOG_DBG("COF", "Parse error at line %lu: %s", XML_GetCurrentLineNumber(parser), XML_ErrorString(XML_GetErrorCode(parser))); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); return 0; } diff --git a/lib/Epub/Epub/parsers/TocNavParser.cpp b/lib/Epub/Epub/parsers/TocNavParser.cpp index 8c8afdd51..dc293c141 100644 --- a/lib/Epub/Epub/parsers/TocNavParser.cpp +++ b/lib/Epub/Epub/parsers/TocNavParser.cpp @@ -2,6 +2,7 @@ #include #include +#include #include "../BookMetadataCache.h" @@ -18,15 +19,7 @@ bool TocNavParser::setup() { return true; } -TocNavParser::~TocNavParser() { - if (parser) { - XML_StopParser(parser, XML_FALSE); - XML_SetElementHandler(parser, nullptr, nullptr); - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; - } -} +TocNavParser::~TocNavParser() { destroyXmlParser(parser); } size_t TocNavParser::write(const uint8_t data) { return write(&data, 1); } @@ -40,11 +33,7 @@ size_t TocNavParser::write(const uint8_t* buffer, const size_t size) { void* const buf = XML_GetBuffer(parser, 1024); if (!buf) { LOG_DBG("NAV", "Couldn't allocate memory for buffer"); - XML_StopParser(parser, XML_FALSE); - XML_SetElementHandler(parser, nullptr, nullptr); - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); return 0; } @@ -54,11 +43,7 @@ size_t TocNavParser::write(const uint8_t* buffer, const size_t size) { if (XML_ParseBuffer(parser, static_cast(toRead), remainingSize == toRead) == XML_STATUS_ERROR) { LOG_DBG("NAV", "Parse error at line %lu: %s", XML_GetCurrentLineNumber(parser), XML_ErrorString(XML_GetErrorCode(parser))); - XML_StopParser(parser, XML_FALSE); - XML_SetElementHandler(parser, nullptr, nullptr); - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); return 0; } diff --git a/lib/Epub/Epub/parsers/TocNcxParser.cpp b/lib/Epub/Epub/parsers/TocNcxParser.cpp index 843790251..ac968010c 100644 --- a/lib/Epub/Epub/parsers/TocNcxParser.cpp +++ b/lib/Epub/Epub/parsers/TocNcxParser.cpp @@ -2,6 +2,7 @@ #include #include +#include #include "../BookMetadataCache.h" @@ -18,15 +19,7 @@ bool TocNcxParser::setup() { return true; } -TocNcxParser::~TocNcxParser() { - if (parser) { - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; - } -} +TocNcxParser::~TocNcxParser() { destroyXmlParser(parser); } size_t TocNcxParser::write(const uint8_t data) { return write(&data, 1); } @@ -40,11 +33,7 @@ size_t TocNcxParser::write(const uint8_t* buffer, const size_t size) { void* const buf = XML_GetBuffer(parser, 1024); if (!buf) { LOG_DBG("TOC", "Couldn't allocate memory for buffer"); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); return 0; } @@ -54,11 +43,7 @@ size_t TocNcxParser::write(const uint8_t* buffer, const size_t size) { if (XML_ParseBuffer(parser, static_cast(toRead), remainingSize == toRead) == XML_STATUS_ERROR) { LOG_DBG("TOC", "Parse error at line %lu: %s", XML_GetCurrentLineNumber(parser), XML_ErrorString(XML_GetErrorCode(parser))); - XML_StopParser(parser, XML_FALSE); // Stop any pending processing - XML_SetElementHandler(parser, nullptr, nullptr); // Clear callbacks - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); return 0; } diff --git a/lib/OpdsParser/OpdsParser.cpp b/lib/OpdsParser/OpdsParser.cpp index 84feef74f..ef769f11f 100644 --- a/lib/OpdsParser/OpdsParser.cpp +++ b/lib/OpdsParser/OpdsParser.cpp @@ -1,6 +1,7 @@ #include "OpdsParser.h" #include +#include #include @@ -12,15 +13,7 @@ OpdsParser::OpdsParser() { } } -OpdsParser::~OpdsParser() { - if (parser) { - XML_StopParser(parser, XML_FALSE); - XML_SetElementHandler(parser, nullptr, nullptr); - XML_SetCharacterDataHandler(parser, nullptr); - XML_ParserFree(parser); - parser = nullptr; - } -} +OpdsParser::~OpdsParser() { destroyXmlParser(parser); } size_t OpdsParser::write(uint8_t c) { return write(&c, 1); } @@ -39,7 +32,8 @@ size_t OpdsParser::write(const uint8_t* xmlData, const size_t length) { void* const buf = XML_GetBuffer(parser, chunkSize); if (!buf) { errorOccured = true; - XML_ParserFree(parser); + LOG_DBG("OPDS", "Couldn't allocate memory for buffer"); + destroyXmlParser(parser); return length; } @@ -48,7 +42,9 @@ size_t OpdsParser::write(const uint8_t* xmlData, const size_t length) { if (XML_ParseBuffer(parser, static_cast(toRead), 0) == XML_STATUS_ERROR) { errorOccured = true; - XML_ParserFree(parser); + LOG_DBG("OPDS", "Parse error at line %lu: %s", XML_GetCurrentLineNumber(parser), + XML_ErrorString(XML_GetErrorCode(parser))); + destroyXmlParser(parser); return length; } currentPos += toRead; @@ -60,8 +56,7 @@ size_t OpdsParser::write(const uint8_t* xmlData, const size_t length) { void OpdsParser::flush() { if (XML_Parse(parser, nullptr, 0, XML_TRUE) != XML_STATUS_OK) { errorOccured = true; - XML_ParserFree(parser); - parser = nullptr; + destroyXmlParser(parser); } } diff --git a/lib/XmlParserUtils/XmlParserUtils.h b/lib/XmlParserUtils/XmlParserUtils.h new file mode 100644 index 000000000..bbab9c933 --- /dev/null +++ b/lib/XmlParserUtils/XmlParserUtils.h @@ -0,0 +1,13 @@ +#pragma once + +#include + +// Safely tear down an expat parser: stop processing, clear callbacks, free, and null the pointer. +inline void destroyXmlParser(XML_Parser& parser) { + if (!parser) return; + XML_StopParser(parser, XML_FALSE); + XML_SetElementHandler(parser, nullptr, nullptr); + XML_SetCharacterDataHandler(parser, nullptr); + XML_ParserFree(parser); + parser = nullptr; +}