Last Comment Bug 344228 - Crash [@ nsTreeBodyFrame::VisibilityChanged] [@ nsGfxScrollFrameInner::SetScrollbarVisibility]
: Crash [@ nsTreeBodyFrame::VisibilityChanged] [@ nsGfxScrollFrameInner::SetScr...
Status: VERIFIED FIXED
[sg:critical] branch patch
: crash, testcase, verified1.8.0.10, verified1.8.0.13, verified1.8.1.2, verified1.8.1.5
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: PowerPC Mac OS X
: -- critical (vote)
: mozilla1.8.1
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
Mentors:
Depends on: 363848 365234 366112 386607 388583
Blocks: 344486
  Show dependency treegraph
 
Reported: 2006-07-11 05:45 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
11 users (show)
mtschrep: blocking1.8.1-
dveditz: blocking1.8.1.2+
dveditz: blocking1.8.0.7-
dveditz: blocking1.8.0.10+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (427 bytes, application/vnd.mozilla.xul+xml)
2006-07-11 05:46 PDT, Jesse Ruderman
no flags Details
stack trace (mac debug / gdb) (10.29 KB, text/plain)
2006-07-11 05:47 PDT, Jesse Ruderman
no flags Details
first valgrind warning (11.71 KB, text/plain; charset=utf-8)
2006-08-21 11:57 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
possible patch (3.27 KB, patch)
2006-08-21 15:44 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
my patch (27.98 KB, patch)
2006-11-30 19:33 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
enndeakin: review-
bzbarsky: superreview+
Details | Diff | Splinter Review
my patch, fixed (27.45 KB, patch)
2006-12-06 19:52 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
enndeakin: review+
roc: superreview+
Details | Diff | Splinter Review
branch patch (16.21 KB, patch)
2006-12-07 12:48 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jaymoz: approval1.8.1.2+
jaymoz: approval1.8.0.10+
Details | Diff | Splinter Review
fix (2.89 KB, patch)
2007-07-01 15:46 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
mats: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Description Jesse Ruderman 2006-07-11 05:45:26 PDT
This crash looks like a security hole: in a debug build, it crashes trying to access memory at 0xDDDDDDD5.
Comment 1 Jesse Ruderman 2006-07-11 05:46:34 PDT
Created attachment 228801 [details]
testcase
Comment 2 Jesse Ruderman 2006-07-11 05:47:25 PDT
Created attachment 228802 [details]
stack trace (mac debug / gdb)
Comment 3 Jesse Ruderman 2006-07-11 05:49:50 PDT
Nightlies crash too:

  Thread 0 Crashed:
  nsTreeBodyFrame::VisibilityChanged
  nsGfxScrollFrameInner::SetScrollbarVisibility
  nsXULScrollFrame::AddRemoveScrollbar
  ...
Comment 4 Jesse Ruderman 2006-07-11 06:29:11 PDT
Crashes Firefox 1.5.0.4 (release build) too, with a bogus-looking top frame.

Thread 0 Crashed:
0    FbMergeRopBits + 193596
1    nsGfxScrollFrameInner::SetScrollbarVisibility(nsIFrame*, int) + 148
2    nsGfxScrollFrameInner::SetScrollbarVisibility(nsIFrame*, int) + 148
3    nsXULScrollFrame::AddRemoveScrollbar(nsBoxLayoutState&, nsRect&, int, int, int) + 108
...
Comment 5 Daniel Veditz [:dveditz] 2006-07-13 16:40:20 PDT
In a debug 1.5.0.5 build I get a deleted nsIScrollbarMediator in nsGfxScrollFrameInner::SetScrollbarVisibility
Comment 6 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-08-21 11:57:39 PDT
Created attachment 234812 [details]
first valgrind warning
Comment 7 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-08-21 15:28:33 PDT
It seems like the basic problem here is that the treebody frame is adding a weak pointer to itself to a frame that's not a descendant of the primary frame for the content node for which it is a frame.  (That definition allows a descendant of a wrapping scrollframe, but not a descendant of a higher ancestor.)
Comment 8 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2006-08-21 15:44:15 PDT
Created attachment 234862 [details] [diff] [review]
possible patch

This fixes this crash.  Groveling through the frame tree during frame destruction like this scares me a bit, though.
Comment 9 Boris Zbarsky [:bz] (TPAC) 2006-08-21 16:05:19 PDT
If we want to be safe, could we either use an nsWeakFrame or give the random other frame a strong ref to something that we also hold on to and notify when we go away?
Comment 10 Daniel Veditz [:dveditz] 2006-08-24 11:44:39 PDT
Moving to 1.8.0.8 given the "at risk" designation.
Comment 11 Mike Schroepfer 2006-09-11 16:21:32 PDT
Moving out from 1.8.1 to 1.8.1.1
Comment 12 Daniel Veditz [:dveditz] 2006-09-19 15:37:36 PDT
Restoring lost blocking flag
Comment 13 Daniel Veditz [:dveditz] 2006-11-03 11:20:28 PST
What would it take to get this bug moving again? Sounds like we know what to do here.
Comment 14 Neil Deakin (away until Oct 3) 2006-11-13 17:57:59 PST
dbaron, did you want someone to review this patch?
Comment 15 Jay Patel [:jay] 2006-11-30 13:04:59 PST
dbaron is away on travel and we need traction here if it's going to make 1.8.0.9/1.8.1.1.  Who else can take this bug?

Anyone in this bug willing to take a look at the patch and give the necessary reviews?  If we feel good about the patch, please ask for approvals asap.  Thanks!
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-30 17:56:44 PST
I think the right approach is for SetScrollbarMediator to take an nsIContent* and for nsScrollbarFrame to hold a strong ref to the content. Whenever it needs the nsIScrollbarMediator it can do a GetPrimaryFrameFor followed by a QI.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-30 19:33:37 PST
Created attachment 247136 [details] [diff] [review]
my patch

I prefer this patch. Does basically what I said above: hold a strong ref to nsIContent instead of a pointer to a frame.

I also found that there's a lurking problem with native scrollbars --- we pass the mediator (frame) pointer down to the native widget and use it from there! eek! Even worse, clearing the mediator pointer out of the scrollbar doesn't remove it from the widget! This patch fixes that too, by interposing the native scrollbar frame as a proxy mediator which does the necessary dynamic checks. The native scrollbar frame can safely clear the reference to itself when it dies.
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-30 19:34:48 PST
Comment on attachment 247136 [details] [diff] [review]
my patch

mats, bz, whichever one of you gets to this first :-)
Comment 19 Boris Zbarsky [:bz] (TPAC) 2006-11-30 21:01:01 PST
Comment on attachment 247136 [details] [diff] [review]
my patch

>Index: layout/generic/nsGfxScrollFrame.cpp
>+    NS_ASSERTION(PR_FALSE, "This code shouldn't be hit; alert robert@ocallahan.org");

NS_ERROR?

>Index: layout/xul/base/src/nsIScrollbarFrame.h

Note that for branch we don't want the API change here.

>Index: layout/xul/base/src/nsNativeScrollbarFrame.cpp
> // AttributeChanged
...
>-// our native scrollbar with the correct values.
>+// our native scrollbar with the correct values.aScrollbarFrame

Undo this change?

Don't you need to change the QI impl for nsNativeScrollbarFrame to QI to nsIScrollbarMediator?

r+sr=bzbarsky with that.
Comment 20 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-11-30 21:22:34 PST
(In reply to comment #19)
> (From update of attachment 247136 [details] [diff] [review] [edit])
> >Index: layout/generic/nsGfxScrollFrame.cpp
> >+    NS_ASSERTION(PR_FALSE, "This code shouldn't be hit; alert robert@ocallahan.org");
> 
> NS_ERROR?

OK

> >Index: layout/xul/base/src/nsIScrollbarFrame.h
> 
> Note that for branch we don't want the API change here.

Hmm, yes. So the branch fix will be slightly more convoluted (SetScrollbarMediator will need to QI the mediator to nsIFrame and then get the content).

> >Index: layout/xul/base/src/nsNativeScrollbarFrame.cpp
> > // AttributeChanged
> ...
> >-// our native scrollbar with the correct values.
> >+// our native scrollbar with the correct values.aScrollbarFrame
> 
> Undo this change?

Yeah oops!

> Don't you need to change the QI impl for nsNativeScrollbarFrame to QI to
> nsIScrollbarMediator?

I do. Good catch. This is definitely going to need trunk baking to make sure that this doesn't break Mac, so it's not 1.8.0.9 material.
Comment 21 Daniel Veditz [:dveditz] 2006-11-30 23:29:34 PST
Moving out to next release per comment 20
Comment 22 Neil Deakin (away until Oct 3) 2006-12-01 14:37:41 PST
Comment on attachment 247136 [details] [diff] [review]
my patch

This patch doesn't compile.

>Index: layout/xul/base/src/nsNativeScrollbarFrame.cpp
>+  nsIFrame* f;
>+  for (f = GetParent(); f; f = f->GetParent());
>+    nsIContent* currContent = f->GetContent();
>+

Extra semicolon on the for loop

> 
>-  scrollbar->SetContent(scrollbarContent, sb, mediator);
>+  Parts = FindParts();

Can't get these parts with the missing parts.

> 
>   if (!scrollbarContent)
>     return;

scrollbarContent isn't declared.

> 
>   // Check to see if the curpos attribute is already present on the content
>   // node. If so, notify the scrollbar.
> 
>   nsAutoString value;
>   scrollbarContent->GetAttr(kNameSpaceID_None, nsXULAtoms::curpos, value);

Same here.

I can confirm though that this patch fixes a crash in NativeScrollbarFrame::Hookup I just encountered.
Comment 23 Neil Deakin (away until Oct 3) 2006-12-05 14:05:37 PST
Comment on attachment 247136 [details] [diff] [review]
my patch

minus because patch doesn't build
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-12-06 19:52:27 PST
Created attachment 247767 [details] [diff] [review]
my patch, fixed

Does this look good now?
Comment 25 Neil Deakin (away until Oct 3) 2006-12-07 07:34:16 PST
Comment on attachment 247767 [details] [diff] [review]
my patch, fixed

looks good now
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-12-07 11:54:15 PST
Checked into trunk.
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-12-07 12:48:17 PST
Created attachment 247866 [details] [diff] [review]
branch patch

This is the branch version that doesn't change nsIScrollbarFrame. It's a bit simpler although it leaves the code in a messier state, but that's OK for branch :-)
Comment 28 Jay Patel [:jay] 2006-12-20 15:10:32 PST
Comment on attachment 247866 [details] [diff] [review]
branch patch

Approved for both branches, a=jay for drivers.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-12-21 13:41:36 PST
fixed on branches
Comment 30 Boris Zbarsky [:bz] (TPAC) 2007-01-08 11:23:53 PST
This caused bug 366112, since there might not be a scrollbar (the old code warned and bailed in that case).
Comment 31 Marcia Knous [:marcia - use ni] 2007-01-11 16:53:30 PST
verified on the 1.8 branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.1.2pre) Gecko/20070111 BonEcho/2.0.0.2pre. Testcase in Comment 1 does not crash. Adding verified keyword.
Comment 32 Marcia Knous [:marcia - use ni] 2007-01-11 16:58:19 PST
verified on the 1.8.0 branch using Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.10pre) Gecko/20070111 Firefox/1.5.0.10pre. Testcase in Comment 1 does not crash. Adding relevant keyword.
Comment 33 Mats Palmgren (:mats) 2007-07-01 14:11:50 PDT
roc, I think there is an error in the branch patch.
void nsNativeScrollbarFrame::Destroy() should probably be
NS_IMETHOD Destroy(nsPresContext* aPresContext) on branches.
Your intention was to hook up to frame destruction, right?
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsNativeScrollbarFrame.cpp&rev=1.22.24.2&root=/cvsroot&mark=137-138#136
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-07-01 15:30:17 PDT
Yes, great catch.
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-07-01 15:46:53 PDT
Created attachment 270525 [details] [diff] [review]
fix

Fix the branch.

Not calling the superclass destructor would also have been a problem on trunk, except this code no longer exists on trunk.
Comment 36 Mats Palmgren (:mats) 2007-07-01 15:57:24 PDT
Comment on attachment 270525 [details] [diff] [review]
fix

r=mats  (I'm not a superreviewer though)
Comment 37 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-07-01 16:12:50 PDT
Yes you are. I nominated you, Boris seconded, and Brendan agreed. He hasn't updated the Web page yet but that's just a technicality :-)
Comment 38 Mats Palmgren (:mats) 2007-07-01 16:43:11 PDT
Comment on attachment 270525 [details] [diff] [review]
fix

Ok, cool.  r+sr=mats then ;-)
Comment 39 Robert O'Callahan (:roc) (email my personal email if necessary) 2007-07-01 17:39:43 PDT
Comment on attachment 270525 [details] [diff] [review]
fix

fixes a major (possibly *huge*) Mac regression
Comment 40 Daniel Veditz [:dveditz] 2007-07-02 10:55:11 PDT
Comment on attachment 270525 [details] [diff] [review]
fix

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Since tracking branch fixes is hard enough with keyword hacks, created a new bug 386607 to track landing this thing.
Comment 41 Daniel Veditz [:dveditz] 2007-07-08 17:55:42 PDT
adding fixed1.8.1.5 keyword to get off the "approved but not landed" query.
Comment 42 Stephen Donner [:stephend] 2007-07-10 13:51:18 PDT
Replacing fixed1.8.1.5 keyword with verified1.8.1.5 to indicate its status as VERIFIED FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.5pre) Gecko/20070710 BonEcho/2.0.0.5pre.  Will verify trunk later.
Comment 43 Alexander Sack 2007-07-13 08:09:01 PDT
adding fixed1.8.0.13 as the fix been checked in on 1.8.0 branch as well.
Comment 44 juan becerra [:juanb] 2007-08-22 16:24:13 PDT
I was not able to reproduce the problem on Thunderbird 15012 or 15013, so leaving keyword (fixed1.8.0.13) unchanged.
Comment 45 Carsten Book [:Tomcat] 2007-08-23 07:50:35 PDT
verified fixed 1.8.0.13 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre

No crash on testcase - adding verified keyword
Comment 46 Jesse Ruderman 2007-12-18 16:27:49 PST
Crashtest checked in.
Comment 47 Stephen Donner [:stephend] 2008-01-25 02:19:56 PST
Verified on trunk, too: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012404 Minefield/3.0b3pre

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