831993 - convert nsISupportsArray m_serversToGetNewMailFor variable from nsPop3IncomingServer.cpp to something better

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[:aceman]

Defined as a variable in:

    mailnews/local/src/nsPop3IncomingServer.cpp (View Hg log or Hg annotations)
        line 46 -- nsCOMPtr <nsISupportsArray> m_serversToGetNewMailFor;
        line 676 -- m_serversToGetNewMailFor = servers;
        line 711 -- m_serversToGetNewMailFor->Count(&numServersLeft);
        line 716 -- m_serversToGetNewMailFor->RemoveElementAt(0); 

Referenced in:

    mailnews/local/src/nsPop3IncomingServer.cpp (View Hg log or Hg annotations)
        line 715 -- nsCOMPtr <nsIPop3IncomingServer> popServer (do_QueryElementAt(m_serversToGetNewMailFor, 0));

Comment 1

[:aceman]

Attachment: WIP patch

Comment 2

[:aceman]

Attachment: WIP patch 2

This alternative version makes m_serversToGetNewMailFor nsIArray as it seems we do not really need to alter it. However this crashes in tests, so there is a bug somewhere.

Comment 3

[[e-mail]]

Attachment: Alternative approach

Note: this compiles but is otherwise untested.

Ideally I would use nsCOMArray<nsIPop3IncomingServer> throughout but that would require either a [noscript] version of downloadMailFromServers or the nsCOMArray improvements awaiting review in bug 493711.

Comment 4

[[e-mail]]

Comment on attachment 703637 [details] [diff] [review]
WIP patch 2

>   uint32_t numServersLeft;
>-  m_serversToGetNewMailFor->Count(&numServersLeft);
>+  m_serversToGetNewMailFor->GetLength(&numServersLeft);
> 
>-  for (; numServersLeft > 0;)
>+  while (numServersLeft > 0)
>   {
>-    nsCOMPtr <nsIPop3IncomingServer> popServer (do_QueryElementAt(m_serversToGetNewMailFor, 0));
>-    m_serversToGetNewMailFor->RemoveElementAt(0);
>+    nsCOMPtr<nsIPop3IncomingServer> popServer =
>+      do_QueryElementAt(m_serversToGetNewMailFor, numServersLeft - 1);
>     numServersLeft--;
The old code ends up enumerating the servers in ascending order because it removes the first one each time. The new code enumerates the servers in descending order. This could be the cause of your problem. Or I might just be clutching at straws...

Comment 5

[:aceman]

Comment on attachment 703874 [details] [diff] [review]
Alternative approach

I can't say I understand what this new patch does. Passing in a nsTArray is interesting :)

Comment 6

[:aceman]

(In reply to [e-mail] from comment #4)
> The old code ends up enumerating the servers in ascending order because it
> removes the first one each time. The new code enumerates the servers in
> descending order. This could be the cause of your problem. Or I might just
> be clutching at straws...
Sure, I can change that, probably at the cost of a new variable that runs up to the numServersLeft.
Were you able to find the problem why this version crashes?

Comment 7

[[e-mail]]

No, and you're going to need a stack trace if you want me to look harder.

Comment 8

[:aceman]

Attachment: WIP patch 2 v2

For some reason the tests do not crash today. So let's see which version does rkent like.

Comment 9

[Kent James (:rkent)]

Comment on attachment 704019 [details] [diff] [review]
WIP patch 2 v2

Overall this looks good. I prefer this approach.

Just as an aside, ironically today I was checking some old code of mine where I did exactly what I told you NOT to do yesterday, that is to pass in an nsIArray then QI to nsIMutableArray. Not sure why did did that :(

Comment 10

[Kent James (:rkent)]

Comment on attachment 703874 [details] [diff] [review]
Alternative approach

I don't really understand the value of using raw XPCOM arrays, with the need to pass count and do careful NS_IF_RELEASE() when nsIMutableArray is a perfectly valid replacement.

Comment 11

[Kent James (:rkent)]

Comment on attachment 703598 [details] [diff] [review]
WIP patch

Isn't this obsolete, or is there some reason I need to look at it as well? If the question is whether nsIArray or nsIMutableArray is used, I would only use nsIMutableArray into a method that you know is going to change the array.

(diff service is temporarily offline so I cannot easily see the changes).

Comment 12

[[e-mail]]

(In reply to Kent James from comment #10)
> I don't really understand the value of using raw XPCOM arrays, with the need
> to pass count and do careful NS_IF_RELEASE() when nsIMutableArray is a
> perfectly valid replacement.
As I said, this really wants the nsCOMArray improvements in bug 493711, then there wouldn't be any need for manual refcounting. (The value comes from the array being typesafe; nsIMutableArray could contain anything.)

Comment 13

[Kent James (:rkent)]

Typesafe is an advantage. I really appreciate how my C++ code generates compile errors when someone changes an interface, but the javascript code only fails when you try to run it.

Comment 14

[:aceman]

(In reply to Kent James (:rkent) from comment #11)
> Comment on attachment 703598 [details] [diff] [review]
> WIP patch
> 
> Isn't this obsolete, or is there some reason I need to look at it as well?
> If the question is whether nsIArray or nsIMutableArray is used, I would only
> use nsIMutableArray into a method that you know is going to change the array.
> 
> (diff service is temporarily offline so I cannot easily see the changes).

The differene between "WIP patch" and "WIP patch 2 (v2)" is that in WIP patch I used mutable arrays and had to copy one array as one interface had an nsIArray in argument. The "WIP patch 2" solves it all with nsIArray as it seems we do not really need to alter the array internally (of course I can't say if there isn't any leak now). So I prefer the version 2, as it probably allows the caller to pass in mutable or immutable array as he sees fit.

Comment 15

[[e-mail]]

Attachment: Alternative approach

Note: this version needs the patch from bug 493711 that I'll attach shortly, but I just want to show you how much cleaner the code is when you have a useful nsCOMArray API :-)

Comment 16

[Kent James (:rkent)]

Comment on attachment 704319 [details] [diff] [review]
Alternative approach

So I have mixed feelings about this, Neil. The issue comes down to what you fear most, and what I fear most is refcount errors and crashes from nulls.

So I see stuff like:

sresult nsPop3GetMailChainer::GetNewMailForServers(nsIPop3IncomingServer** servers ...

then

m_serversToGetNewMailFor.AppendElements(servers, count);

and I start asking what happened to the memory that was allocated for servers. Or was it allocated? I don't know, so I would have to look in detail at the memory model issues associated with .Elements() and .AppendElements.

You'll probably tell me that one of the simplifications of this is that the memory allocation is handled automatically, but I don't know that so I would have to look at it in more detail.

I still like the type safety though.

I also see that you are not protecting for null values in calls like rv = deferredServers[0]-> so once again I would have to check in details the code for nsCOMArray to see if it does the protection.

So this forces me to learn a new set of issues associated with an object that is fairly rarely used. In my own code, I would not bother (at least for usage through XPCOM. As an internal variable in a C++ method I would use nsCOMArray.)

This of course is just my opinion, and reflects my own skill level (or lack thereof).

Comment 17

[[e-mail]]

(In reply to Kent James from comment #16)
> So I see stuff like:
> 
> sresult nsPop3GetMailChainer::GetNewMailForServers(nsIPop3IncomingServer**
> servers ...
> 
> then
> 
> m_serversToGetNewMailFor.AppendElements(servers, count);
> 
> and I start asking what happened to the memory that was allocated for
> servers. Or was it allocated? I don't know, so I would have to look in
> detail at the memory model issues associated with .Elements() and
> .AppendElements.
Under the XPCOM interface contract, servers is a read-only array of pointers (to mutable objects, although that's not relevant here). AppendElements copies the array into its local mutable nsCOMArray (and also AddRefs them, so that the caller is free to Release its copies).

> I also see that you are not protecting for null values in calls like rv =
> deferredServers[0]-> so once again I would have to check in details the code
> for nsCOMArray to see if it does the protection.
No, it doesn't, but my version of GetDeferredServers only returns non-null POP3 servers, since it happens to save a call to QueryInterface. (If the old code had been using my test for a deferred server then it wouldn't have had to null-check the server either.)

Comment 18

[:aceman]

So what now?

Comment 19

[[e-mail]]

Comment on attachment 704319 [details] [diff] [review]
Alternative approach

OK, so this uses the new improved nsCOMArray from bug 493711.

>-        nsCOMPtr <nsIMsgIncomingServer> server (do_QueryElementAt(allServers, i));
>+        nsCOMPtr<nsIPop3IncomingServer> server(do_QueryElementAt(allServers, i));
>         if (server)
This now ensures that only actual nsIPop3IncomingServer objects get added to the array. The change in nsNoIncomingServer relies on this.

>-  void downloadMailFromServers(in nsISupportsArray aServers, 
>-                              in nsIMsgWindow aMsgWindow,
>-                              in nsIMsgFolder aFolder, 
>-                              in nsIUrlListener aListener);
>+  void downloadMailFromServers(
>+    [array, size_is(count)]in nsIPop3IncomingServer aServers,
>+    in unsigned long count, in nsIMsgWindow aMsgWindow,
>+    in nsIMsgFolder aFolder, in nsIUrlListener aListener);
This means that the function is still scriptable. (But I haven't updated the callers yet.)

>-  m_serversToGetNewMailFor = servers;
>+  m_serversToGetNewMailFor.AppendElements(servers, count);
The old code used an nsISupportsArray so you just had to add a new reference to that array to make it keep its contents. The new code uses an array of XPCOM pointers so that the elements of the array have to be appended to a local array. Fortunately the new nsCOMArray makes that easy.

>+    nsCOMPtr<nsIPop3IncomingServer> popServer(m_serversToGetNewMailFor[0]);
>+    m_serversToGetNewMailFor.RemoveObjectAt(0);
>     numServersLeft--;
>     if (popServer)
I left this null-check in as the array could have come from script and therefore potentially contain null elements.

Comment 20

[Kent James (:rkent)]

Comment on attachment 704319 [details] [diff] [review]
Alternative approach

I like this approach with the new nsCOMArray. Memory management seems to be much cleaner, which is what you would hope for with a custom array type.

Comment 21

[:aceman]

rkent, would you mind reviewing this when you already saw it, or do we need to wait for standard8?

Or did Neil choose standard8 intentionally?

Comment 22

[Kent James (:rkent)]

I would be willing to review this if requested by Neil or Standard8 to do so.

Comment 23

[Mark Banner (:standard8)]

Comment on attachment 704319 [details] [diff] [review]
Alternative approach

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

::: mailnews/local/public/nsIPop3IncomingServer.idl
@@ -29,5 @@
>    attribute ACString deferredToAccount;
>    // whether get new mail in deferredToAccount gets
>    // new mail with this server.
>    attribute boolean deferGetNewMail;
> -  void downloadMailFromServers(in nsISupportsArray aServers, 

This file needs a uuid change.

Comment 24

[[e-mail]]

Pushed comm-central changeset 323851e9e786.

Comment 25

[:aceman]

This seem to have caused failures in mail/test/mozmill/instrumentation/test-instrument-setup.js like this:
[Exception... "Not enough arguments [nsIPop3IncomingServer.downloadMailFromServers]"  nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  location: "JS frame :: chrome://messenger/conte
nt/mailWindowOverlay.js :: MsgGetMessagesForAllServers :: line 1433"  data: no]

But the test still passes, that is why it was not detected.
It seems "Get all mail" does not work for POP3 servers in TB now.
Neil, can you audit all callers of the changed DownloadMailFromServers ?

Comment 26

[Sven Grull]

I just created bug 855631 for SeaMonkey because of the get all mails problem.

Comment 27

[:aceman]

Thanks for confirmation. So we can move over there.