Skip to content

Fixstrinindexoutofbounds#2777

Closed
carstenartur wants to merge 42 commits into
eclipse-jdt:masterfrom
carstenartur:Fixstrinindexoutofbounds
Closed

Fixstrinindexoutofbounds#2777
carstenartur wants to merge 42 commits into
eclipse-jdt:masterfrom
carstenartur:Fixstrinindexoutofbounds

Conversation

@carstenartur
Copy link
Copy Markdown
Contributor

Fix race condition in "Format edited lines" causing StringIndexOutOfBoundsException

External region calculation becomes stale between computation and formatter invocation, causing crashes in TokenManager.countLineBreaksBetween() when offsets are invalid.

Solution

Introduces DocumentDirtyTracker that maintains dirty line state within the document lifecycle:

// Instead of using stale external regions
IRegion[] regionsToFormat = changedRegions;

// Now uses atomically-tracked regions
DocumentDirtyTracker tracker = DocumentDirtyTracker.get(document);
IRegion[] dirtyRegions = tracker.getDirtyRegions();
regionsToFormat = validateRegions(dirtyRegions, document);

DocumentDirtyTracker (new)

  • Tracks dirty lines via IDocumentListener, adjusts on insert/delete
  • Uses documentAboutToBeChanged() to capture pre-modification state
  • Thread-safe via Collections.synchronizedMap(WeakHashMap)
  • Marks only start/end lines, not intermediates

CleanUpPostSaveListener

  • Switches from external to tracker-based region calculation
  • Adds validateRegions() with defensive bounds checking
  • Clears tracker after successful format

CompilationUnitDocumentProvider

  • Initializes tracker on document connect

Edge Cases Handled

  • Empty regions at document end (cursor positioning)
  • Multi-byte UTF-8 characters and emojis
  • Rapid successive edits
  • Line insertions/deletions mid-edit

Tests

  • 14 comprehensive JUnit 5 test cases covering all edge cases
Original prompt

Problem

When using "Format edited lines" save action in Eclipse, users frequently encounter a StringIndexOutOfBoundsException: Index -1 out of bounds error. This is caused by a race condition where region offsets calculated externally can become stale/invalid between calculation and formatting.

See: #2499

Root Cause

The current implementation in CleanUpPostSaveListener calculates edited regions externally and passes them to the formatter. Between region calculation and formatting, the document can change, leading to invalid offsets that cause crashes in TokenManager.countLineBreaksBetween() and WrapPreparator.applyBreaksOutsideRegions().

Stack trace from the issue:

java.lang.StringIndexOutOfBoundsException: Index -1 out of bounds for length 147
    at org.eclipse.jdt.internal.formatter.TokenManager.countLineBreaksBetween(TokenManager.java:244)
    at org.eclipse.jdt.internal.formatter.linewrap.WrapPreparator.applyBreaksOutsideRegions(WrapPreparator.java:1459)

Solution

Implement a DocumentDirtyTracker that:

  1. Tracks dirty lines internally - The document itself tracks which lines have been modified since the last format/save, updated atomically with each edit operation.

  2. Provides always-valid regions - When the formatter requests regions to format, they are calculated from the current document state, not from stale external data.

  3. Adjusts line numbers on edits - When lines are inserted or deleted, the dirty line numbers are automatically adjusted so they remain valid.

  4. Add defensive bounds checking - As an additional safety measure, add bounds validation in TokenManager.countLineBreaksBetween() to prevent crashes even if invalid data somehow gets through.

Implementation Details

New class: DocumentDirtyTracker

  • Implements IDocumentListener to track document changes
  • Maintains a TreeSet<Integer> of dirty line numbers
  • Adjusts line numbers when lines are inserted/deleted
  • Provides getDirtyRegions() to get valid IRegion[] for formatting
  • Uses WeakHashMap to associate trackers with documents without API changes

Modify: CleanUpPostSaveListener

  • Use DocumentDirtyTracker.get(document).getDirtyRegions() instead of external region calculation
  • Clear dirty lines after successful formatting

Modify: TokenManager.countLineBreaksBetween()

  • Add defensive bounds checking:
public int countLineBreaksBetween(String text, int startPosition, int endPosition) {
    if (startPosition < 0 || endPosition > text.length() || startPosition >= endPosition) {
        return 0;
    }
    // ... existing code
}

Modify: CompilationUnitDocumentProvider

  • Initialize DocumentDirtyTracker when document is created/connected

Benefits

  • Eliminates race condition between region calculation and formatting
  • Dirty lines are always consistent with current document state
  • No public API changes required
  • Fixes not just this bug but an entire class of potential race condition bugs

Tests to Add

Add tests in the formatter test suite that verify:

  1. Formatting specific regions with non-ASCII characters (UTF-8, emojis)
  2. Formatting after rapid successive edits
  3. Region validity after line insertions/deletions
  4. Defensive bounds checking in countLineBreaksBetween

This pull request was created from Copilot chat.

What it does

How to test

Author checklist

carstenartur and others added 30 commits October 14, 2025 22:06
* Bump codacy/codacy-analysis-cli-action from 1.1.0 to 4.4.0 (#19)

* Formaster3 (#20)

* Update maven.yml (#9)

* Formaster4 (#8)

* Create dependabot.yml

* Update dependabot.yml

* Update dependabot.yml

* Update dependabot.yml

* Create codacy.yml

* Update pom.xml

* Bump tycho.version from 2.3.0 to 2.7.3

Bumps `tycho.version` from 2.3.0 to 2.7.3.

Updates `tycho-surefire-plugin` from 2.3.0 to 2.7.3

Updates `tycho-source-plugin` from 2.3.0 to 2.7.3
- [Release notes](https://github.com/eclipse/tycho/releases)
- [Changelog](https://github.com/eclipse/tycho/blob/master/RELEASE_NOTES.md)
- [Commits](eclipse-tycho/tycho@tycho-2.3.0...tycho-2.7.3)

Updates `tycho-p2-plugin` from 2.3.0 to 2.7.3

---
updated-dependencies:
- dependency-name: org.eclipse.tycho:tycho-surefire-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: org.eclipse.tycho:tycho-source-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
- dependency-name: org.eclipse.tycho:tycho-p2-plugin
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Create codeql.yml

* Create maven.yml

* Update README.md

* Update README.md

* Update README.md

* Bump tycho.version from 2.7.3 to 2.7.4

Bumps `tycho.version` from 2.7.3 to 2.7.4.

Updates `tycho-surefire-plugin` from 2.7.3 to 2.7.4

Updates `tycho-source-plugin` from 2.7.3 to 2.7.4
- [Release notes](https://github.com/eclipse/tycho/releases)
- [Changelog](https://github.com/eclipse/tycho/blob/tycho-2.7.4/RELEASE_NOTES.md)
- [Commits](eclipse-tycho/tycho@tycho-2.7.3...tycho-2.7.4)

Updates `tycho-p2-plugin` from 2.7.3 to 2.7.4

---
updated-dependencies:
- dependency-name: org.eclipse.tycho:tycho-surefire-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.eclipse.tycho:tycho-source-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.eclipse.tycho:tycho-p2-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump tycho.version from 2.7.4 to 2.7.5

Bumps `tycho.version` from 2.7.4 to 2.7.5.

Updates `tycho-surefire-plugin` from 2.7.4 to 2.7.5

Updates `tycho-source-plugin` from 2.7.4 to 2.7.5
- [Release notes](https://github.com/eclipse/tycho/releases)
- [Changelog](https://github.com/eclipse-tycho/tycho/blob/tycho-2.7.5/RELEASE_NOTES.md)
- [Commits](eclipse-tycho/tycho@tycho-2.7.4...tycho-2.7.5)

Updates `tycho-p2-plugin` from 2.7.4 to 2.7.5

---
updated-dependencies:
- dependency-name: org.eclipse.tycho:tycho-surefire-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.eclipse.tycho:tycho-source-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
- dependency-name: org.eclipse.tycho:tycho-p2-plugin
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* Update README.md

* Update maven.yml

* Update codeql.yml

* Update README.md

* Update README.md

* Update README.md

* Update README.md

* Update dependabot.yml

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update maven.yml

* Update maven.yml (#10)

* Update maven.yml for java 20 (#11)

* Update codeql.yml (#12)

* Update maven.yml

* Update codeql.yml

* Update codeql.yml

* Bump actions/setup-java from 2 to 4 (#17)

Bumps [actions/setup-java](https://github.com/actions/setup-java) from 2 to 4.
- [Release notes](https://github.com/actions/setup-java/releases)
- [Commits](actions/setup-java@v2...v4)

---
updated-dependencies:
- dependency-name: actions/setup-java
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump github/codeql-action from 2 to 3 (#16)

Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2 to 3.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@v2...v3)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump codacy/codacy-analysis-cli-action from 1.1.0 to 4.3.0 (#15)

Bumps [codacy/codacy-analysis-cli-action](https://github.com/codacy/codacy-analysis-cli-action) from 1.1.0 to 4.3.0.
- [Release notes](https://github.com/codacy/codacy-analysis-cli-action/releases)
- [Commits](codacy/codacy-analysis-cli-action@d840f88...5cc54a7)

---
updated-dependencies:
- dependency-name: codacy/codacy-analysis-cli-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump cirrus-actions/rebase from 1.7 to 1.8 (#14)

Bumps [cirrus-actions/rebase](https://github.com/cirrus-actions/rebase) from 1.7 to 1.8.
- [Release notes](https://github.com/cirrus-actions/rebase/releases)
- [Commits](cirrus-actions/rebase@1.7...1.8)

---
updated-dependencies:
- dependency-name: cirrus-actions/rebase
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update codeql.yml

* Update codacy.yml

* Bump actions/checkout from 3 to 4 (#13)

* Update codacy.yml

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump stCarolas/setup-maven from 4.5 to 5 (#22)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Copilot AI and others added 12 commits February 3, 2026 04:29
…ener

- Created DocumentDirtyTracker to track dirty lines internally and prevent race conditions
- Modified CleanUpPostSaveListener to use DocumentDirtyTracker for region calculation
- Added defensive bounds checking in validateRegions method
- Initialized tracker in CompilationUnitDocumentProvider.connect()

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
- Created DocumentDirtyTrackerTest with 14 test cases
- Tests cover single/multiple edits, line insertion/deletion, UTF-8 handling
- Tests verify region bounds validation and concurrent document tracking
- Added test to JdtTextTestSuite

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
- Fixed linesRemoved calculation by using documentAboutToBeChanged to capture state before modification
- Changed dirty line marking to only mark start and end lines, not all intermediate lines
- Improved bounds validation to properly handle empty regions at document end
- Changed test assertions from assertEquals to assertTrue for better readability

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
- Use Collections.synchronizedMap for thread-safe WeakHashMap
- Remove double-locking by simplifying get() and dispose() methods
- Use static NullProgressMonitor instance to reduce allocations
- Extract isValidRegion() helper method with clear documentation

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
- Document thread-safety assumption for lineCountBeforeChange (IDocument guarantees serialized changes)
- Fix logic to clear dirty lines after finally block, using saved needsChangedRegions flag
- Add clarifying comment about DocumentDirtyTracker.get() side effect in CompilationUnitDocumentProvider

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
- Changed imports from org.junit.* to org.junit.jupiter.api.*
- Removed unused assertArrayEquals import
- Changed @before to @beforeeach and @after to @AfterEach
- Fixed build compilation error

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
- Changed all assertions to use correct JUnit 5 parameter order (value first, message last)
- Fixed assertNull, assertNotNull, assertEquals, and assertTrue calls
- Resolved build compilation errors

Co-authored-by: carstenartur <3164220+carstenartur@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants