344228 - Crash [@ nsTreeBodyFrame::VisibilityChanged] [@ nsGfxScrollFrameInner::SetScrollbarVisibility]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

This crash looks like a security hole: in a debug build, it crashes trying to access memory at 0xDDDDDDD5.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack trace (mac debug / gdb)

Comment 3

[Jesse Ruderman]

Nightlies crash too:

  Thread 0 Crashed:
  nsTreeBodyFrame::VisibilityChanged
  nsGfxScrollFrameInner::SetScrollbarVisibility
  nsXULScrollFrame::AddRemoveScrollbar
  ...

Comment 4

[Jesse Ruderman]

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]]

In a debug 1.5.0.5 build I get a deleted nsIScrollbarMediator in nsGfxScrollFrameInner::SetScrollbarVisibility

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: first valgrind warning

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: 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 [:bzbarsky]]

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]]

Moving to 1.8.0.8 given the "at risk" designation.

Comment 11

[Mike Schroepfer]

Moving out from 1.8.1 to 1.8.1.1

Comment 12

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 13

[Daniel Veditz [:dveditz]]

What would it take to get this bug moving again? Sounds like we know what to do here.

Comment 14

[Neil Deakin]

dbaron, did you want someone to review this patch?

Comment 15

[Jay Patel [:jay]]

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)]

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)]

Attachment: 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)]

Comment on attachment 247136 [details] [diff] [review]
my patch

mats, bz, whichever one of you gets to this first :-)

Comment 19

[Boris Zbarsky [:bzbarsky]]

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 [e-mail]");

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)]

(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 [e-mail]");
> 
> 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]]

Moving out to next release per comment 20

Comment 22

[Neil Deakin]

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]

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)]

Attachment: my patch, fixed

Does this look good now?

Comment 25

[Neil Deakin]

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)]

Checked into trunk.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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]]

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)]

fixed on branches

Comment 30

[Boris Zbarsky [:bzbarsky]]

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]]

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]]

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 (inactive)]

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)]

Yes, great catch.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: 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 (inactive)]

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)]

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 (inactive)]

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)]

Comment on attachment 270525 [details] [diff] [review]
fix

fixes a major (possibly *huge*) Mac regression

Comment 40

[Daniel Veditz [:dveditz]]

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]]

adding fixed1.8.1.5 keyword to get off the "approved but not landed" query.

Comment 42

[Stephen Donner [:stephend] Not actively reading bugmail]

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]

adding fixed1.8.0.13 as the fix been checked in on 1.8.0 branch as well.

Comment 44

[juan becerra [:juanb]]

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]]

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]

Crashtest checked in.

Comment 47

[Stephen Donner [:stephend] Not actively reading bugmail]

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