Continuous Integration: Stabilize Address Sanitizer (ASan) CI jobs

RELEASE_NOTES.md

@@ -10,6 +10,17 @@ Release Notes ⋮
 [high-priority issues]: https://github.com/davidfstr/Crystal-Web-Archiver/issues?q=is%3Aopen+is%3Aissue+label%3Apriority-high
 [medium-priority issues]: https://github.com/davidfstr/Crystal-Web-Archiver/issues?q=is%3Aopen+is%3Aissue+label%3Apriority-medium
 
+### main
+
+* Minor fixes
+    * Fix closing a project to no longer have a couple of heap-use-after-free
+      issues which could corrupt memory, potentially crashing Crystal later.
+
+* Testing improvements
+    * Automated tests are now regularly run with Address Sanitizer,
+      which helps to identify heap-use-after-free bugs related to
+      interacting with wxPython that corrupt memory.
+
 ### v1.9.0b (June 22, 2024)
 
 This release contains error-handling improvements and bug fixes

src/crystal/progress.py

@@ -181,6 +181,7 @@ class OpenProjectProgressDialog(_AbstractProgressDialog, OpenProjectProgressList
 
     # NOTE: Only changed when tests are running
     _always_show_upgrade_required_modal = False
+    _upgrading_revision_progress = 0  # type: Optional[int]
 
     def __init__(self) -> None:
         super().__init__()
@@ -300,6 +301,8 @@ def will_upgrade_revisions(self, approx_revision_count: int, can_veto: bool) ->
                 raise CancelOpenProject()
             else:
                 raise AssertionError()
+
+        OpenProjectProgressDialog._upgrading_revision_progress = 0
 
     @override
     def upgrading_revision(self, index: int, revisions_per_second: float) -> None:
@@ -312,6 +315,7 @@ def upgrading_revision(self, index: int, revisions_per_second: float) -> None:
         """
         print(f'Upgrading revisions: {index:n} / {self._approx_revision_count:n} ({int(revisions_per_second):n} rev/sec)')
         self._update(index)
+        OpenProjectProgressDialog._upgrading_revision_progress = index
 
     @override
     def did_upgrade_revisions(self, revision_count: int) -> None:
@@ -320,6 +324,7 @@ def did_upgrade_revisions(self, revision_count: int) -> None:
         """
         assert self._dialog is not None
         self._dialog.SetRange(max(revision_count, 1))
+        OpenProjectProgressDialog._upgrading_revision_progress = None  # done
 
     # === Phase 2: Load ===
 

src/crystal/tests/test_menus.py

@@ -6,7 +6,6 @@
     crystal_shell,
 )
 from crystal.tests.util.windows import OpenOrCreateDialog
-from crystal.tests.util.xos import skip_on_windows
 import tempfile
 import textwrap
 
@@ -17,8 +16,6 @@ async def test_can_close_project_with_menuitem() -> None:
 
 
 async def test_can_quit_with_menuitem() -> None:
-    skip_on_windows()()  # crystal_shell doesn't support Windows
-
     with crystal_shell() as (crystal, _):
         _create_new_empty_project(crystal)
 

src/crystal/tests/test_project_migrate.py

@@ -18,7 +18,7 @@
 from crystal.tests.util.controls import click_button
 from crystal.tests.util.runner import bg_sleep
 from crystal.tests.util.server import extracted_project
-from crystal.tests.util.wait import wait_for, window_condition
+from crystal.tests.util.wait import wait_for, wait_while, window_condition
 from crystal.tests.util.windows import MainWindow, OpenOrCreateDialog
 from crystal.util.db import DatabaseCursor
 import os.path
@@ -168,7 +168,8 @@ def click_continue_in_upgrade_required_modal(dialog: wx.Dialog) -> int:
 
         with _upgrade_required_modal_always_shown(), \
                 patch('crystal.progress.ShowModal', click_continue_in_upgrade_required_modal):
-            async with (await OpenOrCreateDialog.wait_for()).open(project_dirpath) as mw:
+            async with (await OpenOrCreateDialog.wait_for()).open(
+                    project_dirpath, wait_func=_wait_for_project_to_upgrade) as mw:
                 assert did_respond_to_upgrade_required_modal
 
                 maybe_project = Project._last_opened_project
@@ -328,7 +329,8 @@ async def test_can_upgrade_project_from_major_version_1_to_2() -> None:
 
         # Upgrade the project to major version >= 2.
         # Ensure revisions appear to be migrated correctly.
-        async with (await OpenOrCreateDialog.wait_for()).open(project_dirpath) as mw:
+        async with (await OpenOrCreateDialog.wait_for()).open(
+                project_dirpath, wait_func=_wait_for_project_to_upgrade) as mw:
             maybe_project = Project._last_opened_project
             assert maybe_project is not None
             project = maybe_project
@@ -402,7 +404,8 @@ def upgrading_revision(index: int, revisions_per_second: float) -> None:
 
         # Resume upgrading the project to major version >= 2. Allow to finish.
         if True:
-            async with (await OpenOrCreateDialog.wait_for()).open(project_dirpath) as mw:
+            async with (await OpenOrCreateDialog.wait_for()).open(
+                    project_dirpath, wait_func=_wait_for_project_to_upgrade) as mw:
                 maybe_project = Project._last_opened_project
                 assert maybe_project is not None
                 project = maybe_project
@@ -491,3 +494,12 @@ def _count_files_in(dirpath: str) -> int:
     for (_, _, filenames) in os.walk(dirpath):
         file_count += len(filenames)
     return file_count
+
+
+async def _wait_for_project_to_upgrade() -> None:
+    if OpenProjectProgressDialog._upgrading_revision_progress is None:
+        OpenProjectProgressDialog._upgrading_revision_progress = 0
+
+    def progression_func() -> Optional[int]:
+        return OpenProjectProgressDialog._upgrading_revision_progress
+    await wait_while(progression_func)

src/crystal/tests/test_shell.py

@@ -4,14 +4,14 @@
 from crystal.tests.util.asserts import assertEqual, assertIn, assertNotIn
 from crystal.tests.util.wait import (
     DEFAULT_WAIT_PERIOD, DEFAULT_WAIT_TIMEOUT, HARD_TIMEOUT_MULTIPLIER,
-    WaitTimedOut,
+    wait_for_sync, WaitTimedOut,
 )
 from crystal.tests.util.windows import MainWindow
 from crystal.tests.util.screenshots import take_error_screenshot
 from crystal.tests.util.server import served_project
+from crystal.tests.util.skip import skipTest
 from crystal.tests.util.subtests import SubtestsContext, with_subtests
-from crystal.tests.util.xos import skip_on_windows
-from crystal.util.xos import is_windows
+from crystal.util.xos import is_asan, is_windows
 from crystal.util.xthreading import fg_call_and_wait
 from functools import wraps
 from io import StringIO, TextIOBase
@@ -74,7 +74,6 @@
 # ------------------------------------------------------------------------------
 # Tests
 
-@skip_on_windows
 @with_subtests
 def test_can_launch_with_shell(subtests: SubtestsContext) -> None:
     with crystal_shell() as (crystal, banner):
@@ -124,7 +123,6 @@ def test_can_launch_with_shell(subtests: SubtestsContext) -> None:
             assertIn('Help on MainWindow in module crystal.browser object:', _py_eval(crystal, 'help(window)'))
 
 
-@skip_on_windows
 def test_can_use_pythonstartup_file() -> None:
     with tempfile.NamedTemporaryFile(suffix='.py') as startup_file:
         startup_file.write(textwrap.dedent('''\
@@ -145,7 +143,6 @@ def test_can_use_pythonstartup_file() -> None:
 # NOTE: This test code was split out of the test_can_launch_with_shell() test above
 #       because it is particularly easy to break and having a separate test function
 #       makes the break type quicker to identify.
-@skip_on_windows
 @with_subtests
 def test_builtin_globals_have_stable_public_api(subtests: SubtestsContext) -> None:
     with crystal_shell() as (crystal, _):
@@ -163,9 +160,10 @@ def test_builtin_globals_have_stable_public_api(subtests: SubtestsContext) -> No
                 'Public API of MainWindow class has changed')
 
 
-@skip_on_windows
 @with_subtests
 def test_shell_exits_with_expected_message(subtests: SubtestsContext) -> None:
+    _ensure_can_use_crystal_shell()
+
     with subtests.test(case='test when first open/create dialog is closed given shell is running then shell remains running'):
         with crystal_shell() as (crystal, _):
             _close_open_or_create_dialog(crystal)
@@ -291,7 +289,6 @@ def test_shell_exits_with_expected_message(subtests: SubtestsContext) -> None:
                     timeout=.5 + DEFAULT_WAIT_TIMEOUT)
 
 
-@skip_on_windows
 def test_when_typed_code_raises_exception_then_print_traceback() -> None:
     with crystal_shell() as (crystal, _):
         expected_traceback = (
@@ -302,7 +299,6 @@ def test_when_typed_code_raises_exception_then_print_traceback() -> None:
         assertEqual(expected_traceback, _py_eval(crystal, 'Resource'))
 
 
-@skip_on_windows
 @with_subtests
 def test_can_read_project_with_shell(subtests: SubtestsContext) -> None:
     with served_project('testdata_xkcd.crystalproj.zip') as sp:
@@ -394,7 +390,6 @@ def test_can_read_project_with_shell(subtests: SubtestsContext) -> None:
                 assertIn(b'<title>xkcd: Air Gap</title>', response_bytes)
 
 
-@skip_on_windows
 @with_subtests
 def test_can_write_project_with_shell(subtests: SubtestsContext) -> None:
     with served_project('testdata_xkcd.crystalproj.zip') as sp:
@@ -443,6 +438,7 @@ def test_can_write_project_with_shell(subtests: SubtestsContext) -> None:
                 # Test can download ResourceRevision
                 with _delay_between_downloads_minimized(crystal):
                     assertEqual('', _py_eval(crystal, 'rr_future = r.download()'))
+                    # TODO: Use wait_for_sync() rather than a manual loop
                     while True:
                         is_done = (literal_eval(_py_eval(crystal, 'rr_future.done()')) == True)
                         if is_done:
@@ -457,9 +453,11 @@ def test_can_write_project_with_shell(subtests: SubtestsContext) -> None:
                     "ResourceGroup('Comic','http://localhost:2798/_/https/xkcd.com/#/')\n",
                     _py_eval(crystal, f'rg = ResourceGroup(p, "Comic", {comic_pattern!r}); rg'))
                 # Ensure ResourceGroup includes some members discovered by downloading resource Home
-                assertEqual(
-                    '9\n',
-                    _py_eval(crystal, f'len(rg.members)'))
+                def rg_member_count() -> int:
+                    count = literal_eval(_py_eval(crystal, f'len(rg.members)'))
+                    assert isinstance(count, int)
+                    return count
+                wait_for_sync(lambda: 9 == rg_member_count())
 
             with subtests.test(case='test can delete project entities', return_if_failure=True):
                 # Test can delete ResourceGroup
@@ -503,6 +501,7 @@ def test_can_write_project_with_shell(subtests: SubtestsContext) -> None:
                 # Test can download RootResource
                 with _delay_between_downloads_minimized(crystal):
                     assertEqual('', _py_eval(crystal, 'rr_future = root_r.download()'))
+                    # TODO: Use wait_for_sync() rather than a manual loop
                     while True:
                         is_done = (literal_eval(_py_eval(crystal, 'rr_future.done()')) == True)
                         if is_done:
@@ -525,6 +524,7 @@ def test_can_write_project_with_shell(subtests: SubtestsContext) -> None:
                 # Test can download ResourceGroup
                 with _delay_between_downloads_minimized(crystal):
                     assertEqual('', _py_eval(crystal, 'drgt = rg.download()'))
+                    # TODO: Use wait_for_sync() rather than a manual loop
                     while True:
                         is_done = (literal_eval(_py_eval(crystal, 'drgt.complete')) == True)
                         if is_done:
@@ -560,7 +560,6 @@ def test_can_delete_project_entities() -> None:
     pass
 
 
-@skip_on_windows
 def test_can_import_guppy_in_shell() -> None:
     with crystal_shell() as (crystal, _):
         # Ensure can import guppy
@@ -702,10 +701,7 @@ def crystal_shell(*, env_extra={}) -> Iterator[Tuple[subprocess.Popen, str]]:
     Context which starts "crystal --shell" upon enter
     and cleans up the associated process upon exit.
     """
-    if platform.system() == 'Windows':
-        # Windows doesn't provide stdout for graphical processes,
-        # which is needed by the current implementation
-        raise AssertionError('not supported on Windows')
+    _ensure_can_use_crystal_shell()
 
     # Determine how to run Crystal on command line
     crystal_command: List[str]
@@ -757,6 +753,19 @@ def crystal_shell(*, env_extra={}) -> Iterator[Tuple[subprocess.Popen, str]]:
         crystal.wait()
 
 
+def _ensure_can_use_crystal_shell() -> None:
+    if is_windows():
+        # NOTE: Windows doesn't provide stdout for graphical processes,
+        #       which is needed by the current implementation.
+        #       Workaround is possible with run_exe.py but time-consuming to implement.
+        skipTest('not supported on Windows; graphical subprocesses are mute')
+    if is_asan():
+        # NOTE: ASan slows down many operations, causing shell operations to
+        #       spuriously fail timeout checks, even when
+        #       CRYSTAL_GLOBAL_TIMEOUT_MULTIPLIER is used
+        skipTest('too slow when run with Address Sanitizer')
+
+
 def _py_eval(
         python: subprocess.Popen,
         py_code: str,

src/crystal/tests/util/runner.py

@@ -129,11 +129,11 @@ def run(self) -> _T:
 
 class SleepCommand(Command[None]):
     def __init__(self, delay: float) -> None:
-        self._delay = delay  # in seconds
+        self.delay = delay  # in seconds
 
     @bg_affinity
     def run(self) -> None:
-        time.sleep(self._delay)
+        time.sleep(self.delay)
 
 
 class FetchUrlCommand(Command['WebPage']):

src/crystal/tests/util/wait.py

@@ -20,7 +20,7 @@
 _T3 = TypeVar('_T3')
 
 
-GLOBAL_TIMEOUT_MULTIPLIER = IGNORE_USE_AFTER_FREE = (
+GLOBAL_TIMEOUT_MULTIPLIER = (
     float(os.environ.get('CRYSTAL_GLOBAL_TIMEOUT_MULTIPLIER', '1.0'))
 )
 
@@ -103,6 +103,9 @@ async def wait_for(
     Waits up to `timeout` seconds for the specified condition to become non-None,
     returning the result of the condition, checking every `period` seconds.
     
+    The condition is always checked on the foreground thread.
+    The foreground thread is released while waiting between checks.
+    
     Raises:
     * WaitTimedOut -- if the timeout expires before the condition becomes non-None
     """
@@ -168,6 +171,21 @@ async def wait_for(
                     stacklevel=(2 + stacklevel_extra))
 
 
+def wait_for_sync(condition: Callable[[], Optional[_T]], *args, **kwargs) -> _T:
+    """
+    Similar to wait_for() but does not release the current thread between waits.
+    """
+    from crystal.tests.util.runner import SleepCommand
+    coro = wait_for(condition, *args, **kwargs)
+    while True:
+        try:
+            command = coro.send(None)
+        except StopIteration as e:
+            return e.value
+        assert isinstance(command, SleepCommand)
+        time.sleep(command.delay)
+
+
 class WaitTimedOut(Exception):
     pass
 

src/crystal/tests/util/windows.py

@@ -19,7 +19,7 @@
 import sys
 import tempfile
 import traceback
-from typing import AsyncIterator, Callable, Optional, Tuple, TYPE_CHECKING
+from typing import AsyncIterator, Awaitable, Callable, Optional, Tuple, TYPE_CHECKING
 import wx
 
 if TYPE_CHECKING:
@@ -107,6 +107,7 @@ async def open(self,
             *, readonly: Optional[bool]=None,
             autoclose: bool=True,
             using_crystalopen: bool=False,
+            wait_func: Optional[Callable[[], Awaitable[None]]]=None,
             ) -> AsyncIterator[MainWindow]:
         if readonly is not None:
             self.open_as_readonly.Value = readonly
@@ -119,6 +120,8 @@ async def open(self,
         with file_dialog_returning(itempath_to_open):
             click_button(self.open_button)
 
+            if wait_func is not None:
+                await wait_func()
             mw = await MainWindow.wait_for(timeout=self._TIMEOUT_FOR_OPEN_MAIN_WINDOW)
 
         exc_info_while_close = None