TBrowse:refreshAll() discards the skip block's return value and can leave the cursor on a different record

[kamilprzyb2]

TBrowse:refreshAll() discards the skip block's return value and can leave the cursor on a different record

Background

I was investigating a problem report against an application of ours: a DBF LOCATE
run from inside a TBrowse would, under certain circumstances, leave the browse sitting
on the row it started from, so the search looked as though it had done nothing. Tracing
that led here.

A disclosure, because it should affect how you weigh what follows: this investigation
was carried out by Claude Opus 5 (1M context) running in Claude Code, and it wrote this
report. I have verified and confirmed all of it myself — I built both trees, ran the
tests, and checked the source analysis against the code. None of it is passed on
unchecked, but the wording is not mine.

I am not proposing a concrete fix at this stage. The part I cannot settle is the
CA-Clipper compatibility question: whether refreshAll() moving the record pointer at
all is deliberate replication of Clipper behaviour, or an implementation detail that
later grew dependencies. That needs someone with more history in this code. The options
under Notes toward a fix are sketches, not a recommendation.

Summary

TBrowse:refreshAll() re-anchors the browse by skipping the data source backward
nBufferPos - 1 records and then asserting ::nBufferPos := 1. It does not look at
how far the skip block actually moved. When the skip block cannot travel the full
distance — at BOF, or with a skip block that limits movement to a scope — the browse's
row accounting no longer matches the data source, and after the next full stabilization
the cursor sits on a record other than the one the caller left the record pointer on.

refreshAll() is the only place in src/rtl/tbrowse.prg that asks the skip block to
move and then ignores the count it returns.

Affected versions

Reproduced on both maintained lineages, from a clean checkout of each:

tree	version	commit	result
harbour/core	3.2.1dev	c1941b5994 (2026-09-01)	2 of 4 probes wrong
vszakats/hb	3.4.0dev	8bfdf3087f (2025-10-01)	2 of 4 probes wrong

src/rtl/tbrowse.prg has diverged between the two (Unicode-aware string functions,
hb_keyStd() in applyKey()), but every method involved here is identical in both:
refreshAll(), readRecord()'s skip arithmetic, setPosition(), stabilize(),
goTop() and goBottom(). The tbrowse.prg line numbers cited below therefore hold
for both trees. dbEdit()'s compensation is also identical, at dbedit.prg:330-347
in harbour/core and 334-351 in vszakats/hb.

The code is long-standing; the 2013 tree-flattening commit (a4a357a18b) is as far
back as git log -S reaches, so there is no recorded rationale for the skip line.

The defect

src/rtl/tbrowse.prg:1059:

METHOD refreshAll() CLASS TBrowse

   ::setUnstable()

   Eval( ::bSkipBlock, 1 - ::nBufferPos )
   ::nBufferPos := 1
   ::lFrames := .T.
   /* In CA-Cl*pper refreshAll() method does not discards
    * record buffer here but only set's flag that the record
    * buffer should be reloaded in stabilize method. [druzus]
    */
   ::lRefresh := .T.

   RETURN Self

How the other call sites use the return value

Every site that asks for movement reads nMoved back and adjusts ::nBufferPos by
that value. refreshAll() is the exception: it assigns 1 without looking.

line	method	requests	reads nMoved?	resulting ::nBufferPos
614	readRecord()	nRow - ::nBufferPos	yes	::nBufferPos += nMoved
686	setPosition()	::nMoveOffset	yes	::nBufferPos += nMoved
813	stabilize() final alignment	::nRowPos - ::nBufferPos	yes	::nBufferPos += nMoved
1235	goBottom()	-( ::rowCount - 1 )	yes	:= 1, shortfall carried into ::nMoveOffset := -nMoved
1223, 1244	goTop(), goBottom()	0	n/a, nothing moves	:= 1, already true
1063	refreshAll()	1 - ::nBufferPos	no	:= 1, asserted

The += nMoved form is self-correcting: whatever the skip block managed, the browse's
belief follows it. Only two places assign ::nBufferPos := 1 outright. The zero-skips
in goTop()/goBottom() are safe because nothing moved and the assignment was already
made true by an absolute reposition. goBottom()'s real skip pays for its assignment by
carrying the shortfall into ::nMoveOffset. refreshAll() assigns after a skip that can
fall short, and carries nothing.

Because readRecord() and setPosition() do follow the count, a short forward skip at
EOF is already handled — readRecord() sets ::nLastRow, and stabilize() clamps
::nRowPos to it at line 807. A short backward skip in refreshAll() has no
equivalent.

__dbSkipper() reports short counts honestly — src/rdd/dbcmd.c:2374-2385 breaks out of
the backward loop on BOF without counting the failed step — so the information
refreshAll() needs is available; it is simply thrown away.

goBottom() is the same shape, compensated

   Eval( ::bGoBottomBlock )
   nMoved := _SKIP_RESULT( Eval( ::bSkipBlock, -( ::rowCount - 1 ) ) )
   ::lRefresh := .T.
   ::nRowPos := 1
   ::nBufferPos := 1
   ::nMoveOffset := -nMoved
   Eval( ::bSkipBlock, 0 )

goBottom() issues a backward skip that clamps at BOF whenever the table holds fewer
records than the screen — exactly refreshAll()'s failure mode — and absorbs the short
count into ::nMoveOffset so the cursor still lands on the right row. Same author,
adjacent method, handled there and not here.

The [druzus] comment annotates a different statement

The comment inside refreshAll() reads as though it might sanction the current
behaviour. It does not: it is a template appearing three times in the file, always
directly above ::lRefresh := .T. — refreshAll() (1066), goTop() (1215),
goBottom() (1236) — and it describes deferring the cell-buffer invalidation
(AFill( ::aCellStatus, .F. ), which stabilize() performs under IF ::lRefresh at
line 757), not record pointer movement. goTop() carries it while doing no relative
skipping at all, and in goBottom() it sits below the skip line.

Consequence

::nRowPos is left untouched by refreshAll(). After the buffer refills and
stabilize() performs its final alignment to ::nRowPos, the record under the cursor is:

the record the backward skip actually landed on, plus nRowPos - 1 records forward

which equals the caller's record only if the backward skip moved the full
nRowPos - 1 records.

Why the drift is always forward

The backward skip can come up short but never long, so the cursor always ends up
after the intended record, never before. This is consistent with the workaround in
dbEdit() (below) correcting only with Up().

Reproduction

refreshAll() is documented as the way to tell a browse that the data changed, and it
is what callers reach for after repositioning the record pointer themselves. The failure
needs only that the target record be closer to BOF than rowPos - 1 records.

/* tbrfa.prg -- TBrowse:refreshAll() loses the record position near BOF
 *
 * Build and run (from hb/tests, with a freshly built hbmk2 on PATH):
 *
 *    hbmk2 tbrfa.prg && tbrfa
 *
 * Every probe should report OK. Those whose target record is closer to BOF
 * than rowPos-1 records fail: refreshAll() skips the data source backward
 * nBufferPos-1 records to re-anchor, discards how far the skip block actually
 * moved, and asserts ::nBufferPos := 1 anyway. The backward skip is short near
 * BOF, so stabilize()'s final alignment to ::nRowPos lands the cursor on
 * record #rowPos of the file instead of on the requested record.
 */

#define _TARGET   1
#define _GOT      2

PROCEDURE Main()

   LOCAL oBrw
   LOCAL aResult := {}
   LOCAL aRow
   LOCAL nRowCount, nRowPos
   LOCAL nFail := 0
   LOCAL aOut := {}
   LOCAL cLine
   LOCAL i

   dbCreate( "tbrfa", { { "NUM", "N", 6, 0 } }, , .T., "tbrfa" )
   FOR i := 1 TO 200
      dbAppend()
      FIELD->NUM := i
   NEXT

   dbGoTop()

   oBrw := TBrowseDB( 1, 1, 12, 20 )
   oBrw:addColumn( TBColumnNew( "NUM", {|| FIELD->NUM } ) )
   oBrw:forceStable()

   /* move the cursor down to row 6 */
   FOR i := 1 TO 5
      oBrw:down()
   NEXT
   oBrw:forceStable()

   nRowCount := oBrw:rowCount
   nRowPos   := oBrw:rowPos

   AAdd( aResult, Probe( oBrw, 50 ) )   /* far from BOF, backward skip has room */
   AAdd( aResult, Probe( oBrw,  7 ) )   /* exactly enough room                  */
   AAdd( aResult, Probe( oBrw,  3 ) )   /* backward skip clamps at BOF          */
   AAdd( aResult, Probe( oBrw,  1 ) )   /* backward skip cannot move at all     */

   /* stabilize() parks the hardware cursor on the browse's current cell, so
      report only once every repaint is done -- otherwise the browse overwrites
      its own test output. The same lines go to stderr, so that
      "tbrfa 2>report.txt" yields a clean copy free of terminal control codes. */
   AAdd( aOut, "TBrowse:refreshAll() -- record position after an external dbGoto()" )
   AAdd( aOut, "" )
   AAdd( aOut, "rowCount: " + hb_ntos( nRowCount ) + "   rowPos: " + hb_ntos( nRowPos ) )
   AAdd( aOut, "" )
   AAdd( aOut, "  dbGoto()   RecNo() after refreshAll():forceStable()" )
   FOR EACH aRow IN aResult
      AAdd( aOut, "  " + Str( aRow[ _TARGET ], 8 ) + "   " + Str( aRow[ _GOT ], 8 ) + "   " + ;
                  iif( aRow[ _TARGET ] == aRow[ _GOT ], "OK", "*** WRONG ***" ) )
      IF aRow[ _TARGET ] != aRow[ _GOT ]
         nFail++
      ENDIF
   NEXT
   AAdd( aOut, "" )
   AAdd( aOut, "RESULT: " + iif( nFail == 0, "all probes OK", ;
                                 hb_ntos( nFail ) + " of " + hb_ntos( Len( aResult ) ) + " probes wrong" ) )
   AAdd( aOut, "Harbour: " + Version() )

   CLS
   FOR EACH cLine IN aOut
      ? cLine
   NEXT
   ?
   FOR EACH cLine IN aOut
      OutErr( cLine + hb_eol() )
   NEXT

   dbCloseArea()

   ErrorLevel( iif( nFail == 0, 0, 1 ) )

   RETURN

STATIC FUNCTION Probe( oBrw, nRec )

   dbGoto( nRec )
   oBrw:refreshAll()
   oBrw:forceStable()

   RETURN { nRec, RecNo() }

Expected on every probe: RecNo() equals the record passed to dbGoto().

Core-only build, no contribs, so none of the optional dependencies are needed:

git clone https://github.com/harbour/core.git && cd core
HB_BUILD_CONTRIBS=no HB_INSTALL_PREFIX=$PWD/../hb-install make -j4 install
cd .. && ./hb-install/bin/hbmk2 tbrfa.prg && ./tbrfa 2>report.txt

The report is written to stderr as well as the screen, because the browse's own cursor
positioning otherwise overwrites it; 2>report.txt yields a copy free of terminal
control codes.

Observed on harbour/core (Linux x86_64, gcc 12.2, default DBF/NTX):

TBrowse:refreshAll() -- record position after an external dbGoto()

rowCount: 11   rowPos: 6

  dbGoto()   RecNo() after refreshAll():forceStable()
        50         50   OK
         7          7   OK
         3          6   *** WRONG ***
         1          6   *** WRONG ***

RESULT: 2 of 4 probes wrong
Harbour: Harbour 3.2.1dev (r2609011352)

and on vszakats/hb, identically:

TBrowse:refreshAll() -- record position after an external dbGoto()

rowCount: 11   rowPos: 6

  dbGoto()   RecNo() after refreshAll():forceStable()
        50         50   OK
         7          7   OK
         3          6   *** WRONG ***
         1          6   *** WRONG ***

RESULT: 2 of 4 probes wrong
Harbour: Harbour 3.4.0dev (8bfdf3087f) (2025-10-01 16:37)

With nRowPos == 6 the re-anchor requests 5 records backward. Targets 7 and 50 have
that headroom and land correctly. Targets 3 and 1 do not: the backward skip clamps at
BOF, ::nBufferPos is forced to 1 regardless of how far the skip block actually got,
and stabilize()'s final alignment then counts nRowPos - 1 records forward from the
top of the file — landing on record 6 in both cases, which is also the record that was
under the cursor before the dbGoto().

The boundary is ::nRowPos: every target below it collapses onto record nRowPos.
Since nRowPos varies with where the user happens to have scrolled, the failure looks
data-dependent from an application's point of view, which is how it tends to get
reported.

dbEdit() compensates, and only dbEdit()

src/rtl/dbedit.prg, in CallUser():

   IF nAction == DE_REFRESH .OR. nPrevRecNo != RecNo()

      IF nAction != DE_ABORT

         lAppend := .F.

         IF ( Set( _SET_DELETED ) .AND. Deleted() ) .OR. ;
            ( ! Empty( dbFilter() ) .AND. ! Eval( hb_macroBlock( dbFilter() ) ) )
            dbSkip()
         ENDIF
         IF Eof()
            dbGoBottom()
         ENDIF

         nPrevRecNo := RecNo()
         oBrowse:refreshAll():forceStable()
         DO WHILE nPrevRecNo != RecNo()
            oBrowse:Up():forceStable()
         ENDDO

dbEdit() records RecNo(), calls refreshAll():forceStable(), then walks Up() until
the pointer is back where the user function left it. That loop has no other purpose.

It works completely. Driving the same four targets through dbEdit() with a user
function that does the dbGoto() and returns DE_REFRESH:

dbEdit() + user function returning DE_REFRESH after dbGoto()

  dbGoto()   RecNo() seen by the next user-function call
        50         50   OK   (row 6 held record 6 before the jump)
         7          7   OK   (row 6 held record 6 before the jump)
         3          3   OK   (row 6 held record 6 before the jump)
         1          1   OK   (row 6 held record 6 before the jump)

Harbour: Harbour 3.2.1dev (r2609011352)

All four correct, including the two that come back as record 6 through TBrowse
directly. dbEdit() also cannot hit the termination hazard described under Notes toward
a fix, because it installs its own conforming skip block (Skipped()) that callers
cannot replace.

So this is not a bug report about dbEdit(). It is a report about refreshAll(),
which is public, documented API on a public class, and about the fact that the only
correct usage pattern exists as an unexplained workaround inside one caller in core.
Applications that drive TBrowse themselves get no such protection, and nothing
indicates that they need it.

That is how we ran into this. Our application uses an in-house replacement for
dbEdit() that drives TBrowse directly. It calls refreshAll() after repositioning
the record pointer, which is what the method's documentation suggests, and it has none
of CallUser()'s compensation — there was no reason to suspect it needed any.

Impact

Anything that drives TBrowse directly and repositions the record pointer itself:
dbSeek(), dbGoto(), LOCATE, dbGoTop()/dbGoBottom(), SET ORDER/SET INDEX,
dbSetFilter(), dbSetScope().

dbSeek() is a particularly good trigger because it lands on the first record of a
key group in index order, so seeking a key that sorts early puts the target within a
few records of BOF — exactly the clamp window.

Applications that install a skip block limited to a key scope (a common idiom for
scoped browses) are affected far more often, because the clamp boundary is the top of
the scope rather than the top of the file, and can be only a handful of records away
at any time.

Symptomatically this reads as "the browse ignores my seek and jumps back", and it is
data-dependent, so it tends to be reported as intermittent.

Notes toward a fix

Sketches, not a recommendation — see Background.

1. Capture the count and correct ::nRowPos in refreshAll(). Requesting
1 - nBufferPos and getting back nMoved means the caller's record now sits at row
1 - nMoved, so ::nRowPos := 1 - nMoved keeps the cursor on it. nMoved ranges from
1 - nBufferPos to 0, so the result always falls within 1 .. nBufferPos and needs no
clamping. When nothing clamps it comes out unchanged, so every currently-working case is
byte-identical and the Up() loop in dbEdit() degrades to a no-op rather than breaking.

2. Standardise dbEdit()'s correction as the documented caller-side remedy. The
nPrevRecNo != RecNo() test plus the Up() loop already works, and promoting it from an
unexplained local workaround to a documented pattern costs nothing in core. Two limits
worth stating if this is the route:

• It cannot be hoisted into TBrowse itself. The correction is built on RecNo(), and
  TBrowse is deliberately data-source agnostic — its only channel to the data is
  ::bSkipBlock, which is purely relative. dbEdit() can compensate precisely because it
  knows it is driving a DBF; refreshAll() cannot.
• The loop is unbounded and corrects in one direction only. With a conforming skip block
  that is sufficient: the drift is ( nRowPos - 1 ) - m with m <= nRowPos - 1, so the
  cursor always lands at or after the target and Up() terminates. But a skip block that
  limits movement to a scope, or that misreports its count, leaves Up() unable to reach
  the target while RecNo() never matches — an infinite loop. Any documented version
  should carry a bound.

3. Leave refreshAll() as it is and document the precondition — "the data changed,
the position did not". Cheapest, and the honest objection to it is that the precondition
is not one a caller can always honour: another user deleting records above the cursor on
a shared table, or SET DELETED ON with a deletion above the cursor, breaks it with the
caller having done nothing wrong — and refreshAll() is precisely the method one is
supposed to call when the data changed underneath.

What I have not verified

• Behaviour under RDDs other than the default DBF/NTX.
• Whether CA-Clipper's refreshAll() moves the record pointer at all, which is the
  compatibility question behind the first fix option above.
• The code path is platform-independent, but both runs above are Linux/gcc only.

[alcz]

I suppose you need to be in TBrowse god-mode to move around such issue. Here are some Clipper results:

TBrowse:refreshAll() -- record position after an external dbGoto()
                                                                  
rowCount:         11   rowPos:          6                         
                                                                  
  dbGoto()   RecNo() after refreshAll():forceStable()             
        50         50   OK                                        
         7          7   OK                                        
         3          6   *** WRONG ***                             
         1          6   *** WRONG ***                             
                                                                  
RESULT:          2 of          4 probes wrong                     
Harbour: Clipper (R) 5.3b Intl. (Rev. 338) (1997.04.25)           

Quick port of the code to CLP...

/* tbrfa.prg -- TBrowse:refreshAll() loses the record position near BOF
 *
 * Build and run (from hb/tests, with a freshly built hbmk2 on PATH):
 *
 *    hbmk2 tbrfa.prg && tbrfa
 *
 * Every probe should report OK. Those whose target record is closer to BOF
 * than rowPos-1 records fail: refreshAll() skips the data source backward
 * nBufferPos-1 records to re-anchor, discards how far the skip block actually
 * moved, and asserts ::nBufferPos := 1 anyway. The backward skip is short near
 * BOF, so stabilize()'s final alignment to ::nRowPos lands the cursor on
 * record #rowPos of the file instead of on the requested record.
 */

#define _TARGET   1
#define _GOT      2

PROCEDURE Main()

   LOCAL oBrw
   LOCAL aResult := {}
   LOCAL aRow
   LOCAL nRowCount, nRowPos
   LOCAL nFail := 0
   LOCAL aOut := {}
   LOCAL cLine
   LOCAL i

   dbCreate( "tbrfa", { { "NUM", "N", 6, 0 } }, , .T., "tbrfa" )
   FOR i := 1 TO 200
      dbAppend()
      FIELD->NUM := i
   NEXT

   dbGoTop()

   oBrw := TBrowseDB( 1, 1, 12, 20 )
   oBrw:addColumn( TBColumnNew( "NUM", {|| FIELD->NUM } ) )
   oBrw:forceStable()

   /* move the cursor down to row 6 */
   FOR i := 1 TO 5
      oBrw:down()
   NEXT
   oBrw:forceStable()

   nRowCount := oBrw:rowCount
   nRowPos   := oBrw:rowPos

   AAdd( aResult, Probe( oBrw, 50 ) )   /* far from BOF, backward skip has room */
   AAdd( aResult, Probe( oBrw,  7 ) )   /* exactly enough room                  */
   AAdd( aResult, Probe( oBrw,  3 ) )   /* backward skip clamps at BOF          */
   AAdd( aResult, Probe( oBrw,  1 ) )   /* backward skip cannot move at all     */

   /* stabilize() parks the hardware cursor on the browse's current cell, so
      report only once every repaint is done -- otherwise the browse overwrites
      its own test output. The same lines go to stderr, so that
      "tbrfa 2>report.txt" yields a clean copy free of terminal control codes. */
   AAdd( aOut, "TBrowse:refreshAll() -- record position after an external dbGoto()" )
   AAdd( aOut, "" )
   AAdd( aOut, "rowCount: " + Str( nRowCount ) + "   rowPos: " + Str( nRowPos ) )
   AAdd( aOut, "" )
   AAdd( aOut, "  dbGoto()   RecNo() after refreshAll():forceStable()" )
   FOR i := 1 TO Len( aResult )
      aRow := aResult [ i ]
      AAdd( aOut, "  " + Str( aRow[ _TARGET ], 8 ) + "   " + Str( aRow[ _GOT ], 8 ) + "   " + ;
                  iif( aRow[ _TARGET ] == aRow[ _GOT ], "OK", "*** WRONG ***" ) )
      IF aRow[ _TARGET ] != aRow[ _GOT ]
         nFail++
      ENDIF
   NEXT
   AAdd( aOut, "" )
   AAdd( aOut, "RESULT: " + iif( nFail == 0, "all probes OK", ;
                                 Str( nFail ) + " of " + Str( Len( aResult ) ) + " probes wrong" ) )
   AAdd( aOut, "Harbour: " + Version() )

   CLS
   FOR i := 1 TO Len( aOut )
      ? aOut[ i ]
   NEXT
   ?
/*
   FOR i := 1 TO Len( aOut )
      OutErr( aOut[ i ] + chr(13) + chr(10) )
   NEXT
*/
   dbCloseArea()

   ErrorLevel( iif( nFail == 0, 0, 1 ) )

   RETURN

STATIC FUNCTION Probe( oBrw, nRec )

   dbGoto( nRec )
   oBrw:refreshAll()
   oBrw:forceStable()

   RETURN { nRec, RecNo() }

For now, I suggest subclassing TBrowse in your own code.

[kamilprzyb2]

Thank you for confirming this error is in fact, carried over from clipper.

For now, I suggest subclassing TBrowse in your own code.

Yes I already have a working workaround in place.

[alcz]

If your solution can be generalized I'd suggest creating a forked harbour/core repo on GitHub with something like "hbtb" contrib added - I'd say that a subclassed TBrowse would be good.