143903 - crash if I close the mail window while a save as dialog is open for a message attachment

Status: VERIFIED FIXED

(MailNews Core :: Attachments, defect)

Description

[henry.hippo]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc2)
Gecko/20020510
BuildID:    2002051006

When a save as dialog is open for a message attachment and the mail window is
closed Mozilla crashes.

Reproducible: Always
Steps to Reproduce:
1. Select a message with an attachment
2 [details] [diff] [review]. Right click the attachment in the preview pane
3. Select save as (Don't select Save or Cancel, just leave it open)
4. Close mail window from the top right corner x.

Actual Results:  Mozilla crashes

Expected Results:  Possibly save the attachment.

I am using the pinball theme and haven't tested this on modern. The closest
relating bug that I could find is #141444.

Comment 1

[G. Todd]

Confirmed on Linux 2002051009.  I can't get a crash I get a freeze and hang (no
TB  crash data)

Comment 2

[Jeff Wasilko]

can't dupe on either OSX or Solaris.

OSX puts up a modal dialog box, so no other action in Mozilla can take place
while the save-as dialog is up.

On Solaris 8 (CDE), picking Close (from teh window manager's adornment) in the
mail window while the save-as dialog box is up does nothing. No crash, and the
mail window stays up.

-jeff

Comment 3

[Antonio.Xu]

i can reproduce it on solaris.

Comment 4

[Antonio.Xu]

i use RC2,solaris8

Comment 5

[Antonio.Xu]

I can reproduce it every time useing RC2,so i will research it.

Comment 6

[Antonio.Xu]

i can reproduce this bug in NS622,it is really a long time bug.

Comment 7

[Henry Jia]

CCing to Calvin & me

Comment 8

[Rene Pronk]

Been able to reproduce it here

Build 2002060904 on linux.

Talkbacks:
TB7203614Y
TB7200835H
TB7200830K

Comment 9

[Antonio.Xu]

I found a way to fix this bug,but i'm not sure my fix is get agreement from 
other people.  Now when i change the code, my debuging build will not crash, 
but when i close mailnews windows, it will leave over the filepicker popup 
windows, but it can work fine.  So if somebody agree with it, i will attach my 
patch for fix this bug.

Comment 10

[Antonio.Xu]

Attachment: patch version 1.00,please rv & sr

I found that mozilla try to get all parent frame for reflow, when open a
filepicker popup windows then close mailnews window. So i add some judge for it
, it can avoid crash and leave over filepicker popup windows. Hello, Mscott. 
Could you rv= ?bug143903, Thank you.

Comment 11

[Scott MacGregor]

Antonio, your going to need someone in layout to look at this patch. I don't
feel comfortable reviewing changes to nsCSSFrameConstructor. The code certainly
looks harmless, your just checking for an error code on an init call and
returning if there's an error. But I don't know the assumptions about potential
return values from ChildIterator::Init.

Comment 12

[Antonio.Xu]

The crash will happen after ChildIterator::Init(). Sometime ChildIterator::Init
() will failing and return a wrong Iterator but we didn't judge it, so when 
program want to use the wrong Iterator , it will crash. this thing will happen 
when some popup windows will reflow (contentinsert) and try to find his 
prevSibling or nestSibling,but those has been destroy. 

Comment 13

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

Comment on attachment 87657 [details] [diff] [review]
patch version 1.00,please rv & sr

This patch seems like a good approach.	(One style suggestion, though:	there's
no point declaring the variable before you use it.  It's better to do

nsresult rv = ChildIterator::Init(...);

than doing

nsresult rv = NS_OK;
...
rv = ChildIterator::Init(...);


However, I'm a little curious why ChildIterator::Init is failing?  Could you
check to see which of the three early returns is happening?  (Were you seeing
the "element not in document" assertion?  That seems the most likely.  It would
suggest that we're operating on content after DocumentViewerImpl::Close has
been called (which calls SetScriptGlobalObject(nsnull), which calls
SetDocument(nsnull, ...), which sets the |mDocument| of all the content nodes
to null.  A stack showing why this is happening (i.e., the full stack of the
crash) could be enlightening.)

Comment 14

[Antonio.Xu]

I found ChildIterator::Init is failing due to aContainer:mdocumnet is null, so 
when ChildIterator::Init() is failing, we should check ChildIterator::Init()'s 
return then judge whether  we should use it.  I think aContainer:mdocumnet is 
null dut to mailwindows has benn closed.

Comment 15

[Antonio.Xu]

I have seen the "element not in document" assertion,it is happen in 
ChildIterator::Init().  It is means mdocument is null when init(). so i think 
we should judge init()'s return value for avoid using the failing Iterator. 

Comment 16

[Antonio.Xu]

it will retrun NS_ERROR_FAILURE due to mdocument is null from 
ChildIterator::Init

Comment 17

[Antonio.Xu]

Attachment: assertion stack

Comment 18

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

So there are still a few possible ways to improve the patch:

 1. As I mentioned in comment 13, you should move the declaration of |nsresult
rv| to the location of its first use.

 2. There are other callers of |ChildIterator::Init| that have the same problem,
and they should probably be fixed as well.

However, it seems to me that there is a deeper problem here, which is that we
shouldn't be doing manipulations like this after we've called
DocumentViewerImpl::Close.  That's the function that's called at the beginning
of paint suppression (when we're still displaying a page but we've really
started to load the next page), and which, among other things, turns off script
execution.  I'm curious as to why we're doing this after the page is already
beginning to go away.

Beyond that, ChildIterator::Init could be fixed so that we can get the document
after |mDocument| is null.  This would involve replacing:

nsCOMPtr<nsIDocument> doc;
aContent->GetDocument(*getter_AddRefs(doc));

with:

nsCOMPtr<nsIDOMNode> domNode = do_QueryInterface(aContent);
nsCOMPtr<nsIDOMDocument> domDoc;
domNode->GetDOMDocument(getter_AddRefs(domDoc));
nsCOMPtr<nsIDocument> doc = do_QueryInterface(domDoc);

(This might require an alternate code path if we ever trigger this code for
nsAttributeContent.)

This might prevent the display from being incorrect in some way due to the
failure.  However, I don't think that we should have to do this since I don't
think we should be reaching this point in the first place.  The stack attached
in comment 17 shows something that I don't think should be happening -- I really
don't think the popup code should be doing everything by setting attributes and
triggering attribute changed notifications.

That said, this fix (revised according to point 1 and probably point 2 above)
should probably be good enough, and we can file a separate bug on the popup code.

Comment 19

[Antonio.Xu]

Attachment: patch version 1.01,please rv & sr

This patch has been improved accoding to David Baron's advice, I had fixed
point 1 and 2 in comment18.  Please rv=? and sr=? 
Thank you

Comment 20

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

Comment on attachment 88103 [details] [diff] [review]
patch version 1.01,please rv & sr 

>@@ -8253,9 +8255,9 @@
> 
>         // Construct an iterator to locate this child at its correct index.
>         ChildIterator iter, last;
>-        for (ChildIterator::Init(insertionContent, &iter, &last);
>-         iter != last;
>-         ++iter) {
>+        if (!ChildIterator::Init(insertionContent, &iter, &last))
>+          continue;
>+        for ( ; iter != last; ++iter) {
>           nsIContent* item = nsCOMPtr<nsIContent>(*iter);
>           if (item == child)
>             // Call ContentInserted with this index.

This one is wrong.  You should be checking NS_FAILED() on the
result, since there are other possible success results other
than NS_OK (which is 0).  I also think you may as well just do
an early return with NS_ENSURE_SUCCESS if you fail, rather than
a |continue;|.

(I'm also beginning to wonder whether perhaps ChildIterator::Init
should handle its own errors by setting the iterators to be equal,
and thus avoid the need for the caller to handle errors.  But this
way is probably OK too.)

Comment 21

[Antonio.Xu]

Attachment: patch version 1.02,please rv & sr

Change some wrong code according to Dbaron's advice.
Thank you ,please rv=? & sr=?

Comment 22

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

Comment on attachment 88241 [details] [diff] [review]
patch version 1.02,please rv & sr 

Out of the seven changes, 1 and 4-7 should be

NS_ENSURE_SUCCESS(rv, rv)

while 2 and 3 should be the way you have them now:

NS_ENSURE_SUCCESS(rv, nsnull)

If you fix that, then r=dbaron

Comment 23

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

Sorry I didn't notice that earlier.

Comment 24

[Antonio.Xu]

Attachment: patch version 1.03,please rv & sr

Change some code according to Dbaron's new advice.
Thank you Dbaron
please sr=?

Comment 25

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

Comment on attachment 88256 [details] [diff] [review]
patch version 1.03,please rv & sr 

r=dbaron

Comment 26

[Chris Waterson]

So, couldn't you solve this with two lines of code by initializing the iterators
in ChildIterator::Init? E.g.,

Index: nsChildIterator.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/style/src/nsChildIterator.cpp,v
retrieving revision 1.1
diff -u -r1.1 nsChildIterator.cpp
--- nsChildIterator.cpp	16 Dec 2001 09:13:48 -0000	1.1
+++ nsChildIterator.cpp	19 Jun 2002 17:49:22 -0000
@@ -45,6 +45,10 @@
                     ChildIterator* aFirst,
                     ChildIterator* aLast)
 {
+  // Initialize out parameters to be equal, in case of failure.
+  aFirst->mContent = aLast->mContent = nsnull;
+  aFirst->mIndex   = aLast->mIndex   = 0;
+
   NS_PRECONDITION(aContent != nsnull, "no content");
   if (! aContent)
     return NS_ERROR_NULL_POINTER;


That would also allow us to change the return signature of ChildIterator::Init
to "void".

Comment 27

[Antonio.Xu]

Attachment: patch version 1.04,please rv & sr

Thanks for Waterson's advice, I have chenged my patch.Please sr=? bug143903

Comment 28

[Chris Waterson]

Well, my point was that if you add the initialization to ChildIterator, you
won't need all of the other checks in nsCSSFrameConstructor! :-)

If you revert your changes to nsCSSFrameConstructor, does the initialization
alone fix the problem? (It should...)

Comment 29

[Antonio.Xu]

Attachment: patch version 1.05,please rv & sr

I think I know your means, but I still must be change some code in
nsCSSFramsConstrutor.cpp, please saw my patch have a line
"iter.seek(aIndexInContainer);" , it will change the iter::mindex, and then
"while" can running. If i didn't change it, mozilla will crash when i try to
reproduce this bug.  Please sr=? my patch

Comment 30

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

Comment on attachment 88414 [details] [diff] [review]
patch version 1.05,please rv & sr 

r=dbaron

Comment 31

[Chris Waterson]

Comment on attachment 88414 [details] [diff] [review]
patch version 1.05,please rv & sr 

sr=waterson

Comment 32

[Antonio.Xu]

patch check in trunk

Comment 33

[chris hofmann]

Comment on attachment 88414 [details] [diff] [review]
patch version 1.05,please rv & sr 

a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking into the
branch.

Comment 34

[Pete Zha]

checked into MOZILLA_1_0_BRANCH

Comment 35

[Trix Supremo]

verified on mozilla branch build 2002071006. changing keyword to verified1.0.1

Comment 36

[Antonio.Xu]

reassign to me