96291 - assert when entering address [reply-all], don't get autocomplete popup on the first autcomplete - Trunk [@ nsOutlinerBodyFrame::SetVisibleScrollbar]

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[sairuh (rarely reading bugmail)]

found using 2001.08.21.08-comm on linux.

to repro:
1. click Reply All button to bring up message compose window.
2. drag-selected one of the CC: addresses and deleted it [it was my own address;
i didn't want to get an extra copy].
3. in the now-empty CC: line, start typing another email address.

result: crash.

talkback report #34331833:


 Stack Signature     nsOutlinerBodyFrame::SetVisibleScrollbar() 1816c48e 
 Trigger Time             2001-08-21 12:28:40  
 Trigger Reason              SIGSEGV: Segmentation Fault: (signal 11)

nsOutlinerBodyFrame::SetVisibleScrollbar() 
nsOutlinerBodyFrame::SetView() 
nsOutlinerBodyFrame::Reflow() 
nsBoxToBlockAdaptor::Reflow() 
nsBoxToBlockAdaptor::Reflow() 
nsBoxToBlockAdaptor::GetPrefSize() 
nsSprocketLayout::GetPrefSize() 
nsContainerBox::GetPrefSize() 
nsBoxFrame::GetPrefSize() 
nsSprocketLayout::GetPrefSize() 
nsContainerBox::GetPrefSize() 
nsBoxFrame::GetPrefSize() 
nsSprocketLayout::GetPrefSize() 
nsContainerBox::GetPrefSize() 
nsBoxFrame::GetPrefSize() 
nsSprocketLayout::PopulateBoxSizes() 
nsSprocketLayout::Layout() 
nsContainerBox::DoLayout() 
nsBoxFrame::DoLayout() 
nsBox::Layout() 
nsSprocketLayout::Layout() 
nsContainerBox::DoLayout() 
nsBoxFrame::DoLayout() 
nsBox::Layout() 
nsSprocketLayout::Layout() 
nsContainerBox::DoLayout() 
nsBoxFrame::DoLayout() 
nsBox::Layout() 
nsPopupSetFrame::DoLayout() 
nsBox::Layout() 
nsSprocketLayout::Layout() 
nsContainerBox::DoLayout() 
nsBoxFrame::DoLayout() 
nsBox::Layout() 
nsStackLayout::Layout() 
nsContainerBox::DoLayout() 
nsBoxFrame::DoLayout() 
nsBox::Layout() 
nsBoxFrame::Reflow() 
nsRootBoxFrame::Reflow() 
nsContainerFrame::ReflowChild() 
ViewportFrame::Reflow() 
nsHTMLReflowCommand::Dispatch() 
PresShell::ProcessReflowCommand() 
PresShell::ProcessReflowCommands() 
HandlePLEvent() 
PL_HandleEvent() 
PL_ProcessEventsBeforeID() 
processQueue() 
nsVoidArray::EnumerateForwards() 
nsAppShell::ProcessBeforeID() 
handle_gdk_event() 
libgdk-1.2.so.0 + 0x174db (0x4030f4db) 
libglib-1.2.so.0 + 0x10186 (0x4033f186) 
libglib-1.2.so.0 + 0x10751 (0x4033f751) 
libglib-1.2.so.0 + 0x108f1 (0x4033f8f1) 
libgtk-1.2.so.0 + 0x8c5b9 (0x402635b9) 
nsAppShell::Run() 
nsAppShellService::Run() 
main1() 
main() 
libc.so.6 + 0x189cb (0x404389cb)

Comment 1

[sairuh (rarely reading bugmail)]

this is a regression, as this is a task i've done quite frequently w/o crashing.

pls dup/reassign as needed, however.

Comment 2

[(not reading, please use [e-mail] instead)]

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

Comment 3

[sairuh (rarely reading bugmail)]

not a problem using 2001.08.15.14-comm bits.

Comment 4

[sairuh (rarely reading bugmail)]

this isn't occurring with all message i reply to --i've found that it happened
with a couple which were written in plain-text [i also have plain-text turned on
for msg composition]. didn't occur with a couple i tested which were written in
html.

Comment 5

[sairuh (rarely reading bugmail)]

Attachment: sample msg that causes crash

Comment 6

[(not reading, please use [e-mail] instead)]

taking, ducarroz is on vacation.

Comment 7

[(not reading, please use [e-mail] instead)]

mScrollbar is null in SetVisibleScrollbar()

Comment 8

[(not reading, please use [e-mail] instead)]

Attachment: bullet proofing patch (to prevent the crash)

Comment 9

[(not reading, please use [e-mail] instead)]

blocker since reply crashes.

hyatt, is that patch ok?

Comment 10

[Håkan Waara]

Is this the real fix? I don't think the scrollbar should be null here.

Comment 11

[(not reading, please use [e-mail] instead)]

the bulletproofing is in, sr=hyatt.

over to hyatt and updating summary to reflect the new problem:

we assert, and where we'd use to crash we just don't put up a autocomplete popup.

the good news for end users is that as they continue to type, one will come up.

Comment 12

[Jay Patel [:jay]]

Adding topcrash keyword and Trunk [@ nsOutlinerBodyFrame::SetVisibleScrollbar]
to summary for tracking.  This is a topcrasher according to the latest Talkback
reports..

Comment 13

[Brian Ryner (not reading)]

Ok, it looks like the outliner's frame mapping is getting nulled out (and the 
frame possibly destroyed!) by nsXULTreeGroupFrame::ContinueReflow...

Comment 14

[Brian Ryner (not reading)]

Attachment: stack trace of frame mapping being nulled out

Comment 15

[Brian Ryner (not reading)]

The trigger for this seems to simply be having more recipients than there are 
visible rows of the tree.  It looks like we construct frames for the out-of-
view rows, then tear them down, but don't tear down the outliner body frames.  

Comment 16

[David Hyatt]

It looks like the placeholder automatically has its out of flow frame removed as
a mapping, but it doesn't automatically do the destruction, so the mapping is
nulled out, but the frame is left alive to then receive a reflow later.

Not sure what the best fix is here.  We could correct this oddity created by the
tree widget only by having the popup set frame call getprimaryframefor on a menu
popup before flowing it to see if it really is alive.  If it gets null, it nukes
the frame instead of flowing it. :)  Sounds hacky.

Alternatively we move my Destroy code for popups that is in the ContentRemoved
method of the frame constructor into this DoDeletingFrameSubtree.  That might be
better.

Comment 17

[Jay Patel [:jay]]

Just for the record, Talkback shows the nsOutlinerBodyFrame::SetVisibleScrollbar
crash last occurring with MozillaTrunk build 2001082111.

Comment 18

[David Hyatt]

This bug and the bug with addresses disappearing have the same root cause.  I
need to fix that problem. Nominating for PDT.

Comment 19

[selmer (gone)]

Seems like crash and topcrash keywords should come off this bug given Jay's
recent comment.  If the real fix is available RSN, then it has a chance of going
into 094.  Otherwise, IMO the bulletproofing makes it possible to punt this to
095.  Time's running out for 094...

Comment 20

[lchiang]

Any progress in coming up with a fix? per PDT

Comment 21

[David Hyatt]

I have a fix for this.  It also patches a bad leak of autocomplete widgets in
the message compose pane.  I want to test it more before posting the patch.

Comment 22

[David Hyatt]

Attachment: Patch to fix leaky popups in tree.

Comment 23

[David Hyatt]

Here is the patch to fix popups.  I believe this fix may also be required for 
96899 (I was running with this patch as well as the ugly hack in 96899 when I 
fixed 96899).

Looking for r/sr.

Waterson, since you're frame constructor man, now, you should review this and 
let me explain how popups work to you these days.

Comment 24

[Chris Waterson]

This looks a lot like what we do for out-of-flow frames. Could the popup frame
just be like a ``regular'' out-of-flow frame, with a placeholder frame in the
popupset?

Comment 25

[David Hyatt]

No.  The problem is that the popupset is not really a parent of the popup.   It
does not enclose the popup, so unlike the absolute/float cases, you can't just
go to a separate list on your parent.  Heck, your parent can be anything (a
block, a box, whatever).

Comment 26

[Chris Waterson]

How about treating it like a fixed frame, then? In that case, there is a
placeholder left in the frame that is the content parent of the fixed frame, but
its geometric parentage is altered so that it's in the nearest area frame's (?)
fixed list. The popup must be owned by the window in which the popupset was
declared, right?

Or are you saying that any frame in the window can be the ``parent'' of the
popup? (If so, how is such parentage meaningful?)

Comment 27

[David Hyatt]

I don't understand.  This sounds like something I'm not familiar with.  

Comment 28

[David Hyatt]

The out-of-flow frame (the popup) is placed into a special entry list under the 
popup set.  There is only one popupset per XUL document that resides just 
underneath the root frame (as a child of the root box).  

The code that I added behaves logically very similarly to the other types of 
out-of-flow frames.  I have to go through some hoops to locate the popup set 
though.

By geometric parentage, are you suggesting that I parent the out-of-flow frame 
(the actual popup) to the popup set with a frame parent/child relationship?

Comment 29

[David Hyatt]

One reason that makes me nervous is that I recall running into issues with code 
that wanted to get additional child lists from frames doing the wrong things 
with popups.  I've never been able to use the traditional additional frame 
child list stuff with popups, since there's code that does things that mess up 
popups.

In principle, it sounds like something to pursue for 0.9.5+, but I'm hesitant 
to try to do it for eMojo (this patch is trying to fix a regression).

Comment 30

[Chris Waterson]

Ok, sorry - I wasn't paying attenting to the milestone. r/sr=waterson on the
patch as is: it looks reasonable. Let's file a bug (for later) to see if we can
collapse this logic with the existing out-of-flow frame stuff.

Comment 31

[Asa Dotzler [:asa]]

0.9.4 is out the door

Comment 32

[Jaime Rodriguez, Jr.]

PDT+ per PDT, please check it into the trunk and branch.

Comment 33

[David Hyatt]

Fix checked in to trunk and branch.

Comment 34

[Ninoschka Baca]

sairuh, can you check if this is fixed on the branch? I am unable to duplicate
the crash using any plain text message or the attached sample message.

Comment 35

[sairuh (rarely reading bugmail)]

using 2001.09.28.04-branch commercial bits --and a debug mozilla trunk build
from 9/25, evening-ish-- on linux, i no longer see this problem. i tested using
the original email from one of my mail folders.

marking verified.

anyone else see this with recent bits on other platforms?