1340902 - Do not throw in nsIMsgSendLater if an Outbox (nsMsgQueueForLater flagged) folder does not exist

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[alta88]

From Bug 524863:

(In reply to alta88 from comment #48)
> In a brand new profile, if you create a chat account and go offline it
> throws. I was also considering removing Local Folders from feed account
> creation. The issue happens if there isn't an Outbox, iirc.

(In reply to :aceman from comment #51)
> Can I persuade you to make hasUnsentMessages return false at
> https://dxr.mozilla.org/comm-central/rev/
> 6ee27a7af6cd3e7bd25f392048fd40160910275a/mailnews/compose/src/nsMsgSendLater.
> cpp#703 if there is no unsent folder, similarly to when there are no
> accounts? Then we do not need the catches.

(In reply to alta88 from comment #52)
> Yeah, that is the better way.. But I don't think it's just that, it's merely
> creating an instance of nsIMsgSendLater that's a problem (iirc, the try
> catch was done quite a while ago).  I'll take a look and file a new bug, it
> shouldn't hold up the patch here. It was related only in that there are
> little things that assume identities and outboxes and email things that
> shouldn't bother non email account types.

Comment 1

[alta88]

Attachment: sendlater.patch

Comment 2

[:aceman]

Comment on attachment 8838963 [details] [diff] [review]
sendlater.patch

Review of attachment 8838963 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

can you also add the check into nsMsgSendLater::InternalSendMessages ?

Would there be a way to somehow merge those 3 blocks doing the same stuff? Even the comments are the same there.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +118,5 @@
>    // XXX This code should be set up for multiple unsent folders, however we
>    // don't support that at the moment, so for now just assume one folder.
>    rv = GetUnsentMessagesFolder(nullptr, getter_AddRefs(mMessageFolder));
> +  // There doesn't have to be a nsMsgQueueForLater flagged folder.
> +  if (!mMessageFolder)

I think check (NS_FAILED(rv) || !mMessageFolder) for safety.

@@ +701,5 @@
>    {
>      rv = GetUnsentMessagesFolder(nullptr,
>                                   getter_AddRefs(mMessageFolder));
> +    // There doesn't have to be a nsMsgQueueForLater flagged folder.
> +    if (!mMessageFolder)

I think check (NS_FAILED(rv) || !mMessageFolder).

Comment 3

[alta88]

(In reply to :aceman from comment #2)
> Comment on attachment 8838963 [details] [diff] [review]
> sendlater.patch
> 
> Review of attachment 8838963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks.
> 
> can you also add the check into nsMsgSendLater::InternalSendMessages ?
> 

i specifically didn't do it there since by then there must be a folder, so something is wrong and it should throw.  no?

> Would there be a way to somehow merge those 3 blocks doing the same stuff?
> Even the comments are the same there.
> 
> ::: mailnews/compose/src/nsMsgSendLater.cpp
> @@ +118,5 @@
> >    // XXX This code should be set up for multiple unsent folders, however we
> >    // don't support that at the moment, so for now just assume one folder.
> >    rv = GetUnsentMessagesFolder(nullptr, getter_AddRefs(mMessageFolder));
> > +  // There doesn't have to be a nsMsgQueueForLater flagged folder.
> > +  if (!mMessageFolder)
> 
> I think check (NS_FAILED(rv) || !mMessageFolder) for safety.
> 
> @@ +701,5 @@
> >    {
> >      rv = GetUnsentMessagesFolder(nullptr,
> >                                   getter_AddRefs(mMessageFolder));
> > +    // There doesn't have to be a nsMsgQueueForLater flagged folder.
> > +    if (!mMessageFolder)
> 
> I think check (NS_FAILED(rv) || !mMessageFolder).

ok, but how can it really matter, even theoretically?

Comment 4

[:aceman]

(In reply to alta88 from comment #3)
> > can you also add the check into nsMsgSendLater::InternalSendMessages ?
> i specifically didn't do it there since by then there must be a folder, so
> something is wrong and it should throw.  no?

OK, if you know that InternalSendMessages is only ever called after we determined there are messages to send out.

> > I think check (NS_FAILED(rv) || !mMessageFolder).
> 
> ok, but how can it really matter, even theoretically?

Theoretically for the crazy case if GetUnsentMessagesFolder returned mMessageFolder as non-null (but invalid) and set rv to Error. But it seems that the LocateMessageFolder (called from GetUnsentMessagesFolder) resets the return argument to nullptr as the first action. But not all functions are as well behaved.

Comment 5

[alta88]

Attachment: sendlater.patch

(In reply to :aceman from comment #4)
> (In reply to alta88 from comment #3)
> > > can you also add the check into nsMsgSendLater::InternalSendMessages ?
> > i specifically didn't do it there since by then there must be a folder, so
> > something is wrong and it should throw.  no?
> 
> OK, if you know that InternalSendMessages is only ever called after we
> determined there are messages to send out.

that's what i concluded after tracing it out a bit.  it's dubious whether the second GetUnsentMessagesFolder try is necessary; if the init didn't find one, it's unlikely that the folder appeared between then and trying to send.  but that would take totally wasted time and effort to prove for certain, all risk and no gain..

> 
> > > I think check (NS_FAILED(rv) || !mMessageFolder).
> > 
> > ok, but how can it really matter, even theoretically?
> 
> Theoretically for the crazy case if GetUnsentMessagesFolder returned
> mMessageFolder as non-null (but invalid) and set rv to Error. But it seems
> that the LocateMessageFolder (called from GetUnsentMessagesFolder) resets
> the return argument to nullptr as the first action. But not all functions
> are as well behaved.

so.. not actually theoretically in this case.  i did look at LocateMessageFolder. i make the point since it's not good to mechanically copy stuff unnecessarily just because it's a pattern and valid for some cases.  but ok anyway.

Comment 6

[:aceman]

Comment on attachment 8839268 [details] [diff] [review]
sendlater.patch

Review of attachment 8839268 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.
Try run https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b5885909479faa0224891ac9fe08151c91048697 .

Comment 7

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/d25229d4bffc347ee9ca14f7ac384611a176b81c