291531 - Crash [@ nsTreeBodyFrame::PrefillPropertyArray] treecols:hover {display:none} with evil xul testcase

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Could be related to bug 284252, perhaps.

The upcoming testcase crashes my most recent trunk build when hovering over the
text.

Talkback ID: TB5286683X

nsTreeBodyFrame::PrefillPropertyArray 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,
line 1592]
nsTreeBodyFrame::PaintColumn 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,
line 2161]
nsTreeBodyFrame::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,
line 2109]
PresShell::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsPresShell.cpp,
line 5768]
nsView::Paint 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsView.cpp, line
316]
nsViewManager::RenderDisplayListElement 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 1464]
nsViewManager::RenderViews 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 1379]
nsViewManager::Refresh 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 938]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsViewManager.cpp,
line 2037]
HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/view/src/nsView.cpp, line
174]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1180]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 4384]
nsWindow::WindowProc 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/widget/src/windows/nsWindow.cpp,
line 1472]
USER32.dll + 0x27b17 (0x77d37b17)
USER32.dll + 0x2cdce (0x77d3cdce)
USER32.dll + 0x459d (0x77d1459d)
USER32.dll + 0x47b4 (0x77d147b4)
ntdll.dll + 0x2589f (0x77f6589f)
USER32.dll + 0x9611 (0x77d19611)
nsAppStartup::Run 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/toolkit/components/startup/src/nsAppStartup.cpp,
line 145]
main 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/browser/app/nsBrowserApp.cpp,
line 60]
kernel32.dll + 0x1eb69 (0x77e5eb69)

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Jesse Ruderman]

Attachment: another testcase

I hit the same crash while playing with bug 306663.  Here's my testcase.  It's
pretty similar to the other testcase, but it uses removeChild instead of
display:none.

Comment 3

[Jesse Ruderman]

This might be exploitable.  I've seen crashes that I think are related with 0
and 8 at the top of the stack.

Comment 4

[[e-mail]]

Comment on attachment 197166 [details]
another testcase

I couldn't get this one to crash.

Comment 5

[[e-mail]]

Comment on attachment 181579 [details]
Testcase

Well, this crash happens because of a stale scrollable view pointer.

Comment 6

[Jesse Ruderman]

I've seen crashes that seem related with 0xf7fff7fc and ShowProfileManager at
the top of the stack, so I'm marking this bug as [sg:fix].

Comment 7

[Stef Walter]

Attachment: Le Patch

This should fix the problem.

Comment 8

[Mike Schroepfer]

Nate - since you are taking care of it assigning to you.

Comment 9

[[e-mail]]

Comment on attachment 197725 [details] [diff] [review]
Le Patch

Passing the buck to someone more familiar with scrolling views ;-)

Comment 10

[Stef Walter]

Niel, if you have a moment could you commit this? I don't have commit access. 

Comment 11

[Mike Connor [:mconnor]]

landed on trunk, roc, is this pretty safe for branch?

Comment 12

[[e-mail]]

No, this was a trunk-only regression.

Comment 13

[Jesse Ruderman]

The first testcase crashes for me using yesterday's Gecko 1.8 branch build of
Firefox on WinXP.

Comment 14

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

I think this could be taken on branch if it helps there.

Comment 15

[Martijn Wargers (dead)]

Yes, it helps the 1.8 branch. See also the date when I filed this, which was way
before the 1.9a branch opened.

Comment 16

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

Comment on attachment 197725 [details] [diff] [review]
Le Patch

see comments.

Comment 17

[Asa Dotzler [:asa]]

Comment on attachment 197725 [details] [diff] [review]
Le Patch

too scary for a not-topcrasher.

Comment 18

[Boris Zbarsky [:bzbarsky]]

The problem is that like a lot of our other crashes this is likely to be
exploitable...

Comment 19

[Stef Walter]

How is this exploitable? It's a crash caused by a dangling pointer. 

Comment 20

[Jesse Ruderman]

Dangling pointer situations are often exploitable if the code calls a virtual
function on the object, because calling a virtual function involves looking up a
function address using memory that is part of the deleted object object.

Comment 21

[Stef Walter]

Makes sense. And since nsIScrollableView is an interface obviously all calls on
the dangling pointer are virtual. So, it's theoretically (extremely
theoretically) exploitable. 

Comment 22

[Boris Zbarsky [:bzbarsky]]

Jesse, if you think security concerns warrant landing this on the branch, please
rerequest approval, ok?

Comment 23

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

Still crashing in today's SeaMonkey and Firefox Windows XP builds.  I'll attach
the "new" stack; I say "new" because this has always crashed, it's just in a
slightly different area now (same method, though).

Comment 24

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

Attachment: Stack

Comment 25

[Jesse Ruderman]

I see the same crash as Stephen on the first testcase using Mozilla/5.0
(Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20051003 Firefox/1.6a1.

Comment 26

[Stef Walter]

Hmmm, interesting. I may be missing something but....

- Although similar, this isn't the bug (or stack trace) which I 
  reproduced and fixed. 
- It looks like it is caused by dangling pointers in the nsTreeColumn 
  code which Jan Varga wrote. Reassigning. The fact that it happens now 
  and didn't happen earlier, is probably just due to pure luck (earlier) 
  and internal memory heap changes etc...
- This doesn't crash for me which is a bummer.

Comment 27

[Jan Varga [:janv]]

Nice, let's assign all crashers to original authors of the code.
Anyway, I'll take a look on linux.

Comment 28

[Jan Varga [:janv]]

Attachment: patch

Comment 29

[Stef Walter]

Over the years I've gotten frustrated with the progress on the horizontal
scrolling bug, and I guess Mozilla development in general. I just want to get
this over and done with so I can move on. At this point I'd really rather never
see another nsCOMPtr or nsAutoString again.

But that's no excuse for being belligerent. Sorry bout that. 

Comment 30

[Jan Varga [:janv]]

I'm sorry too, what's so bad on smart pointers?

Comment 31

[[e-mail]]

(In reply to comment #30)
>I'm sorry too, what's so bad on smart pointers?
Dunno, I was wondering whether mFirstColumn should be an nsCOMPtr...

Comment 32

[Stef Walter]

I was referring to two classes one commonly interacts with when programming
Mozilla. ie: Would rather be hacking something else at this point. 

Comment 33

[[e-mail]]

(In reply to comment #31)
>(In reply to comment #30)
>>I'm sorry too, what's so bad on smart pointers?
>Dunno, I was wondering whether mFirstColumn should be an nsCOMPtr...
Sorry, I mean an nsRefPtr of course.

Comment 34

[[e-mail]]

Comment on attachment 198432 [details] [diff] [review]
patch

Well, it doesn't crash for me either way, but the patch generally improves the
code - although perhaps not as well as an nsRefPtr ;-)

Comment 35

[Asa Dotzler [:asa]]

did "Le Patch" land on the 1.8 branch? 

Comment 36

[Jan Varga [:janv]]

Comment on attachment 198432 [details] [diff] [review]
patch

checked in

Comment 37

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

I'd resolve this as VERIFIED FIXED, but since I can't jump from NEW to VERIFIED
in one step, Jan, can you please mark this FIXED?

Comment 38

[Jan Varga [:janv]]

sure

Comment 39

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

Thanks, Jan.  Verified FIXED using SeaMonkey 1.1a;Mozilla/5.0 (Windows; U;
Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051007 Mozilla/1.0

Comment 40

[Jesse Ruderman]

I think the second patch is still needed on the Gecko 1.8 branch.  See bug 313539.

Comment 41

[[e-mail]]

(In reply to comment #40)
>I think the second patch is still needed on the Gecko 1.8 branch. See bug 313539.
That's quite possible; the first crash patch was against 1.9-only code.

Comment 42

[Asa Dotzler [:asa]]

neil and bz, can you comment on the risk of taking this patch on the branch?

Comment 43

[Boris Zbarsky [:bzbarsky]]

I don't really know this code well enough to evaluate risk.  :(

Comment 44

[Scott MacGregor]

Neil and I talked about this briefly over IM. If everyone thinks this is a serious, exploitable bug then we should take this fix today on the branch. 

Comment 45

[Scott MacGregor]

Now I'm starting to get confused about the two patches, the more I look at this. I think we need Jan to comment here.

The first patch (le Patch) is needed on the trunk only? 

The second patch will still fix the original test case on the branch (which I can reproduce on the branch)?

Comment 46

[Jesse Ruderman]

Applying the patch to the branch works, with these warnings for nsTreeColumns.cpp:

Hunk #5 succeeded at 513 with fuzz 2 (offset -1 lines).
Hunk #6 succeeded at 536 with fuzz 2 (offset -1 lines).

Before applying the patch to my branch tree, I was able to reproduce this crash with an opt build, but not with a debug build.  After applying the patch, I can no longer reproduce the crash with an opt build.  I tested using both the first testcase here and the testcase in bug 313539.

Comment 47

[Jesse Ruderman]

*** Bug 313539 has been marked as a duplicate of this bug. ***

Comment 48

[Jan Varga [:janv]]

(In reply to comment #45)
> The first patch (le Patch) is needed on the trunk only? 

yes, there were quite big changes on the trunk (support for horizontal scrolling)

> 
> The second patch will still fix the original test case on the branch (which I
> can reproduce on the branch)?
> 

yes, I think this patch fixes the real problem with hidden cols

Comment 49

[Mike Schroepfer]

Jan - if the last patch fixes the problem on the branch can you land it there?  

Comment 50

[Jesse Ruderman]

Checked in on MOZILLA_1_8_BRANCH.

Comment 51

[Bob Clary [:bc] (inactive)]

no crash firefox 1.5 rc2 winxp/linux