From a973d02c9fecebc0d2f7f823d9cdbd9c428ac100 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 1 May 2020 15:06:05 +0200 Subject: [PATCH 1/3] Get rid of CppUnit cruft in wxFileSystem test case Use CATCH macros directly and define separate test cases instead of using a useless FileSystemTestCase class. Also add INFO() macros to provide information about the context in case of test failure. No real changes. --- tests/filesys/filesystest.cpp | 49 ++++++++++------------------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/tests/filesys/filesystest.cpp b/tests/filesys/filesystest.cpp index 4f58313de4..06d07a0952 100644 --- a/tests/filesys/filesystest.cpp +++ b/tests/filesys/filesystest.cpp @@ -48,35 +48,10 @@ public: // ---------------------------------------------------------------------------- -// test class +// tests themselves // ---------------------------------------------------------------------------- -class FileSystemTestCase : public CppUnit::TestCase -{ -public: - FileSystemTestCase() { } - -private: - CPPUNIT_TEST_SUITE( FileSystemTestCase ); - CPPUNIT_TEST( UrlParsing ); - CPPUNIT_TEST( FileNameToUrlConversion ); - CPPUNIT_TEST( UnicodeFileNameToUrlConversion ); - CPPUNIT_TEST_SUITE_END(); - - void UrlParsing(); - void FileNameToUrlConversion(); - void UnicodeFileNameToUrlConversion(); - - wxDECLARE_NO_COPY_CLASS(FileSystemTestCase); -}; - -// register in the unnamed registry so that these tests are run by default -CPPUNIT_TEST_SUITE_REGISTRATION( FileSystemTestCase ); - -// also include in its own registry so that these tests can be run alone -CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( FileSystemTestCase, "FileSystemTestCase" ); - -void FileSystemTestCase::UrlParsing() +TEST_CASE("wxFileSystem::URLParsing", "[filesys][url][parse]") { static const struct Data { @@ -151,14 +126,15 @@ void FileSystemTestCase::UrlParsing() for ( size_t n = 0; n < WXSIZEOF(data); n++ ) { const Data& d = data[n]; - CPPUNIT_ASSERT_EQUAL( d.protocol, tst.Protocol(d.url) ); - CPPUNIT_ASSERT_EQUAL( d.left, tst.LeftLocation(d.url) ); - CPPUNIT_ASSERT_EQUAL( d.right, tst.RightLocation(d.url) ); - CPPUNIT_ASSERT_EQUAL( d.anchor, tst.Anchor(d.url) ); + INFO( "Testing URL " << wxString(d.url) ); + CHECK( tst.Protocol(d.url) == d.protocol ); + CHECK( tst.LeftLocation(d.url) == d.left ); + CHECK( tst.RightLocation(d.url) == d.right ); + CHECK( tst.Anchor(d.url) == d.anchor ); } } -void FileSystemTestCase::FileNameToUrlConversion() +TEST_CASE("wxFileSystem::FileNameToUrlConversion", "[filesys][url][filename]") { const static struct Data { const wxChar *input, *expected; @@ -179,12 +155,13 @@ void FileSystemTestCase::FileNameToUrlConversion() wxFileName fn1(d.input); wxString url1 = wxFileSystem::FileNameToURL(fn1); - CPPUNIT_ASSERT_EQUAL( d.expected, url1 ); - CPPUNIT_ASSERT( fn1.SameAs(wxFileName::URLToFileName(url1)) ); + INFO( "Testing URL " << wxString(d.input) ); + CHECK( url1 == d.expected ); + CHECK( fn1.SameAs(wxFileName::URLToFileName(url1)) ); } } -void FileSystemTestCase::UnicodeFileNameToUrlConversion() +TEST_CASE("wxFileSystem::UnicodeFileNameToUrlConversion", "[filesys][url][filename][unicode]") { const unsigned char filename_utf8[] = { 0x4b, 0x72, 0xc3, 0xa1, 0x73, 0x79, 0x50, 0xc5, @@ -196,7 +173,7 @@ void FileSystemTestCase::UnicodeFileNameToUrlConversion() wxString url = wxFileSystem::FileNameToURL(filename); - CPPUNIT_ASSERT( filename.SameAs(wxFileName::URLToFileName(url)) ); + CHECK( filename.SameAs(wxFileName::URLToFileName(url)) ); } #endif // wxUSE_FILESYSTEM From 10ee1b8d100e14f3478c1ce1c34431440bac4d18 Mon Sep 17 00:00:00 2001 From: Ben Bronk Date: Fri, 1 May 2020 15:40:43 +0200 Subject: [PATCH 2/3] Fix use of invalid find iterator in wxMemoryFSHandler Calling FindFirst() with an URL without wildcards returned the correct result, but didn't reset m_findIter, which kept its value from the previous search that could have been invalidated since then, e.g. if RemoveFile() has been called. Using this value could result in a crash during the next call to FindNext(). Fix this by ensuring that we always reset m_findIter to the valid (even if pointing to the end) value as the first thing we do in FindFirst(). Closes #18744. --- src/common/fs_mem.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/common/fs_mem.cpp b/src/common/fs_mem.cpp index dd74323c36..93faa21487 100644 --- a/src/common/fs_mem.cpp +++ b/src/common/fs_mem.cpp @@ -123,6 +123,10 @@ wxFSFile * wxMemoryFSHandlerBase::OpenFile(wxFileSystem& WXUNUSED(fs), wxString wxMemoryFSHandlerBase::FindFirst(const wxString& url, int flags) { + // Make sure to reset the find iterator, so that calling FindNext() doesn't + // reuse its value from the last search that could well be invalid. + m_findIter = m_Hash.end(); + if ( (flags & wxDIR) && !(flags & wxFILE) ) { // we only store files, not directories, so we don't risk finding From c098363e0d23bab1dc810b1a2c0f9d9a7eceaad5 Mon Sep 17 00:00:00 2001 From: Vadim Zeitlin Date: Fri, 1 May 2020 15:43:20 +0200 Subject: [PATCH 3/3] Add test for wxMemoryFSHandler bug fixed in the parent commit Executing this test before the changes of the previous commit would result in test failure/crash/Valgrind errors. Now it passes. See #18744. --- tests/filesys/filesystest.cpp | 50 +++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/tests/filesys/filesystest.cpp b/tests/filesys/filesystest.cpp index 06d07a0952..a46430ae11 100644 --- a/tests/filesys/filesystest.cpp +++ b/tests/filesys/filesystest.cpp @@ -24,6 +24,8 @@ #if wxUSE_FILESYSTEM +#include "wx/fs_mem.h" + // ---------------------------------------------------------------------------- // helpers // ---------------------------------------------------------------------------- @@ -176,4 +178,52 @@ TEST_CASE("wxFileSystem::UnicodeFileNameToUrlConversion", "[filesys][url][filena CHECK( filename.SameAs(wxFileName::URLToFileName(url)) ); } +// Test that using FindFirst() after removing a previously found URL works: +// this used to be broken, see https://trac.wxwidgets.org/ticket/18744 +TEST_CASE("wxFileSystem::MemoryFSHandler", "[filesys][memoryfshandler][find]") +{ + // Install wxMemoryFSHandler just for the duration of this test. + class AutoMemoryFSHandler + { + public: + AutoMemoryFSHandler() + : m_handler(new wxMemoryFSHandler()) + { + wxFileSystem::AddHandler(m_handler); + } + + ~AutoMemoryFSHandler() + { + wxFileSystem::RemoveHandler(m_handler); + } + + private: + wxMemoryFSHandler* const m_handler; + } autoMemoryFSHandler; + + wxMemoryFSHandler::AddFile("foo.txt", "foo contents"); + wxMemoryFSHandler::AddFile("bar.txt", "bar contents"); + wxFileSystem fs; + + wxString const url = fs.FindFirst("memory:*.txt"); + INFO("Found URL was: " << url); + + wxString filename; + REQUIRE( url.StartsWith("memory:", &filename) ); + + // We don't know which of the two files will be found, this depends on the + // details of the hash table implementation and varies across builds, so + // handle both cases and remove the other file, which should certainly + // invalidate the iterator pointing to it. + if ( filename == "foo.txt" ) + wxMemoryFSHandler::RemoveFile("bar.txt"); + else if ( filename == "bar.txt" ) + wxMemoryFSHandler::RemoveFile("foo.txt"); + else + FAIL("Unexpected filename: " << filename); + + CHECK( fs.FindFirst(url) == url ); + CHECK( fs.FindNext() == "" ); +} + #endif // wxUSE_FILESYSTEM