Last Comment Bug 831993 - convert nsISupportsArray m_serversToGetNewMailFor variable from nsPop3IncomingServer.cpp to something better
: convert nsISupportsArray m_serversToGetNewMailFor variable from nsPop3Incomin...
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Networking: POP (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 22.0
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
Depends on: 855631
Blocks: 394167 838805
  Show dependency treegraph
 
Reported: 2013-01-17 13:12 PST by :aceman
Modified: 2013-04-10 00:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch (14.16 KB, patch)
2013-01-17 14:28 PST, :aceman
no flags Details | Diff | Splinter Review
WIP patch 2 (13.96 KB, patch)
2013-01-17 15:32 PST, :aceman
neil: feedback+
Details | Diff | Splinter Review
Alternative approach (13.42 KB, patch)
2013-01-18 05:38 PST, neil@parkwaycc.co.uk
rkent: feedback-
Details | Diff | Splinter Review
WIP patch 2 v2 (14.07 KB, patch)
2013-01-18 11:50 PST, :aceman
rkent: feedback+
Details | Diff | Splinter Review
Alternative approach (12.44 KB, patch)
2013-01-20 04:34 PST, neil@parkwaycc.co.uk
standard8: review+
rkent: feedback+
Details | Diff | Splinter Review

Description :aceman 2013-01-17 13:12:56 PST
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 2013-01-17 14:28:29 PST
Created attachment 703598 [details] [diff] [review]
WIP patch
Comment 2 :aceman 2013-01-17 15:32:49 PST
Created attachment 703637 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2013-01-18 05:38:57 PST
Created attachment 703874 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2013-01-18 05:48:28 PST
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 2013-01-18 05:50:17 PST
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 2013-01-18 05:52:09 PST
(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 2013-01-18 05:56:28 PST
No, and you're going to need a stack trace if you want me to look harder.
Comment 8 :aceman 2013-01-18 11:50:23 PST
Created attachment 704019 [details] [diff] [review]
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) 2013-01-18 15:17:47 PST
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) 2013-01-18 15:28:38 PST
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) 2013-01-18 15:33:17 PST
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 neil@parkwaycc.co.uk 2013-01-18 15:54:07 PST
(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) 2013-01-18 17:01:14 PST
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 2013-01-19 07:14:59 PST
(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 neil@parkwaycc.co.uk 2013-01-20 04:34:35 PST
Created attachment 704319 [details] [diff] [review]
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) 2013-01-22 14:04:56 PST
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 neil@parkwaycc.co.uk 2013-01-22 16:23:31 PST
(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 2013-01-23 10:58:38 PST
So what now?
Comment 19 neil@parkwaycc.co.uk 2013-02-06 12:52:50 PST
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) 2013-02-12 13:16:18 PST
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 2013-02-22 03:59:27 PST
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) 2013-02-26 09:58:54 PST
I would be willing to review this if requested by Neil or Standard8 to do so.
Comment 23 Mark Banner (:standard8, limited time in Dec) 2013-03-24 15:07:42 PDT
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 neil@parkwaycc.co.uk 2013-03-25 07:51:53 PDT
Pushed comm-central changeset 323851e9e786.
Comment 25 :aceman 2013-03-27 16:39:34 PDT
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 2013-03-28 01:05:10 PDT
I just created bug 855631 for SeaMonkey because of the get all mails problem.
Comment 27 :aceman 2013-03-28 01:24:14 PDT
Thanks for confirmation. So we can move over there.

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