286289 - Compose Window getting closed twice leading to nasty JS errors

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Scott MacGregor]

When sending a message in recent trunk builds I get the following errors in the
console:

CopyListener: SUCCESSFUL ON THE COPY OPERATION!
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "gMsgCompose has no properties" {file: "chrom
e://messenger/content/messengercompose/MsgComposeCommands.js" line: 156}]' when
calling method: [nsIMsgComposeStateListener::ComposeProcessDone]"  nsresult: "0x
80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  d
ata: yes]
************************************************************
nsMsgComposeSendListener: Success on the message copy operation!

ComposeUnload from XUL
WARNING: nsComposerCommandsUpdater::SelectionIsCollapsed - no domSelection, file
 c:/build/trees/tbirddbg/mozilla/editor/composer/src/nsComposerCommandsUpdater.c
pp, line 386
###!!! ASSERTION:
XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.

Comment 1

[Scott MacGregor]

It looks like the compose window is getting told to close itself twice. 

1) Once when the IMAP url finishes running, ImapFolder::OnCopyCompleted ends up
calling the copy service which ends up calling CopyListener::OnStopCopy.

2) The second time happens when the imap mail folder calls
::CopyNextStreamListener. The copy listener still has a valid pointer to the
compose send object so we tell it to close again, here's the stack for this case:

nsMsgCompose::NotifyStateListeners(nsMsgCompose * const 0x045ba7b8,
TStateListenerNotification eComposeProcessDone, unsigned int 0) line 3619
nsMsgComposeSendListener::OnStopCopy(nsMsgComposeSendListener * const
0x053e4050, unsigned int 0) line 2972
nsMsgComposeAndSend::NotifyListenerOnStopCopy(nsMsgComposeAndSend * const
0x034b4b50, unsigned int 0) line 3989
CopyListener::OnStopCopy(CopyListener * const 0x05480180, unsigned int 0) line 155
nsImapMailFolder::CopyNextStreamMessage(nsImapMailFolder * const 0x0313c6f0, int
1, nsISupports * 0x054802c8) line 6045

Comment 2

[Scott MacGregor]

Attachment: a fix

Read the last comment for more details.

This patch prevents the duplicate call to close the compose window by nulling
out the composesendlistener after we've told it we are done copying the first
time. So the 2nd call doesn't do anything.

It still is not clear to me:
1) why this is just now manifesting itself
2) I wonder if we should be looking at the 2nd call coming from
nsImapMailFolder::CopyNextStreamMessage as the place where we fix this instead.
It seems wrong that we would call Done on the same copy listener object.

Comment 3

[Scott MacGregor]

ah David, it looks like Bug #235013 caused this duplicate call. I'll let you
decide if you want to fix it with the quick mail compose fix or to look at Bug
#235013 in more detail to avoid the duplicate notifications to the same copy
object. I suspect we should do the later. 

Comment 4

[David :Bienvenu]

yes, I'm going to do the latter tomorrow...it could be your fix is fine, and
needed for local folder fcc.

Comment 5

[David :Bienvenu]

OK, what I should have done in bug 235013 was use the copy service to copy
messages, and *not* add the extra OnStopCopy call in nsImapMailFolder. I'll
attach  a patch later today.

Comment 6

[David :Bienvenu]

Attachment: proposed fix

this should fix it - folder drag drop is broken in general on the trunk, so I
need to fix that before verifying that local to imap folder copy still works
with this patch.

Comment 7

[David :Bienvenu]

OK, copying local folders to imap is working, so I think this patch is OK. I'll
attach another patch that allows folder copy across servers (instead of just move)

Comment 8

[David :Bienvenu]

Attachment: allow cross-server folder copy

this is why I was confused earlier - move was working, but I was trying to
force a copy...

Comment 9

[David :Bienvenu]

Attachment: fix for some more issues

this patch fixes a couple more issues - folder copy wasn't sending a done
notification in some cases, because we had two requests with the same source,
one a folder copy and one a message copy. In that case, we need to clear both
requests. And I also fixed a ref-counting problem - my bad for using delete
this instead of Release(). I did verify that the destructor for
nsImapFolderCopyState does get called with these changes...

Comment 10

[Scott MacGregor]

Comment on attachment 177987 [details] [diff] [review]
allow cross-server folder copy

this is the wrong patch I think :)

Comment 11

[David :Bienvenu]

Comment on attachment 177987 [details] [diff] [review]
allow cross-server folder copy

no, this is an additional patch, to allow cross-server folder copy, as opposed
to cross-server folder move

Comment 12

[David :Bienvenu]

fix checked in.