Closed Bug 17146 Opened 25 years ago Closed 25 years ago

[Tree] Assertion in nsTableRowFrame::SetRowIndex(-1)

Categories

(Core :: XUL, defect, P1)

Product:
Core
Component:
XUL
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: bratell, Assigned: selmer)

Details

Attachments

(1 file)

Context diff for this fix
3.88 KB, patch
Details | Diff | Splinter Review 
Overview Description:
I scrolled the bookmark tree in the sidebar when I got an assertion caused by
nsTableFrame::AdjustRowIndices being called with rowindex 0 and adjustment -1.

I traced it to nsTableFrame::RemoveRowFromMap being called with rowindex of 0.
The comment in that function says that it should increment the row indices but
as far as I can see it _decreases_ the indices. Anyway, I don't know if the
comment is wrong, the code is wrong, or if I just don't understand how it should
work. Anyway, the result is an assertion.

Stack Trace:
NTDLL! 77f7629c()
nsDebug::PreCondition(const char * 0x01945370
??_C@_0BF@JGIA@unexpected?5row?5index?$AA@, const char * 0x01945388
??_C@_0DJ@ICKM@?$CIaRowIndex?5?$DO?$DN?50?$CJ?5?$CG?$CG?5?$CIaRowIndex?5?$DM@,
const char * 0x019453c4 ??_C@_0DL@GFFC@g?3?2mozilla?2mozilla?2layout?2html?2t@,
int 321) line 262 + 13 bytes
nsTableRowFrame::SetRowIndex(int -1) line 321 + 41 bytes
nsTableFrame::AdjustRowIndices(nsTableFrame * const 0x02953840, nsIFrame *
0x02955850, int 0, int -1) line 718
nsTableFrame::AdjustRowIndices(nsTableFrame * const 0x02953840, nsIFrame *
0x02954710, int 0, int -1) line 722
nsTableFrame::RemoveRowFromMap(nsTableFrame * const 0x02953840, nsTableRowFrame
* 0x02955040, int 0) line 744
nsTreeRowGroupFrame::DestroyRows(nsTableFrame * 0x02953840, nsIPresContext &
{...}, int & 1) line 144
nsTreeRowGroupFrame::DestroyRows(nsTableFrame * 0x02953840, nsIPresContext &
{...}, int & 1) line 133
nsTreeRowGroupFrame::PositionChanged(nsTreeRowGroupFrame * const 0x02954754,
nsIPresContext & {...}, int 0, int 2) line 594
nsSliderFrame::CurrentPositionChanged(nsIPresContext * 0x0284ba20) line 654
nsSliderFrame::AttributeChanged(nsSliderFrame * const 0x02973950, nsIPresContext
* 0x0284ba20, nsIContent * 0x029714d0, int 0, nsIAtom * 0x010e3d50, int 2) line
195 + 18 bytes
nsScrollbarFrame::AttributeChanged(nsScrollbarFrame * const 0x020c15b8,
nsIPresContext * 0x0284ba20, nsIContent * 0x029714d0, int 0, nsIAtom *
0x010e3d50, int 2) line 332
nsCSSFrameConstructor::AttributeChanged(nsCSSFrameConstructor * const
0x028734c0, nsIPresContext * 0x0284ba20, nsIContent * 0x029714d0, int 0, nsIAtom
* 0x010e3d50, int 2) line 7643 + 35 bytes
StyleSetImpl::AttributeChanged(StyleSetImpl * const 0x02873560, nsIPresContext *
0x0284ba20, nsIContent * 0x029714d0, int 0, nsIAtom * 0x010e3d50, int -1) line
992
PresShell::AttributeChanged(PresShell * const 0x028733b8, nsIDocument *
0x0284aa50, nsIContent * 0x029714d0, int 0, nsIAtom * 0x010e3d50, int -1) line
1834 + 57 bytes
XULDocumentImpl::AttributeChanged(XULDocumentImpl * const 0x0284aa50, nsIContent
* 0x029714d0, int 0, nsIAtom * 0x010e3d50, int -1) line 1729
nsXULElement::SetAttribute(nsXULElement * const 0x029714d0, int 0, nsIAtom *
0x010e3d50, const nsString & {...}, int 1) line 2292
nsSliderFrame::SetCurrentPosition(nsIContent * 0x029714d0, nsIFrame *
0x02974cb0, int 2) line 676 + 36 bytes
nsSliderFrame::HandleEvent(nsSliderFrame * const 0x02973950, nsIPresContext &
{...}, nsGUIEvent * 0x0012fbe8, nsEventStatus & nsEventStatus_eIgnore) line 531
PresShell::HandleEvent(PresShell * const 0x028733b4, nsIView * 0x029738c0,
nsGUIEvent * 0x0012fbe8, nsEventStatus & nsEventStatus_eIgnore) line 2209 + 38
bytes
nsView::HandleEvent(nsView * const 0x029738c0, nsGUIEvent * 0x0012fbe8, unsigned
int 28, nsEventStatus & nsEventStatus_eIgnore, int & 0) line 834
nsViewManager::DispatchEvent(nsViewManager * const 0x02873c90, nsGUIEvent *
0x0012fbe8, nsEventStatus & nsEventStatus_eIgnore) line 1739
HandleEvent(nsGUIEvent * 0x0012fbe8) line 63
nsWindow::DispatchEvent(nsWindow * const 0x02873654, nsGUIEvent * 0x0012fbe8,
nsEventStatus & nsEventStatus_eIgnore) line 401 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012fbe8) line 422
nsWindow::DispatchMouseEvent(unsigned int 300, nsPoint * 0x00000000) line 3394 +
21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 300, nsPoint * 0x00000000) line
3612
nsWindow::ProcessMessage(unsigned int 512, unsigned int 1, long 3014831, long *
0x0012fdf0) line 2617 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x00290c9e, unsigned int 512, unsigned int 1, long
3014831) line 579 + 27 bytes
USER32! 77e71820()
JS3250! 002e00af()

Actual Results:
An assertion (really a "precondition" but the difference is to subtle for me)
preventing the program to go on.

Expected Results:
No assertion, a scroll of the tree.

Build Date & Platform Bug Found:
CVS build 1999-10-24
Windows NT SP5, Visual C++ 6 SP3
Assignee: trudelle → alecf
I can't reproduce any problem associated with this in the opt builds,
reassigning to alecf for triage.
Status: NEW → ASSIGNED
I've seen this happen before, I think it's when you scroll down in the folder
pane
there seems to be some kind of off-by-one bug in the selection code, might be
related.
Yes, I think when we move down one, the top element in the table is being set to
index -1 because the current position is 1 and the adjustment is -1 (since we're
moving down).  Therefore this assertion is hit.  I'm not sure where the best
place is to protect again this.  Probably in AdjustRowIndices.
Target Milestone: M11
Target Milestone: M11 → M12
M12. No behavior is described which is bad enough to hold M11 for this.
Summary: Assertion in nsTableRowFrame::SetRowIndex(-1) → [Tree] Assertion in nsTableRowFrame::SetRowIndex(-1)
Priority: P3 → P1
To reproduce this problem (in 19991109 build) open your bookmarks panel in
sidebar, resize the window until a scrollbar appears in the sidebar, scroll
bookmarks panel into the middle of the bookmarks.  When the top item gets
scrolled off the screen, AdjustRowIndices gets called with the -1 increment.

The call to AdjustRowIndices originates from RemoveRowFromMap and that is only
called from DestroyRows and ReverseDestroyRows.  Adding it all up, the symptom
only happens when the first row gets destroyed.

The root cause of the problem appears to be the cut/paste from InsertRowIntoMap
that was used to create RemoveRowFromMap.  In the insert case, you want to
modify the insertion index because it's implemented as an insert-before
(implying that the index inserted-before must be increased.  In the delete case
you don't need to modify at the index since it's being deleted.  Doing so causes
all the indices to be off by one because the row's frame has not itself been
deleted yet (the DestroyRows guys do that when we get back from
RemoveRowFromMap.)

Since both the insert and the delete cases depend on the same routine to adjust
indices, I'm implementing the fix by bumping up the index used for the delete
case - one line of code in RemoveRowFromMap.
Ignore the sentence above about indices being off by one.  The only effect of
this bug is to make an entry have the entry number -1 if you delete the zeroeth
element.  The recommended fix is still the correct fix though.
you may run into other problems. I've found that most scroll-up operations
corrupt the row indicies, usually setting the FIRST row to the index of the last
row + 1 (so if there are 20 rows, the newly inserted row, at row 0, gets a row
index of 20 instead of 0)

I have kind of a hacky workaround for that in my tree, but can you attach your
patch? I'm curious what kind of effects it will have on the scrolling problems
I'm working on.
ok, this looks good.  alecf and i identified the other scrolling problem (not
related to this), and i have a fix for that one in my tree.
I checked in that fix hyatt mentions....but I forgot to see if this is affected.

I'm still waiting for my build - steve, can you still produce this problem? now
that row indicies are not as screwed up as before, I'm wondering if this problem
may have gone away.
I've got a fix in my tree for nsCellMap's SetRowIndex.  That routine was doing
very bad things to indices during row index changes.  It may be related to the
problems you're seeing.  In addition, it makes no attempt to renumber the
indices so I suspect it's contributing to a down-stream problem.  I haven't
fixed the renumbering part yet, but it should be trivial to add that.
wait, the row renumbering was totally busted from the tree's perspective, that's
what dave & fixed yesterday. I don't think that the cellmap should have to
renumber anything.

karnaze, do you have any comments on this stuff?
Sorry, if I misunderstood.  Don't the cellmaps relate directly to the frames in
the table?  Maybe Append/InsertBefore are taking care of it already, that's what
I was going to look into next.  (Remember, I'm still figuring out how all this
stuff relates to each other :-)
Steve, I think you are confusing this bug with the other one you have fixed. You
meant to say nsHTMLTableRowElement::SetRowIndex. nsCellMap doesn't have such a
method. nsCellMap is a matrix of (row,column) entries and is larger than the
content side of things since it considers row and col spans.
In the tree, the cellmaps correspond only to the frames, which correspond to the
actual visible content nodes.... which is different from tables, where all
content nodes have frames.
But, a tree is a table.  Can I get a javascript reference to the table
underneath the tree?  If so, then I can probably do row.rowIndex=<n> on some row
and cause things to get horked.  If we're protecting the table by making it
invisible when it's really a tree then I agree there is no problem.

Is there any form of getElement* that allows me to find the table under the tree
from javascript?
can you attach your patch for the table cellmap? I'm curious if it fixes
something else I'm running into.

    
    

    
  
I have successfully run the regression tests against the attached patch.  No new
failures above baseline.

    
  
Blocks: 18951

    
  
Assignee: alecf → selmer
Status: ASSIGNED → NEW

    
  
Target Milestone: M12 → M13

    
    

    
  
moving to M13

    
  
Assignee: selmer → karnaze

    
    

    
  
Chris, did you ever get a chance to check that rowIndex stuff in?

    
  
Assignee: karnaze → selmer

    
    

    
  
Steve, I have about a month's worth of table work in progress on my machine that
is behind schedule and haven't had time to figure out what you changed based on
the diffs you sent. I would really appreciate it if you could take the bug back.

    
  
Status: NEW → ASSIGNED

    
    

    
  
No problem.  Just wanted to be sure it wasn't completed already.

    
  
Status: ASSIGNED → RESOLVED
Closed: 25 years ago
Resolution: --- → FIXED

    
    

    
  
This appears to be fixed. I have tried a lot of scrolling of bookmarks in the
sidebar and cannot get it to crash.

    
    

    
  
I concur, my testing yesterday yielded the same lack of a result.

    
  
Status: RESOLVED → VERIFIED

    
    

    
  
marking verified

    
  
No longer blocks: 18951

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: