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

RESOLVED FIXED in Thunderbird 54.0

Status

MailNews Core
Backend
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: alta88, Assigned: alta88)

Tracking

unspecified
Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.65 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8838963 [details] [diff] [review]
sendlater.patch
Assignee: nobody → alta88
Attachment #8838963 - Flags: review?(acelists)

Comment 2

a year ago
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).
Attachment #8838963 - Flags: feedback+
(Assignee)

Comment 3

a year ago
(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

a year ago
(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.
(Assignee)

Comment 5

a year ago
Created attachment 8839268 [details] [diff] [review]
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.
Attachment #8838963 - Attachment is obsolete: true
Attachment #8838963 - Flags: review?(acelists)
Attachment #8839268 - Flags: review?(acelists)

Comment 6

a year ago
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 .
Attachment #8839268 - Flags: review?(acelists) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 7

a year ago
https://hg.mozilla.org/comm-central/rev/d25229d4bffc347ee9ca14f7ac384611a176b81c
Status: NEW → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
You need to log in before you can comment on or make changes to this bug.