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

I can't reproduce any problem associated with this in the opt builds, reassigning to alecf for triage.

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.

M12. No behavior is described which is bad enough to hold M11 for this.

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.

moving to M13

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

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.

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

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.

marking verified