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

RESOLVED FIXED in Thunderbird 13.0

Status

MailNews Core
Backend
RESOLVED FIXED
10 years ago
5 years ago

People

(Reporter: Joey Minta, Assigned: aceman)

Tracking

(Blocks: 1 bug, {crash})

Trunk
Thunderbird 13.0
crash
Bug Flags:
wanted-thunderbird3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

19.47 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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.
Attachment #298534 - Flags: superreview?(dmose)
Attachment #298534 - Flags: review?(bugzilla)
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.
Attachment #298534 - Flags: review?(bugzilla) → review-
(Reporter)

Updated

9 years ago
Attachment #298534 - Flags: superreview?(dmose)
Product: Core → MailNews Core
better error checking wanted TB3?
I picked this and  Bug 413590 for wanted-thunderbird? as it seems these two would have the most impact
Flags: wanted-thunderbird3?

Updated

8 years ago
Flags: wanted-thunderbird3? → wanted-thunderbird3+
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).
Attachment #298534 - Attachment is obsolete: true
Whiteboard: [has draft patch][patchlove]
serge, are you up for taking this and finishing it off?
(In reply to comment #4)

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

Comment 6

6 years ago
Serge, will you take it? Or can I?
Assignee: jminta → acelists
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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.
Attachment #587830 - Flags: review?(mbanner)
(Assignee)

Comment 9

5 years ago
Spun off nsMimeConverter.cpp changes to bug 717407. There is some more work and this patch is already too large.
Summary: null-arg checks in nsMsgThreadedDBView.cpp, nsMsgSendLater.cpp, nsSmtpUrl.cpp, nsImapIncomingServer.cpp, nsPop3IncomingServer.cpp, nsMimeConverter.cpp → null-arg checks in nsMsgThreadedDBView.cpp, nsMsgSendLater.cpp, nsSmtpUrl.cpp, nsImapIncomingServer.cpp, nsPop3IncomingServer.cpp
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 ...
Attachment #587830 - Flags: review?(mbanner) → feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 598972 [details] [diff] [review]
patch v3
Attachment #587830 - Attachment is obsolete: true
Attachment #598972 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Whiteboard: [has draft patch][patchlove]
Attachment #598972 - Flags: review?(mbanner) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Checked in: http://hg.mozilla.org/comm-central/rev/4487f9fbfe2d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 13.0
You need to log in before you can comment on or make changes to this bug.