Martijn Wargers (dead) Reporter
	Description • 19 years ago

See testcase.
When clicking on the button, Mozilla crashes.
This regressed between 2005-08-30 and 2005-08-31, indicating bug 212789 as a
likely cause.

	Martijn Wargers (dead) Reporter
	Comment 1 • 19 years ago

When clicking on the button in the testcase, Mozilla crashes.

	Martijn Wargers (dead) Reporter
	Comment 2 • 19 years ago

From talkback ID: TB9946810E

nsTreeBodyFrame::GetRowAt 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,
line 967]
nsTreeBodyFrame::GetRowHeight 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,
line 639]
PresShell::ContentInserted 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 5146]
PresShell::Observe 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6726]
PresShell::Thaw 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.2_Depend/mozilla/layout/base/nsPresShell.cpp,
line 6274]
SHELL32.dll + 0x520c24 (0x778b0c24)

	Ria Klaassen (not reading all bugmail)
	Comment 3 • 19 years ago

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051001
Firefox/1.6a1 ID:2005100108

Small improvement; it does not crash anymore (like this morning) but leaves an
idle process in the task manager that has to be killed.

	Stef Walter
	Comment 4 • 19 years ago

This stack trace is either corrupted or doesn't make sense at all. GetRowHeight
does not call GetRowAt etc...

	Boris Zbarsky [:bzbarsky]
	Comment 5 • 19 years ago

So... the mScrollbar member is pointing to a dead frame.  I see no provisions for resetting that member if the frame dies; am I just missing it?

	Tim Riley [:timr]
	Comment 6 • 19 years ago

crash - nom approved

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 7 • 19 years ago

Here's the patch. It simply removes frame and view references from nsTreeBodyFrame and refetches them whenever they're needed. It's pretty simple, but it looks big because I've altered internal function signatures so we can pass the fetched parts around to avoid redundant fetches.

	neil@parkwaycc.co.uk
	Comment 9 • 19 years ago

Comment on attachment 218229 [details] [diff] [review]
patch; diff -w for review

>+    // The way we do this, searching through the entire frame subtree, is pretty
>+    // dumb! We should know where these frames are.
Theoretically more advanced themes might want to be able to extend the XBL to adjust the appearance of their trees.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 19 years ago

Perhaps, but those more advanced themes would get a nasty surprise if they included an element in their XBL that happened to have overflow:auto, and nsTreeBodyFrame decided to randomly grab those scrollbars and use them to scroll the tree.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 11 • 19 years ago

Comment on attachment 218229 [details] [diff] [review]
patch; diff -w for review

I don't want to wait for Jan; this needs to move into the tree now so I'm going with bz's sr.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 19 years ago

checked in.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 13 • 19 years ago

Backporting to the 1.8 branch won't be trivial because of the changes to add horizontal scrolling to trees, but it shouldn't be terribly hard either.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 19 years ago

Comment on attachment 218230 [details] [diff] [review]
patch for checkin

Looks ok for 1.8.1 branch, but keep an eye out for trunk regressions over the next few days, ok?

	neil@parkwaycc.co.uk
	Comment 15 • 19 years ago

Comment on attachment 218230 [details] [diff] [review]
patch for checkin

>+    // The way we do this, searching through the entire frame subtree, is pretty
>+    // dumb! We should know where these frames are.
Oh, so you'd prefer remote xbl to cause a crash by rearranging the frames?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 16 • 19 years ago

I'd prefer to have a foolproof way to find the right frames if they exist, regardless of any anonymous content thrown into the pot by a theme.

	Stephen Donner [:stephend] Not actively reading bugmail
	Comment 17 • 19 years ago

Testcase https://bugzilla.mozilla.org/attachment.cgi?id=198120 no longer crashes for me using build 2006-04-26-04 of SeaMonkey trunk on Windows XP.

Verified FIXED.

	Daniel Veditz [:dveditz]
	Comment 18 • 19 years ago

This does not crash in Firefox 1.5.0.2, and it appears the regressing patch was checked in on Aug 31, 2005 and the 1.8 branch was cut on Aug 12, 2005. Doesn't look like you need this on the 1.8 

	Daniel Veditz [:dveditz]
	Comment 19 • 19 years ago

Comment on attachment 218230 [details] [diff] [review]
patch for checkin

Too big and too late, and doesn't appear necessary.

Renominating for 1.8.1 for a double-check that you really do still want it.

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 19 years ago

We still want it for 1.8.1 (and imo for 1.8.0.x for whatever x).  If you want roc or me or Martijn to spend some time to create testcases that would crash 1.8 and 1.8.0 branches with similar issues, I'm pretty sure we can do so successfully.  But it seems like rather a waste of time.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 19 years ago

We do need this on the branches, but without a testcase, it's non-urgent and can happily slip to 1.8.0.5.

	Daniel Veditz [:dveditz]
	Comment 22 • 19 years ago

OK, renominating for 1.8.0.5.  Please land on the 1.8 branch soon for testing, and get it in early in 1.8.0.5 when the tree opens for that.

	Daniel Veditz [:dveditz]
	Comment 23 • 18 years ago

Comment on attachment 218230 [details] [diff] [review]
patch for checkin

Given the size of the patch could we get a second pair of review eyes on this? Thanks

	Daniel Veditz [:dveditz]
	Comment 24 • 18 years ago

Could we at least get this landed on the 1.8 branch? You've already got 1.8.1 approval.

	David Baron :dbaron:
	Comment 25 • 18 years ago

Comment on attachment 218230 [details] [diff] [review]
patch for checkin

I didn't look all that closely and I don't think size alone means it needs another review, but r=dbaron.

	Jay Patel [:jay]
	Comment 26 • 18 years ago

roc:  Can you get you patch checked in on the 1.8.1 branch asap so we can let it bake for a few days and approve this for 1.8.0 (code freeze is next Wed. for 1.5.0.5).  Thanks!

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 27 • 18 years ago

It's smaller than the trunk patch. The horizontal scrolling code that landed on trunk made almost everything conflict, but it's still a relatively simple change (I hope).

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 28 • 18 years ago

Checked in on 1.8 branch.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 29 • 18 years ago

There was bustage on the 1.8 branch, around the call to GetWindow() in #ifdef MAC code, which I have fixed, but will need to be merged for 1.8.0.5.

	Daniel Veditz [:dveditz]
	Comment 31 • 18 years ago

Comment on attachment 225825 [details] [diff] [review]
1.8 branch patch

approved for 1.8.0 branch, a=dveditz for drivers

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 32 • 18 years ago

Someone could check this into 1.8 for me. Otherwise I'll do it on (my) Monday.

	Martijn Wargers (dead) Reporter
	Comment 33 • 18 years ago

Checking in nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTree
BodyFrame.cpp
new revision: 1.240.10.1.2.4; previous revision: 1.240.10.1.2.3
done
Checking in nsTreeBodyFrame.h;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.h,v  <--  nsTreeBo
dyFrame.h
new revision: 1.81.16.2; previous revision: 1.81.16.1
done

Checked into 1.8.0 branch.

	Tracy Walker [:tracy]
	Comment 34 • 18 years ago

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.5) Gecko/20060620 Firefox/1.5.0.5