Last Comment Bug 413548 - null-arg checks in nsMsgThreadedDBView.cpp, nsMsgSendLater.cpp, nsSmtpUrl.cpp, nsImapIncomingServer.cpp, nsPop3IncomingServer.cpp
: null-arg checks in nsMsgThreadedDBView.cpp, nsMsgSendLater.cpp, nsSmtpUrl.cpp...
Status: RESOLVED FIXED
: crash
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 13.0
Assigned To: :aceman
:
:
Mentors:
Depends on:
Blocks: 412109
  Show dependency treegraph
 
Reported: 2008-01-22 13:35 PST by Joey Minta
Modified: 2012-02-23 01:08 PST (History)
7 users (show)
dmose: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (4.23 KB, patch)
2008-01-22 13:35 PST, Joey Minta
standard8: review-
Details | Diff | Splinter Review
patch v2 (19.48 KB, patch)
2012-01-11 14:25 PST, :aceman
standard8: feedback+
Details | Diff | Splinter Review
patch v3 (19.47 KB, patch)
2012-02-20 14:43 PST, :aceman
standard8: review+
Details | Diff | Splinter Review

Description Joey Minta 2008-01-22 13:35:23 PST
Created attachment 298534 [details] [diff] [review]
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, afk until Dec) 2008-01-24 05:10:44 PST
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, NI for questions) 2009-05-17 04:32:20 PDT
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] 2010-08-06 08:51:40 PDT
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, NI for questions) 2010-11-08 07:01:43 PST
serge, are you up for taking this and finishing it off?
Comment 5 Serge Gautherie (:sgautherie) 2010-11-10 20:35:05 PST
(In reply to comment #4)

I should be able to help: see bug 412109 comment 3...
Comment 6 :aceman 2011-11-15 00:32:12 PST
Serge, will you take it? Or can I?
Comment 7 :aceman 2012-01-11 13:19:19 PST
(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 2012-01-11 14:25:04 PST
Created attachment 587830 [details] [diff] [review]
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 2012-01-11 14:26:55 PST
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, afk until Dec) 2012-02-20 12:49:19 PST
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 2012-02-20 14:43:19 PST
Created attachment 598972 [details] [diff] [review]
patch v3
Comment 12 Mark Banner (:standard8, afk until Dec) 2012-02-21 04:36:26 PST
Checked in: http://hg.mozilla.org/comm-central/rev/4487f9fbfe2d

Note You need to log in before you can comment on or make changes to this bug.