The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
XUL
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, 6 keywords)

Trunk
mozilla1.8.1
PowerPC
Mac OS X
crash, testcase, verified1.8.0.10, verified1.8.0.13, verified1.8.1.2, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 -
blocking1.8.1.2 +
blocking1.8.0.7 -
blocking1.8.0.10 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] branch patch, crash signature)

Attachments

(8 attachments)

(Reporter)

Description

11 years ago
This crash looks like a security hole: in a debug build, it crashes trying to access memory at 0xDDDDDDD5.
(Reporter)

Updated

11 years ago
Flags: blocking1.9a1?
Whiteboard: [sg:critical]
(Reporter)

Comment 1

11 years ago
Created attachment 228801 [details]
testcase
(Reporter)

Comment 2

11 years ago
Created attachment 228802 [details]
stack trace (mac debug / gdb)
(Reporter)

Comment 3

11 years ago
Nightlies crash too:

  Thread 0 Crashed:
  nsTreeBodyFrame::VisibilityChanged
  nsGfxScrollFrameInner::SetScrollbarVisibility
  nsXULScrollFrame::AddRemoveScrollbar
  ...
(Reporter)

Comment 4

11 years ago
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
...
Flags: blocking1.8.0.6?
(Reporter)

Updated

11 years ago
Blocks: 344486
In a debug 1.5.0.5 build I get a deleted nsIScrollbarMediator in nsGfxScrollFrameInner::SetScrollbarVisibility
Flags: blocking1.8.1?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Assignee: Jan.Varga → dbaron
Flags: blocking1.8.0.6? → blocking1.8.0.6+
Whiteboard: [sg:critical] → [sg:critical][at risk]
Target Milestone: --- → mozilla1.8.1
Created attachment 234812 [details]
first valgrind warning
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.)
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.
Whiteboard: [sg:critical][at risk] → [sg:critical][at risk][patch]
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?
Whiteboard: [sg:critical][at risk][patch] → [sg:critical][at risk][patch] needs reviews
Moving to 1.8.0.8 given the "at risk" designation.
Flags: blocking1.8.0.8+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+

Comment 11

11 years ago
Moving out from 1.8.1 to 1.8.1.1
Flags: blocking1.8.1.1?
Flags: blocking1.8.1-
Flags: blocking1.8.1+
Restoring lost blocking flag
Flags: blocking1.8.0.8?
Flags: blocking1.8.0.8? → blocking1.8.0.9?
What would it take to get this bug moving again? Sounds like we know what to do here.
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+

Comment 14

11 years ago
dbaron, did you want someone to review this patch?

Comment 15

11 years ago
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!
Attachment #234862 - Flags: superreview?(roc)
Attachment #234862 - Flags: review?(roc)
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.
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.
Assignee: dbaron → roc
Status: NEW → ASSIGNED
Attachment #247136 - Flags: superreview?(bzbarsky)
Attachment #247136 - Flags: review?(bzbarsky)
Comment on attachment 247136 [details] [diff] [review]
my patch

mats, bz, whichever one of you gets to this first :-)
Attachment #247136 - Flags: review?(bzbarsky) → review?(mats.palmgren)
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.
Attachment #247136 - Flags: superreview?(bzbarsky)
Attachment #247136 - Flags: superreview+
Attachment #247136 - Flags: review?(mats.palmgren)
Attachment #247136 - Flags: review+
(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.
Moving out to next release per comment 20
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Whiteboard: [sg:critical][at risk][patch] needs reviews → [sg:critical] needs trunk landing, branch patch

Comment 22

11 years ago
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

11 years ago
Comment on attachment 247136 [details] [diff] [review]
my patch

minus because patch doesn't build
Attachment #247136 - Flags: review+ → review-
Created attachment 247767 [details] [diff] [review]
my patch, fixed

Does this look good now?
Attachment #247767 - Flags: superreview+
Attachment #247767 - Flags: review?(enndeakin)

Comment 25

10 years ago
Comment on attachment 247767 [details] [diff] [review]
my patch, fixed

looks good now
Attachment #247767 - Flags: review?(enndeakin) → review+
Checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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 :-)
Attachment #247866 - Flags: approval1.8.1.2?
Attachment #247866 - Flags: approval1.8.0.10?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10+
Whiteboard: [sg:critical] needs trunk landing, branch patch → [sg:critical] branch patch
(Reporter)

Updated

10 years ago
Depends on: 363848
Attachment #234862 - Flags: superreview?(roc)
Attachment #234862 - Flags: review?(roc)

Comment 28

10 years ago
Comment on attachment 247866 [details] [diff] [review]
branch patch

Approved for both branches, a=jay for drivers.
Attachment #247866 - Flags: approval1.8.1.2?
Attachment #247866 - Flags: approval1.8.1.2+
Attachment #247866 - Flags: approval1.8.0.10?
Attachment #247866 - Flags: approval1.8.0.10+
fixed on branches
Keywords: fixed1.8.0.10, fixed1.8.1.2
Depends on: 366112
This caused bug 366112, since there might not be a scrollbar (the old code warned and bailed in that case).
Depends on: 365234
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.
Keywords: fixed1.8.1.2 → verified1.8.1.2
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.
Keywords: fixed1.8.0.10 → verified1.8.0.10
Group: security

Updated

10 years ago
Flags: blocking1.9a1?
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
Yes, great catch.
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.
Attachment #270525 - Flags: superreview?(mats.palmgren)
Attachment #270525 - Flags: review?(mats.palmgren)
Comment on attachment 270525 [details] [diff] [review]
fix

r=mats  (I'm not a superreviewer though)
Attachment #270525 - Flags: superreview?(mats.palmgren)
Attachment #270525 - Flags: review?(mats.palmgren)
Attachment #270525 - Flags: review+
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 on attachment 270525 [details] [diff] [review]
fix

Ok, cool.  r+sr=mats then ;-)
Attachment #270525 - Flags: superreview+
Comment on attachment 270525 [details] [diff] [review]
fix

fixes a major (possibly *huge*) Mac regression
Attachment #270525 - Flags: approval1.8.1.5?
Attachment #270525 - Flags: approval1.8.0.13?
Flags: in-testsuite?
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.
Attachment #270525 - Flags: approval1.8.1.5?
Attachment #270525 - Flags: approval1.8.1.5+
Attachment #270525 - Flags: approval1.8.0.13?
Attachment #270525 - Flags: approval1.8.0.13+
adding fixed1.8.1.5 keyword to get off the "approved but not landed" query.
Keywords: fixed1.8.1.5
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.
Keywords: fixed1.8.1.5 → verified1.8.1.5

Comment 43

10 years ago
adding fixed1.8.0.13 as the fix been checked in on 1.8.0 branch as well.
Keywords: fixed1.8.0.13
Depends on: 386607

Updated

10 years ago
Depends on: 388583
I was not able to reproduce the problem on Thunderbird 15012 or 15013, so leaving keyword (fixed1.8.0.13) unchanged.
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
Keywords: fixed1.8.0.13 → verified1.8.0.13
(Reporter)

Comment 46

9 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+
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
Status: RESOLVED → VERIFIED

Updated

9 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Crash Signature: [@ nsTreeBodyFrame::VisibilityChanged] [@ nsGfxScrollFrameInner::SetScrollbarVisibility]
You need to log in before you can comment on or make changes to this bug.