Skip to content

Commit

Permalink
DeskTop: Fix corruption when exiting Shortcuts dialogs
Browse files Browse the repository at this point in the history
Introduced in c2975b5 (back in March 2023); rearranging the shortcut
overlay from $9000-$A000 down to $5000-$6000 put at least one routine
into the "danger zone" so that when the shortcuts routine (1) closed
the file picker and (2) cleared updates but (3) the overlay was not
restored, the update clearing would call into bad code leading to
corruption or a crash.

Move the routine into a safe zone. Also, from an audit of the routines
invoked by ClearUpdates, drop in several more asserts. This is
definitely not a perfect solution.

Fixes #790.
  • Loading branch information
inexorabletash committed Mar 1, 2024
1 parent c37bdb0 commit 16df426
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 16 deletions.
2 changes: 2 additions & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ Project Page: https://github.com/a2stuff/a2d
* Show progress bar during copy and delete operations.
* Click on menu bar clock now shows Date & Time DA.
* Refresh correct window after renaming an icon, if view is by name.
* Fix corruption when exiting Shortcuts dialogs with a list view window. (([#790](https://github.com/a2stuff/a2d/issues/790))


### Selector

Expand Down
48 changes: 32 additions & 16 deletions desktop/main.s
Original file line number Diff line number Diff line change
Expand Up @@ -3002,21 +3002,6 @@ selection_preserved_count:
rts
.endproc ; FindIconForRecordNum

;;; ============================================================
;;; Retrieve the `IconEntry::record_num` for a given icon.
;;; Input: A = icon id
;;; Output: A = icon's record index in its window
;;; Trashes $06

.proc GetIconRecordNum
jsr GetIconEntry
ptr := $06
stax ptr
ldy #IconEntry::record_num
lda (ptr),y
rts
.endproc ; GetIconRecordNum

;;; ============================================================
;;; Retrieve the window id for a given icon.
;;; Input: A = icon id
Expand Down Expand Up @@ -6172,6 +6157,7 @@ UncheckViewMenuItem := CheckViewMenuItemImpl::uncheck
;;; ============================================================
;;; Draw all entries (icons or list items) in (cached) window

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc DrawWindowEntries
;; --------------------------------------------------
;; Icons
Expand Down Expand Up @@ -6227,6 +6213,22 @@ done:
rts
.endproc ; DrawWindowEntries

;;; ============================================================
;;; Retrieve the `IconEntry::record_num` for a given icon.
;;; Input: A = icon id
;;; Output: A = icon's record index in its window
;;; Trashes $06

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc GetIconRecordNum
jsr GetIconEntry
ptr := $06
stax ptr
ldy #IconEntry::record_num
lda (ptr),y
rts
.endproc ; GetIconRecordNum

;;; ============================================================

.proc ClearSelection
Expand Down Expand Up @@ -6327,19 +6329,22 @@ skip:

;;; ============================================================

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc CachedIconsScreenToWindow
param_jump CachedIconsXToY, IconPtrScreenToWindow
.endproc ; CachedIconsScreenToWindow

;;; ============================================================

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc CachedIconsWindowToScreen
param_jump CachedIconsXToY, IconPtrWindowToScreen
.endproc ; CachedIconsWindowToScreen

;;; ============================================================

;;; Inputs: A,X = proc to call for each icon
.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc CachedIconsXToY
stax proc

Expand Down Expand Up @@ -7766,6 +7771,7 @@ flags: .byte 0
;;; ============================================================
;;; Draw header (items/K in disk/K available/lines) for active window

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc DrawWindowHeader
;; --------------------------------------------------
;; Separator Lines
Expand Down Expand Up @@ -8340,6 +8346,7 @@ CompareFileRecords_sort_by := CompareFileRecords::sort_by
;;; ============================================================
;;; A = entry number

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc DrawListViewRow

ptr := $06
Expand Down Expand Up @@ -8396,7 +8403,6 @@ set_pos:
stax pos_col::xcoord
MGTK_CALL MGTK::MoveTo, pos_col
rts
.endproc ; DrawListViewRow

;;; ============================================================

Expand Down Expand Up @@ -8443,9 +8449,12 @@ set_pos:
FALL_THROUGH_TO ComposeSizeString
.endproc ; PrepareColSize

.endproc ; DrawListViewRow

;;; ============================================================
;;; Populate `text_buffer2` with "12,345K"

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc ComposeSizeString
stax value ; size in 512-byte blocks

Expand Down Expand Up @@ -8509,6 +8518,7 @@ value: .word 0

;;; ============================================================

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc ComposeDateString
copy #0, text_buffer2
copy16 #text_buffer2, $8
Expand Down Expand Up @@ -14721,6 +14731,7 @@ done: rts
;;; Inputs: A = window id
;;; Outputs: Z = 1 if found, and X = index in `window_id_to_filerecord_list_entries`

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc FindIndexInFilerecordListEntries
ldx window_id_to_filerecord_list_count
dex
Expand All @@ -14734,6 +14745,7 @@ done: rts
;;; Input: A = window_id
;;; Output: A,X = address of FileRecord list (first entry is length)
;;; Assert: Window is found in list.
.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc GetFileRecordListForWindow
jsr FindIndexInFilerecordListEntries
txa
Expand All @@ -14759,6 +14771,7 @@ done: rts
.endproc ; GetActiveWindowViewBy

;;; Assert: There is a cached window
.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc GetCachedWindowViewBy
ldx cached_window_id
lda win_view_by_table-1,x
Expand Down Expand Up @@ -14856,6 +14869,7 @@ ret: rts
;;; ============================================================


.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
;;; Input: A = window_id (0=desktop)
.proc LoadWindowEntryTable
sta cached_window_id
Expand Down Expand Up @@ -14978,6 +14992,7 @@ window_entry_table: .res ::kMaxIconCount+1, 0
;;; logic with 127 icons. A simpler fix may be possible, see commit
;;; 41ebde49 for another attempt, but that introduces other issues.

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
.proc LoadActiveWindowEntryTable
lda active_window_id
jmp LoadWindowEntryTable
Expand Down Expand Up @@ -15212,6 +15227,7 @@ desktop_icon_usage_table:

;;; ============================================================

.assert * < $5000 || * >= $6000, error, "Routine used when clearing updates in overlay zone"
;;; FileRecord for list view
list_view_filerecord:
.tag FileRecord
Expand Down
3 changes: 3 additions & 0 deletions res/notes/testplan.md
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,9 @@
* Launch DeskTop. Open a volume window. Open a folder window. View > by Name. Verify that the selection is still in the volume window, and that there is no selection in the folder window.
* Launch DeskTop. Open a volume window. Open a folder window. Select a file in the folder window. View > by Name. Verify that the selection is still in the folder window.

* Repeat for the Shortcuts > Add, Edit, Delete, and Run a Shortcut commands
* Launch DeskTop. Open a volume window. View > by Name. Run the command from the Shortcuts menu. Cancel. Verify that the window entries repaint correctly (correct types, sizes, dates) and DeskTop doesn't crash.

* Repeat the following cases with these modifiers: Open-Apple, Solid-Apple:
* Launch DeskTop. Open 3 windows (A, B, C). Hold modifier and press Tab repeatedly. Verify that windows are activated and cycle in forward order (A, B, C, A, B, C, ...).
* Launch DeskTop. Open 3 windows (A, B, C). Hold modifier and press \` repeatedly. Verify that windows are activated cycle in forward order (A, B, C, A, B, C, ...).
Expand Down

0 comments on commit 16df426

Please sign in to comment.