Repository navigation
Guard file browser back and forward against leaving the history - #166
Merged
Merged
Conversation
⌘← and ⌘→ in the file browser send goBack:/goForward: straight from FileBrowserOutlineView’s key equivalent handler, with no menu validation in between. The actions stepped the history index unconditionally, so pressing either key with nowhere to go indexed the history array at -1 or at its count, threw NSRangeException, and the exception handler aborted the app. Every other route (menu, header buttons, swipe) was already guarded. The two session-restore sites also clamped the saved index to count rather than count - 1, which lands one past the last entry. goBack:/goForward: now check canGoBack/canGoForward, the clamps use count - 1, and t_history_navigation.mm covers all three cases. The tests failed with the crash’s exception before the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
⌘← and ⌘→ in the file browser crash the app when there is nowhere to go.
FileBrowserOutlineViewsendsgoBack:/goForward:straight to the controller from its key equivalent handler (FileBrowserOutlineView.mm:70-71), and the two actions steppedhistoryIndexunconditionally (FileBrowserViewController.mm:298-299).setHistoryIndex:then indexed the history array at -1 or at its count, threwNSRangeException, and the exception handler aborted the process. The menu items, header buttons and swipe gesture were already guarded throughvalidateMenuItem:, thecanGoBack/canGoForwardbindings and the gesture handler; only the keyboard path was not. Inherited from upstream (a304ebe).Reproduced on 2.6.1-undead: open a project, click in the file browser, press ⌘←.
The two session-restore sites clamped the saved index to
countrather thancount - 1, which lands one past the last entry with the same exception.Change
goBack:andgoForward:do nothing unlesscanGoBack/canGoForward.count - 1.Frameworks/FileBrowser/tests/t_history_navigation.mm: back with no history, forward with no history, restored index past the end.Tests
The three new tests failed before the change with the crash report's exception (
index 18446744073709551615 beyond bounds for empty array,index 1 beyond bounds for empty array,index 1 beyond bounds [0 .. 0]). After it,FileBrowser_tests --no-parallelpasses 8 of 8. The fix was also confirmed in a Debug build of this branch: ⌘← and ⌘→ in a fresh window do nothing.Changelog
File Browser: ⌘← and ⌘→ with nowhere to go no longer crash the app.