Closed Bug 1496598 Opened 11 months ago Closed 11 months ago

Port bug 1493226 - Fix static analysis errors

Categories

(MailNews Core :: General, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 64.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(3 files, 3 obsolete files)

Patch coming.
Ben, can you please take a look at this. It doesn't compile yet, the trickery in JS Account fell over big time ... and at 2 AM I need to leave it.

Also, can you check the patch.

There were a few cases where the QI had an rv, and I did this:

-  nsCOMPtr <nsIFile> file = do_QueryInterface(aLocalFile, &rv);
-  NS_ENSURE_SUCCESS(rv,rv);
+  nsCOMPtr <nsIFile> file = aLocalFile;

Or should this be
+  nsCOMPtr <nsIFile> file = aLocalFile;
+  NS_ENSURE_TRUE(file, NS_ERROR_NULL_POINTER);

In other words, what did the QI do with nulls?
Assignee: nobody → jorgk
Attachment #9014625 - Flags: review?(benc)
Attachment #9014625 - Flags: feedback?(continuation)
Attached patch no-ws.patch (obsolete) — Splinter Review
Same patch with |hg qdiff -w|, so you don't see the indentation changes.
Attached patch no-ws.patch (obsolete) — Splinter Review
That's the no white-space changes patch.
Attachment #9014627 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #1)
> In other words, what did the QI do with nulls?

Interesting point. I didn't think about that when I was fixing similar calls in bug 1496598. It looks like QIing a null via do_QueryInterface actually does return an error, so NS_ENSURE_TRUE(file, NS_ERROR_NULL_POINTER); is the safer way to go, especially when the value is from an argument like this. In the case of nsAbLDAPReplicationData.cpp, the value being QId is a return value from something else, and the other method has its return error checked, so that's probably okay.
Attachment #9014625 - Flags: feedback?(continuation) → feedback+
Just to confirm what Andrew said:
My reading is that QI(xxx,&rv) will return NS_ERROR_NULL_POINTER in rv if xxx is null.
(https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsCOMPtr.cpp#29)
This patch gets it compiling again for me.
I ran out of time and so:
- it hasn't seen any proper testing yet
- I haven't gone over your patch to check out the QI return codes yet.

But hopefully this sorts out the worst of the bustage.

Phew. It was a good one to dig through - templates, multiple inheritance, nested macros, all that great C++ stuff!

Will check in a bit later to help mop up.
Attachment #9014680 - Flags: feedback?(jorgk)
Comment on attachment 9014680 [details] [diff] [review]
js_delegate_fiddling.patch

Thanks, this compiles for me. You don't really need the _jsmethods parameter to the macro, since mMethods is always used, but like this you can get rid of the hard-coding.
Attachment #9014680 - Flags: feedback?(jorgk) → feedback+
Yes, I figured that on balance it was nicer to be explicit about it. Less magic. Not that it makes tooo much difference - DELEGATE_JS is such an ultra-specific macro anyway :-)

There's definitely scope to ditch the remaining do_QueryInterface calls there. We know the type of the target object (mCppBase) at compile time. But it's diminishing returns. The existing code is solid, and I can't imagine it's much of a performance bottleneck. If someone decides it is, I'll be happy to jump back in and turn them all into static casts!
I think the review will be even less fun than writing this. Compared to the WIP I went through with the fine-comb and added error checking where the QI was really a glorified null check.

Then it occurred to me that a whole heap of variables were unneeded, so I removed all those. There was a whole heap of other junk that went out the door with it.

I'll put r? onto the no-ws patch since that will be easier to read.
Attachment #9014625 - Attachment is obsolete: true
Attachment #9014625 - Flags: review?(benc)
Comment on attachment 9014725 [details] [diff] [review]
no-ws.patch

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

Looks OK to me. Added some comments for stuff that's no immediately evident.

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +693,2 @@
>          RemoveConnection(aConnection);
> +    aConnection->TellThreadToDie(false);

aConnection was checked on entry to the function. You don't see it here.

::: mailnews/mapi/mapihook/src/msgMapiImp.cpp
@@ -704,2 @@
>  
> -  if (inputStream)

This test doesn't make sense. 'fileStream' can't be null, otherwise we would have crashed above.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f77c18acceaf
Port bug 1493226: fix 'don't use do_QueryInterface for compile-time-determinable casts' errors. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/93daa9a87d52
Port bug 1493226: remove needless QueryInterface calls in JS delegation. r=jorgk
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
Attachment #9014680 - Flags: feedback+ → review+
Comment on attachment 9014725 [details] [diff] [review]
no-ws.patch

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

This m-c change at least simplifies the code and if it makes tighter checks and faster code then even better.
Thanks for tackling this.

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +235,5 @@
>  nsMsgFilterService::ThrowAlertMsg(const char*aMsgName, nsIMsgWindow *aMsgWindow)
>  {
>    nsString alertString;
>    nsresult rv = GetStringFromBundle(aMsgName, alertString);
> +  nsCOMPtr<nsIMsgWindow> msgWindow = aMsgWindow;

What does this do now? Both variables are of the same type. Is msgWindow needed?

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ +633,5 @@
>  
>      if (NS_SUCCEEDED(aImapUrl->GetMockChannel(getter_AddRefs(mockChannel))) && mockChannel)
>      {
>        nsresult requestStatus;
> +      mockChannel->GetStatus(&requestStatus);

Isn't this losing the null check?

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +436,5 @@
>    {
>      nsCOMPtr<nsISupports> supports;
>      rv = children->GetNext(getter_AddRefs(supports));
> +    currentFolderPath = do_QueryInterface(supports);
> +    if (NS_FAILED(rv) || !currentFolderPath)

This looks ugly going through nsISupports. It seems we should use GetNextFile() on nsIDirectoryEnumerator instead of generic GetNext(), m-c has such patterns.

But I can do this in a new bug as we have many occurences of this old pattern where it could be simplified. We didn't do it when nsIDirectoryEnumerator was made to be returned from GetDirectoryEntries.
Attachment #9014725 - Flags: feedback?(acelists) → feedback+
This bug is a really pain since you need to see the full context of the file :-(

(In reply to :aceman from comment #13)
> > +  nsCOMPtr<nsIMsgWindow> msgWindow = aMsgWindow;
> What does this do now? Both variables are of the same type. Is msgWindow
> needed?
Needed.

  nsCOMPtr<nsIMsgWindow> msgWindow = aMsgWindow;
  if (!msgWindow) {
    nsCOMPtr<nsIMsgMailSession> mailSession ( do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv));
    if (NS_SUCCEEDED(rv))
      rv = mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow));
  }
You can even see it in the review tool.

> > +      mockChannel->GetStatus(&requestStatus);
> Isn't this losing the null check?
This never had an rv and was never checking it.
Right above you have:
if (NS_SUCCEEDED(aImapUrl->GetMockChannel(getter_AddRefs(mockChannel))) && mockChannel)
You can see that in the review tool.

> This looks ugly going through nsISupports. ...
> But I can do this in a new bug ...
Indeed, remember: bustage fix, I already removed the biggest nonsense.
(In reply to Jorg K (GMT+2) from comment #14)
> This bug is a really pain since you need to see the full context of the file
> :-(

True.

> (In reply to :aceman from comment #13)
>   nsCOMPtr<nsIMsgWindow> msgWindow = aMsgWindow;
>   if (!msgWindow) {
>     nsCOMPtr<nsIMsgMailSession> mailSession (
> do_GetService(NS_MSGMAILSESSION_CONTRACTID, &rv));
>     if (NS_SUCCEEDED(rv))
>       rv = mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow));
>   }
> You can even see it in the review tool.

Yes, sorry, it didn't occur to me to look below the line.
 
> > > +      mockChannel->GetStatus(&requestStatus);
> > Isn't this losing the null check?
> This never had an rv and was never checking it.
> Right above you have:
> if (NS_SUCCEEDED(aImapUrl->GetMockChannel(getter_AddRefs(mockChannel))) &&
> mockChannel)
> You can see that in the review tool.

Not rv, but it was checking the 'request'. But I see now it checks mockChannel for null in the if().

> > This looks ugly going through nsISupports. ...
> > But I can do this in a new bug ...
> Indeed, remember: bustage fix, I already removed the biggest nonsense.

Sure, I just better write it when the idea comes to my mind before I forget it in the N hours I come to dev PC :)
Comment on attachment 9014725 [details] [diff] [review]
no-ws.patch

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

Just to confirm: looks good to me too.
(and nice seeing so many runtime type-checks removed!)

::: mailnews/base/search/src/nsMsgFilterService.cpp
@@ +235,5 @@
>  nsMsgFilterService::ThrowAlertMsg(const char*aMsgName, nsIMsgWindow *aMsgWindow)
>  {
>    nsString alertString;
>    nsresult rv = GetStringFromBundle(aMsgName, alertString);
> +  nsCOMPtr<nsIMsgWindow> msgWindow = aMsgWindow;

I've got a newbie question on this one:
It seems that the rest of the function could happily use the raw `aMsgWindow` var, saving the creation of `msgWindow`.
Was `msgWindow` kept to keep the patch simple and non-intrusive, or is it considered better practice to wrap raw pointers with `nsCOMPtr<>` within functions like this?
(can't see it making any difference either way to the final generated code, so I guess it comes down to stylistic/footgun-protection issues)
Attachment #9014725 - Flags: review?(benc) → review+
Umm, I think the
  rv = mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow));
a few lines down makes it necessary, no?
You need to log in before you can comment on or make changes to this bug.