222468 - Creating a mail filter causes Mozilla to crash [@ nsLayoutUtils::CompareTreePosition]

Status: VERIFIED FIXED

(MailNews Core :: Filters, defect, P1)

Description

[Philip K. Warren]

Steps to reproduce:
1) Create a mail account.
2) Go to Tools->Message Filters.
3) Click "New..." button.

Mozilla crashes at this point on AIX and Linux. I have not had time to test a
Windows build yet.

Comment 1

[Philip K. Warren]

Attachment: Stack trace

This is a stack trace from a non-optimized, non-debug build. I don't have a
debug build handy so unfortunately there are no line numbers listed.

Comment 2

[Philip K. Warren]

The stack trace seems to point to the checkin for Bug 217604 as the cause of
this problem.

Comment 3

[Philip K. Warren]

This is not occuring on a build compiled on the morning of 2003/10/14. This is
occuring in yesterday's build.

Comment 4

[Frank Wein [:mcsmurf]]

this bug also seems to cause Bug 220629 and Bug 221676 (same stacktraces)

Comment 5

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

Mine, for sure

Comment 6

[Tobias Weibel]

Attachment: I saw this bug yesterday and made a quick fix but I'm absolutely not sure if something else should be done, because I'm nof familiar with this code

Comment 7

[Boris Zbarsky [:bzbarsky]]

So I caught this in a debugger.  The crash happens because when we call

CompareTreePosition(aFrame->GetContent(), f->GetContent(), parentViewContent)

in nsLayoutUtils::FindSiblingViewFor, parentViewContent is in fact no an
ancestor of aFrame->GetContent().  At this point, aFrame is a nsMenuFrame

The ancestors of aFrame look like this:

nsMenuFrame < nsDeckFrame < nsBoxFrame < nsListItemFrame < nsListBoxBodyFrame

The problem pops up with nsListItemFrame...

(gdb) whats $foo
0x41948ba8 <_ZTV15nsListItemFrame+8>:   0x41452d3c
<_ZN15nsListItemFrame14QueryInterfaceERK4nsIDPPv>
(gdb) p $foo->GetContent()->GetParent()
$35 = (nsIContent *) 0x89e0210
(gdb) p $foo->GetParent()->GetContent()
$36 = (nsIContent *) 0x8a078e8

$foo->GetContent() is a "listitem"; it's parent is a "listbox".

$foo->GetParent()->GetContent() is a "listboxbody".  The "listbox" is the
content node for a frame ancestor of $foo much higher up in the frame hierarchy
(there are some frames that point to "listboxbody" and "listrows" in between). 
The problem is that the ListBoxBodyFrame is the ancestor with a view, so
aCommonAncestor is the "listboxbody", which is not an ancestor of the "listitem"
at all....

Comment 8

[Philip K. Warren]

Attachment: Debug stack trace

It sounds like the situation is already well understood, but I do have a stack
trace from a debug build available now so I figured I'd go ahead and post it.

Comment 9

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

The best thing to do is to make CompareTreePosition use aCommonAncestor as a
hint. If it turns out it's not an ancestor of aContent1 or aContent2, we can
just retry the function with aCommonAncestor == nsnull, which will always work.
I'll cook up a patch for this tonight.

Comment 10

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

IIRC, there's code somewhere that uses |aCommonAncestor| to prevent walking of
the top (i.e., as the null check for when we get to the root) and will walk off
the top and try to dereference content nodes if it's not actually an ancestor.

Comment 11

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

Attachment: fix

This should do the trick nicely.

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

>+  for (c1 = aContent1; c1 && c1 != aCommonAncestor; c1 = c1->GetParent()) {
>     content1Ancestors.AppendElement(c);

s/c/c1/ in the AppendElement call?

>+  for (c2 = aContent2; c2 && c2 != aCommonAncestor; c2 = c2->GetParent()) {
>     content2Ancestors.AppendElement(c);

s/c/c2/

Looks good to me once it compiles, so r+sr=me, but please do build and test
this patch before landing it...

Comment 13

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

Er, yeah :-)

Comment 14

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

I built and tested the patch, and it fixes the crash. I checked in.

Comment 15

[Frank Wein [:mcsmurf]]

fixed, i tested this with a cvs trunk build, mail filter behave as usual, no
crash when clicking the New... Button (the deps of this bug were also fixed with
this, tested that)

Comment 16

[Tobias Weibel]

verified with
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6a) Gecko/20031018

Comment 17

[Boris Zbarsky [:bzbarsky]]

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