1496598 - Port bug 1493226 - Fix static analysis errors

Status: RESOLVED FIXED

(MailNews Core :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Patch coming.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1496598-compile-time-determinable.patch - WIP

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?

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: no-ws.patch

Same patch with |hg qdiff -w|, so you don't see the indentation changes.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: no-ws.patch

That's the no white-space changes patch.

Comment 4

[Andrew McCreight [:mccr8]]

(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.

Comment 5

[Ben Campbell]

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)

Comment 6

[Ben Campbell]

Attachment: js_delegate_fiddling.patch

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.

Comment 7

[Jorg K (CEST = GMT+2)]

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.

Comment 8

[Ben Campbell]

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!

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1496598-compile-time-determinable.patch (v2)

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.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: no-ws.patch

Tries:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=52b1032426e96155d40603993dc75a674d493cfb
That failed due to Apple-only code, so one more:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1ffc4516b7f49fbc0793395880bc8dba76ce75d3

Comment 11

[Jorg K (CEST = GMT+2)]

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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[:aceman]

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.

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[:aceman]

(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 16

[Ben Campbell]

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)

Comment 17

[Jorg K (CEST = GMT+2)]

Umm, I think the
  rv = mailSession->GetTopmostMsgWindow(getter_AddRefs(msgWindow));
a few lines down makes it necessary, no?