53358 - [FIX IN HAND] crash in FindPreviousAnonymousSibling (nsCSSFrameConstructor.cpp)

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[David :Bienvenu]

I crash in FindPreviousAnonymousSibling (nsCSSFrameConstructor.cpp) occasionally
when selecting the subject text frame after entering an e-mail address, because
xblDoc is null. I don't know why xblDoc is null, but checking for null fixes the
crash, and if I continue in the debugger, I don't run into any more problems.
I'll attach the proposed fix next.

Comment 1

[David :Bienvenu]

Attachment: proposed checks for null

Comment 2

[David :Bienvenu]

rods says this is marc's area, and that waterson and steve would be doing code
review, so reassigning and cc'ing as appropriate.

Comment 3

[Marc Attinasi]

I cannot reproduce the problem, and I've been using Netscape6 Mail and Mozilla
Mail exclusively for days now...

The fix looks good, however, and I can see no reason to NOT check for null. In
fact, I'd check for null on the doc too, before the QI for xblDoc, since it is
not guaranteed that the aContainer content will have a doc (or if it is, then at
least assert it).

Accepting: hopefully this can be reproduced, but if not, the code cleanup patch
is good anyway (safer).

Comment 4

[David :Bienvenu]

It's not easy to recreate - I suspect it is partly caused by the address
completion widget trying to pop up at the wrong time. The larger your history
address book is, the slower this will be. If you've only been running for a few
days, your history address book will be puny :-) I've seen this crash in talback
logs, so other people have seen this.

Comment 5

[buster]

without knowing the code, it seems odd that your patch returns null in the first
case if the xbldoc is null, and in the second case you don't return.  cvs blame
points to hyatt for this code.  run it by him, and if it's ok with him, r=buster.

Comment 6

[Marc Attinasi]

Updating some keyword fields to get this on the nsbeta3 radar

Comment 7

[Marc Attinasi]

I think that either case will return null, the first one returns null directly,
and the second causes the bulk of the method to be skipped and nextSibling is
returned, which will be the initial value of null.

It would probably be best to do the same fix in both cases, and I prefer the
second approach. I'll attach another patch that treats the two cases the same,
and also checks for a null document.

Comment 8

[Marc Attinasi]

Attachment: YAP: treats FindNextAnonymousSibling and FindPreviousAnonymousSibling the same way

Comment 9

[David :Bienvenu]

Steve, I don't need to return in the second case because nodeList will remain
null, and we'll fall through to the end of the routine.

Comment 10

[David Hyatt]

It seems very odd that the document would be null.  It shouldn't be possible for 
us to notify the front end about an element that isn't even in the document.  I 
think there's a deeper problem here.  The null checks are great and will bypass 
the crash, but I'm a bit concerned about how we got into this situation in the 
first place.

Comment 11

[David Hyatt]

Could this be related to joki's hover stuff?  It also causes a crash in which 
there's an element with no document.  He has an nsbeta3+ bug on this: crash in 
mailcompose.

Comment 12

[buster]

hyatt:  how about we check in the patch as a stability enhancement, but leave 
this bug open to find out how we got the null document.  If that research points 
towards joki's bug, great.  But if not, we'll have this to track the issue.  If 
it's useful, the patch could be augmented by adding asserts when we get a null 
xbl doc.  If it can't be null, the assert should be there anyway.

Comment 13

[David Hyatt]

This sounds great to me.  For reference, joki's bug is 53219.  It looks 
suspiciously similar to me.

Comment 14

[Marc Attinasi]

Yea, bug 53219 sure looks like the same problem. Marking dependent, but still
want to check in this patch (actually, a new one with assertions - coming).

Comment 15

[Marc Attinasi]

Attachment: YAP: same as last but with assertions too.

Comment 16

[Jean-Francois Ducarroz]

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

Comment 17

[karnaze (gone)]

Marking nsbeta3+ P1.

Comment 18

[Phil Peterson]

PDT agrees P1

Comment 19

[buster]

a=buster

Comment 20

[Jean-Francois Ducarroz]

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

Comment 21

[lchiang]

upgrading the severity of this bug.

Comment 22

[lchiang]

marking nsbeta3++ per email with pdt.

Comment 23

[Marc Attinasi]

OK - I'll be checking it in on the branch as soon as my branch-build is done and
the tree is open.

Comment 24

[Jean-Francois Ducarroz]

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

Comment 25

[Marc Attinasi]

Fix checked into branch.

Comment 26

[Marc Attinasi]

Checked into trunk too, for good measure.

Comment 27

[lchiang]

great!  I'll try out for mail in tomorrow's build.

Comment 28

[laurel]

Mail cases mentioned in bug 53408 and bug 53851 work for me using sept27 mn6
commercial branch builds, mac OS 9.0, NT 4.0 and linux rh6.0

Comment 29

[Chris Petersen]

Marking verfied per last comments.