Guard nil-group and out-of-bounds index in selectedToolbarItemGroupItem:#396
Merged
Conversation
…edToolbarItemGroupItem: selectedToolbarItemGroupItem: previously did `selectedGroup.subitems[selectedIndex]` with no protection against `selectedGroup` being nil (identifier missing from the registration dictionary) or `selectedIndex` being out of range for subitems. The bounds case is a latent NSRangeException crash; the nil case is benign today due to ObjC nil-messaging but is a correctness hazard if any caller stops short- circuiting on a nil item. Add NSAssert + early-return guards for both cases. In Debug the asserts fire loudly so misconfigurations are caught; in Release they compile away and the early return protects users from the crash. Also adds four XCTest cases: unknown identifier, out-of-bounds index, negative index, and a valid-call regression guard. Related to #394
- Declare selectedToolbarItemGroupItem: via a private category in the test file so the call compiles without -Wundeclared-selector - Replace 99/50 magic numbers in the out-of-bounds test with 3/2 (one past the group's 2 subitems) - Add a precondition check in the negative-index test to confirm AppKit doesn't clamp sender.selectedSegment = -1 Related to #394
Contributor
Code Coverage ReportCurrent Coverage: 61.12% Coverage Details (Summary) |
Owner
Author
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
2 participants
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
Adds nil-group and out-of-bounds guards to
selectedToolbarItemGroupItem:inMPToolbarController.m.The method previously did
selectedGroup.subitems[selectedIndex]with no protection against:sender.identifiernot being present intoolbarItemIdentifierObjectDictionary(selectedGroupbecomes nil)selectedIndexbeing negative or>= selectedGroup.subitems.count(NSRangeException fromobjectAtIndexedSubscript:)The bounds case is a latent
NSRangeExceptioncrash. The nil case is benign today thanks to ObjC nil-messaging semantics, but is a correctness hazard if the downstream guards ever change.Both paths now use
NSAssert+ an earlyreturn:ENABLE_NS_ASSERTIONS = YES) the asserts fire loudly with a message that names the offending identifier and the actual vs expected subitem count, so misconfigurations are diagnosed immediately.ENABLE_NS_ASSERTIONS = NO) the asserts compile away and theif (…) return;keeps users out of the crash path.Test coverage
Four new tests in
MPToolbarControllerTests.m:testSelectedToolbarItemGroupItemUnknownIdentifierAssertstestSelectedToolbarItemGroupItemOutOfBoundsIndexAssertstestSelectedToolbarItemGroupItemNegativeIndexAsserts(with a precondition check thatNSSegmentedControlactually stores-1)testSelectedToolbarItemGroupItemValidCallDoesNotCrash(regression guard for the happy path)The three guard tests use
XCTAssertThrowsSpecificNamed(…, NSInternalInconsistencyException)because the test target is built in Debug. A private category declaration exposesselectedToolbarItemGroupItem:to the test target so the call compiles without-Wundeclared-selector.The Release-mode
if … return;branches are defense-in-depth and not directly reachable under Debug; this is noted in a short comment above the new test section.Related Issue
Related to #394
Manual Testing Plan
N/A — the XCTest suite covers the testable surface, and the guards only fire on a misconfigured toolbar that can't be produced through the normal UI (segmented controls are constructed in
toolbarItemGroupWithIdentifier:with matching identifier andsegmentCount == items.count, so the fault conditions are structurally impossible from user interaction).