413548 - null-arg checks in nsMsgThreadedDBView.cpp, nsMsgSendLater.cpp, nsSmtpUrl.cpp, nsImapIncomingServer.cpp, nsPop3IncomingServer.cpp

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Joey Minta]

Attachment: patch v1

These should all be fairly simple.  A couple of crashes on strlen() and one attempted null-check that didn't actually do its job.

Comment 1

[Mark Banner (:standard8)]

Comment on attachment 298534 [details] [diff] [review]
patch v1

+  NS_ENSURE_ARG_POINTER(newHdr);
   nsresult rv = NS_OK;

nit: these need blank lines after them.

-  NS_ENSURE_ARG_POINTER(aSenderIdentity); 
+  NS_ENSURE_ARG_POINTER(*aSenderIdentity); 

What happens if aSenderIdentity is nsnull... ? We need to have both checks here.

Comment 2

[Wayne Mery (:wsmwk)]

better error checking wanted TB3?
I picked this and  Bug 413590 for wanted-thunderbird? as it seems these two would have the most impact

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 298534 [details] [diff] [review]
patch v1

Patch has obsoleted (along with r-):

$ patch -p1 --dry-run < ~/Desktop/p413548.diff 
patching file mailnews/base/src/nsMsgThreadedDBView.cpp
Hunk #1 succeeded at 404 with fuzz 2 (offset -170 lines).
patching file mailnews/compose/src/nsMsgSendLater.cpp
Hunk #1 succeeded at 350 (offset 104 lines).
Hunk #2 FAILED at 1218.
1 out of 2 hunks FAILED -- saving rejects to file mailnews/compose/src/nsMsgSendLater.cpp.rej
patching file mailnews/compose/src/nsSmtpUrl.cpp
Hunk #1 succeeded at 754 with fuzz 1 (offset 143 lines).
patching file mailnews/imap/src/nsImapIncomingServer.cpp
Hunk #1 succeeded at 688 (offset 6 lines).
Hunk #2 succeeded at 2264 (offset -298 lines).
Hunk #3 FAILED at 2435.
1 out of 3 hunks FAILED -- saving rejects to file mailnews/imap/src/nsImapIncomingServer.cpp.rej
patching file mailnews/local/src/nsPop3IncomingServer.cpp
Hunk #1 succeeded at 655 (offset 108 lines).
patching file mailnews/mime/src/nsMimeConverter.cpp
Hunk #1 succeeded at 111 (offset -25 lines).

Comment 4

[Wayne Mery (:wsmwk)]

serge, are you up for taking this and finishing it off?

Comment 5

[Serge Gautherie (:sgautherie)]

(In reply to comment #4)

I should be able to help: see bug 412109 comment 3...

Comment 6

[:aceman]

Serge, will you take it? Or can I?

Comment 7

[:aceman]

(In reply to Mark Banner (:standard8) from comment #1)
> -  NS_ENSURE_ARG_POINTER(aSenderIdentity); 
> +  NS_ENSURE_ARG_POINTER(*aSenderIdentity); 
> 
> What happens if aSenderIdentity is nsnull... ? We need to have both checks
> here.

This breaks tests, so I'll remove it.

Comment 8

[:aceman]

Attachment: patch v2

Rearchitected the patch, added some other enhancements. Tests seem to pass.
The big change in
nsMsgThreadedDBView::OnNewHeader(nsIMsgDBHdr *newHdr, nsMsgKey aParentKey, bool ensureListed)
is just conversion of the function to early returns and thus removing 2 nested else blocks (and indents). It should not change the logic but please check.

Comment 9

[:aceman]

Spun off nsMimeConverter.cpp changes to bug 717407. There is some more work and this patch is already too large.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 587830 [details] [diff] [review]
patch v2

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

This is basically r+, but I'd like you to update the patch before I test it.

::: mailnews/base/src/nsMsgThreadedDBView.cpp
@@ +600,5 @@
>    // This is the mail behaviour, but threaded views want
>    // to insert in order...
> +  PRUint32 msgFlags;
> +  newHdr->GetFlags(&msgFlags);
> +  if ((m_viewFlags & nsMsgViewFlagsType::kUnreadOnly) && !ensureListed && (msgFlags & nsMsgMessageFlags::Read))

nit: lets wrap this a bit:

if ((m_viewFlags & nsMsgViewFlagsType::kUnreadOnly) && !ensureListed &&
    (msgFlags & nsMsgMessageFlags::Read))

@@ +644,3 @@
>  
> +    if (!(flags & nsMsgMessageFlags::Elided)) // thread is expanded
> +    {  // insert child into thread

nit: start comment on next line please, and with a capital as its a sentence.

@@ +696,4 @@
>    }
> +  else // adding msg to thread that's not in view.
> +  {
> +    if (threadHdr)

This can just be combined with the else to make one else if ...

Comment 11

[:aceman]

Attachment: patch v3

Comment 12

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/4487f9fbfe2d