Bug #11757
Audit browse row numbering for indexed-reposition (random access) negative row indexes
0%
History
#1 Updated by Alexandru Lungu 15 days ago
- Priority changed from Normal to High
- Assignee set to Stanislav Lomany
Follow-up on #11596. That ticket fixed FETCH-SELECTED-ROW itself (11596a/11596b); this one collects the surrounding code that was never audited when indexed reposition introduced a second row numbering space. Line numbers are against trunk rev. 16707.
The invariant¶
| mode | query row (currentRowImpl()) | browse/UI row |
|---|---|---|
| scrolling | 1-based absolute | queryRow - 1 |
| random access (after INDEXED-REPOSITION) | signed relative, 0 = the reposition pivot | queryRow, identical |
New rows are numbered from Browse.NEW_ROWS_NUMERATION_START (-2000000000) downwards in both modes. Hence: -1 is a valid row index and cannot be a "none" sentinel, x >= 0 is not a validity test, and the +/- 1 conversion is never unconditional. Correct implementations to copy: BrowseWidget.getCurrentRow() :6121 and setCurrentRow() :6028-6100.
To investigate¶
- G1 -
targetRepositionRowis off by one.AbstractQuery.java:1792computesrow.isUnknown() ? -1 : row.intValue() - 1unconditionally, and that value reachesBrowse.queryRepositionedas a UI row. Afterreposition-to-rowidthe pivot (query row 0) is reported as-1, whichBrowse.refreshScrollRow:7455then treats as its "no target" sentinel. The predicate must be the cursor state (CursorWrapper.isRandomAccess():887), notisIndexedReposition(). Most likely cause of the residual #11596-2 failure. - G2 - the renumbering offset is dropped.
getRows()nullsindexedRepositionOffsetat entry (BrowseWidget:5302) and returns it at:5735, so an offset produced by a reposition elsewhere -fetchSelectedRow:2998,deleteRow:9474,isNewRow:6005- is discarded and the client never renumbers. The server then scrolls while the client still believes it is in random access mode: permanent desync. Reachable fromFETCH-SELECTED-ROW, so #11596 is not fully closed without this. - G3 - the selection is not renumbered on revert.
Browse.revertedFromIndexedReposition:6431-6464shiftsdcData,currentRow,editedRowand friends, but notselectedRowIndicesorlastSelectedRow, so MULTIPLE browses keep stale relative indexes after the cursor reverts to scrolling. Note the asymmetry: entering random access mode clears the selection (:7881), leaving it does nothing. - G4 - sibling sites with the same unaudited conversion. Wrong offset and/or
>= 0gate inBrowseWidget.deleteRow:9471-9474,deleteResultListEntry:4966,isNewRow(int):6005,getRows:5434and:5697.-1sentinel collisions inBrowse.refreshAfterDeletion:7506,getMaximalSelectedRowIndex:3089,refreshScrollRow:7441,scrollRows:9042-9047andqueryRepositioned:7915. - G5 - root cause.
BrowseWidget.reposition(int64, boolean):9494is documented as "1-based row to set" but is called with three different conventions: UI index plus the mode aware offset (:2998,:5916,:6075), a raw query row (:5539,:6625), and a UI index treated as a query row (:6005,:9474, i.e. the G4 bugs). Adding explicittoQueryRow()/toUiRow()helpers is what stops this class of bug recurring. - G6 - FETCH-SELECTED-ROW error handling deviates from its siblings. It uses
displayErrorinstead ofrecordOrShowError, lacks thecheckNoRecordsandcheckInRowDisplayTriggerguards, andFETCH-SELECTED-ROW(0)returns FALSE silently instead of raising 385. Left unchanged in 11596b because each item needs an OE check first. - G7 - test coverage.
TestFetchSelectedRow.clsasserts only the logical return, never which record landed in the buffer, which is precisely why the off-by-one survived. There is no coverage at all for INDEXED-REPOSITION, negative indexes, new rows, MULTIPLE browses, or the revert-to-scrolling path.
Stanislav, please check what side-issues from #11596 were discovered. Let me know which ones are fit for client-side browse fixing and do the required changes in this task. I will do the same for any server-side issues and I will generate some tests before-hand.
#4 Updated by Stanislav Lomany 12 days ago
- Status changed from New to WIP
- reviewer Alexandru Lungu added
#5 Updated by Stanislav Lomany 9 days ago
FYI currently I'm working on DELETE-CURRENT-ROW() issues for INDEXED-REPOSITION case. I met them right away while working on on the first (G1) task.
#6 Updated by Stanislav Lomany 8 days ago
Created task branch 11757a from FWD trunk revision 16721.
#7 Updated by Stanislav Lomany about 18 hours ago
- File targetRepositionRow-single.p
added
Alexandru, I fixed several client-issues related to DELETE-* browse functions, but there are two server-side issues. I investigated them, but I didn't come to a good solution.
1. Abend happens when the number of positive items in ArrayWithNegativeIndexes become zero. ArrayWithNegativeIndexes:getLastIndex() by declaration cannot be below 0 and RandomAccessCursor:getResult fails to properly check the bounds:
this.results.getFirstIndex() <= index && index <= this.results.getLastIndex()Please use branch 11757a and the attached testcase. Reproduction:
- press R;
- hold D until abend.
Stacktrace:
Caused by: java.lang.IndexOutOfBoundsException: Index 0 out of bounds for length 0
at java.base/jdk.internal.util.Preconditions.outOfBounds(Preconditions.java:64)
at java.base/jdk.internal.util.Preconditions.outOfBoundsCheckIndex(Preconditions.java:70)
at java.base/jdk.internal.util.Preconditions.checkIndex(Preconditions.java:266)
at java.base/java.util.Objects.checkIndex(Objects.java:361)
at java.base/java.util.ArrayList.get(ArrayList.java:427)
at com.goldencode.p2j.util.ArrayWithNegativeIndexes.get(ArrayWithNegativeIndexes.java:111)
at com.goldencode.p2j.persist.RandomAccessCursor.getResult(RandomAccessCursor.java:1217)
at com.goldencode.p2j.persist.RandomAccessCursor.getNext(RandomAccessCursor.java:942)
at com.goldencode.p2j.persist.RandomAccessCursor.getNext(RandomAccessCursor.java:585)
at com.goldencode.p2j.persist.CursorWrapper.getNext(CursorWrapper.java:659)
at com.goldencode.p2j.persist.AdaptiveQuery.next(AdaptiveQuery.java:2277)
at com.goldencode.p2j.persist.AbstractQuery.afterReposition(AbstractQuery.java:1787)
at com.goldencode.p2j.persist.AdaptiveQuery.afterReposition(AdaptiveQuery.java:2190)
at com.goldencode.p2j.persist.AdaptiveQuery.reposition(AdaptiveQuery.java:1676)
at com.goldencode.p2j.persist.QueryWrapper.reposition(QueryWrapper.java:4081)
at com.goldencode.p2j.ui.BrowseWidget.reposition(BrowseWidget.java:9636)
at com.goldencode.p2j.ui.BrowseWidget.getRows(BrowseWidget.java:5581)
at com.goldencode.p2j.ui.BrowseWidget.getRows(BrowseWidget.java:5179)
at com.goldencode.p2j.ui.LogicalTerminal.getRows(LogicalTerminal.java:15220)
2. After all elements have been deleted from the scrolling query, the query may be refilled again from the underlying results.
- Press DOWN two times to select the second row.
- Hold D - you'll see that the first row will loop.