661682 - make backend [noscript] methods scriptable where feasible

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

We have a few noscript methods in the mailnews/base and mailnews/db that could be easily made scriptable. Doing so will make it possible to access the functionality from js, and down the road, facilitate skinkglue. I'll submit a patch in a bit.

Comment 1

[David :Bienvenu]

Attachment: wip

this builds and seems to run. "Easily" might have been an overstatement, however. Kent, does this help simplify the skink stuff? Did I miss any important method(s)?

Comment 2

[David :Bienvenu]

looks like I might have messed up folder compaction...

Comment 3

[Kent James (:rkent)]

Yes this does help move us toward scriptable interfaces, which is useful for javascript-based users. But just to be clear, the actual problem that SkinkGlue had was all at the C++ level, caused by the fact that I was trying to pass TArray pointer items between two C++ methods but in different .dll files, which caused MarkAllRead to assert. Your fixes should also fix that problem as well.

Personally though I hate adding all those new XPCOM arrays to code that is primarily C++, as that introduces new non-smart pointers with all of the associated memory management headaches.  When I faced a similar problem in ExQuilla (but for string arrays), I created a simple string array XPCOM object. But that forces all access to go through methods, like theArray->GetAt(i, &theElement)  So if it were my code, I would do the same thing here, creating a simple integer array XPCOM object. But I am probably in the minority.

Comment 4

[David :Bienvenu]

Attachment: fix that passes xpcshell tests

this passes the unit tests. I've kicked off some try server builds as well.

I'm not crazy about getting rid of the array wrappers either, both because of the memory leak protection they offer, and the memory cloning I have to do in a few places. But I'd like the methods to be able to be called from js. A wrapper around arrays would be painful to use in js because we'd have to cross the xpconnect boundary for every array access, whereas xpconnect should know how to convert to js arrays efficiently.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

If we name-checked "fixIterator" hard, we could make it detect whatever C++ friendly thing you create/are already using and have it aggressively transform the structure, possibly using a JS-friendly mechanism (like returning the ugly raw array of things that XPConnect can be reasonably efficient about).  A lot of the JS code already uses fixIterator all over the place, so this wouldn't be a totally crazy thing.

It's also possible to use nsIClassInfo to wrap XPCOM objects with magic JS-friendly wrappers that only JS sees, but they are frankly a bit confusing and easy to screw-up.  (mozStorage has some if you want to see examples).

Comment 6

[David :Bienvenu]

(In reply to comment #5)
> If we name-checked "fixIterator" hard, we could make it detect whatever C++
> friendly thing you create/are already using and have it aggressively
> transform the structure, possibly using a JS-friendly mechanism (like
> returning the ugly raw array of things that XPConnect can be reasonably
> efficient about).  A lot of the JS code already uses fixIterator all over
> the place, so this wouldn't be a totally crazy thing.

So, something like nsIMsgKeyArray, which interface fixIterator would check for, and call the toArray(out unsigned long aCount, [array, size_is(aCount)] out nsMsgKey aKeys)

method to get the raw array and iterate over. Yeah, I guess we could do that. It's more work than what I've done in this patch, but it could be handy in the future as well.

Comment 7

[David :Bienvenu]

Attachment: wip on nsMsgKeyArray approach

this is the nsMsgKeyArray approach - I've got one test failure to fix, and I suspect I can revert some of the previous changes w/ this approach, but this is what I had in mind...

Comment 8

[Kent James (:rkent)]

Comment on attachment 539118 [details] [diff] [review]
wip on nsMsgKeyArray approach

Yes this is looking good. There is at least one place where you have NS_ENSURE_SUCCESS statements that can execute before a ::Free and leak. Also, while the ::Free are probably unavoidable, couldn't you avoid the ::Clone somehow outside of the new key array class by using a method in your new key array class?

Thanks for taking this on.

Comment 9

[David :Bienvenu]

(In reply to comment #8)
> Comment on attachment 539118 [details] [diff] [review] [review]
> wip on nsMsgKeyArray approach
> 
> Yes this is looking good. There is at least one place where you have
> NS_ENSURE_SUCCESS statements that can execute before a ::Free and leak.
> Also, while the ::Free are probably unavoidable, couldn't you avoid the
> ::Clone somehow outside of the new key array class by using a method in your
> new key array class?

Yeah, I could add a CloneKeys method to the key array class (or ToArray). That would be a bit cleaner.
> 
> Thanks for taking this on.

Sure, thanks in advance for reviewing it :-)

Comment 10

[David :Bienvenu]

(In reply to comment #9)

> Yeah, I could add a CloneKeys method to the key array class (or ToArray).
> That would be a bit cleaner.

actually, all the remaining nsMemory::Clone calls are in code that doesn't use nsMsgKeyArray but rather, nsTArray directly. I'm not inclined to change those to use nsMsgKeyArray just to be able to use the getArray method instead of nsMemory::Clone (it wouldn't buy us anything else, I don't think).

Comment 11

[David :Bienvenu]

Attachment: fix for review

this fixes the issue with error returns before freeing up the keys, by putting code in a helper routine. I also added code to allow creation of the nsMsgKeyArray's from js.

Comment 12

[David :Bienvenu]

Attachment: use SetCapacity

oops, I forgot to use SetCapacity in ListAllKeys, after I went to the trouble of exposing it. Since ListAllKeys is called somewhat regularly, it would be nice if it was friendlier to the heap by preallocating the key array storage.

Comment 13

[Kent James (:rkent)]

Comment on attachment 539293 [details] [diff] [review]
use SetCapacity

Here is a first pass review. I need to do more still.

Remove nsMemory::Free(thoseMarked) from AddMarkAllReadUndoAction

This is a duplicate, plus IMHO it is a bad idea to hide that FREE within a subroutine.

Also in AddMarkAllReadUndoAction, you have msgWindow-> without checking for null msgWindow. Yes you do it upstream, but our current practice is to add a NS_ENSURE_ARG_POINTER at the top of a routine that will crash with a null input.


Is there any reason that you left these:

interface nsIMsgDatabase : nsIDBChangeAnnouncer {
...
  [noscript] void ListAllOfflineOpIds(in nsMsgKeyArrayPtr offlineOpIds);
  [noscript] void ListAllOfflineDeletes(in nsMsgKeyArrayPtr offlineDeletes);
  
nsMsgDatabase::MarkAllRead doesn't set *aNumKeys

In nsMsgDatabase::DumpContents you don't want nsMemory::Free(keys) of the nsRefPtr

keyCount is never set in:

@@ -2726,8 +2713,11 @@ NS_IMETHODIMP nsImapMailFolder::UpdateIm
       PR_snprintf(intStrBuf, sizeof(intStrBuf), "%llu",  mailboxHighestModSeq);
       dbFolderInfo->SetCharProperty(kModSeqPropertyName, nsDependentCString(intStrBuf));
     }
-    mDatabase->ListAllKeys(existingKeys);
-    PRInt32 keyCount = existingKeys.Length();
+    PRUint32 keyCount;
+    nsRefPtr<nsMsgKeyArray> keys = new nsMsgKeyArray;
+    rv = mDatabase->ListAllKeys(keys);
+    NS_ENSURE_SUCCESS(rv, rv);
+    existingKeys.AppendElements(keys->m_keys);
     mDatabase->ListAllOfflineDeletes(&existingKeys);
     if (keyCount < existingKeys.Length())

Missing space:

+nsresult nsMailboxService::CopyMessages(PRUint32 aNumKeys,
+                                        nsMsgKey*aMsgKeys,

Comment 14

[David :Bienvenu]

(In reply to comment #13)
Thx for looking at this...
> 
> Here is a first pass review. I need to do more still.
> 
> Remove nsMemory::Free(thoseMarked) from AddMarkAllReadUndoAction
> 
> This is a duplicate, plus IMHO it is a bad idea to hide that FREE within a
> subroutine.
Ooh, good catch.
> 
> Also in AddMarkAllReadUndoAction, you have msgWindow-> without checking for
> null msgWindow. Yes you do it upstream, but our current practice

That's current practice for IDL methods, not normal C++ methods. Because we have no control over the callers of the idl methods.

 
> Is there any reason that you left these:
> 
> interface nsIMsgDatabase : nsIDBChangeAnnouncer {
> ...
>   [noscript] void ListAllOfflineOpIds(in nsMsgKeyArrayPtr offlineOpIds);
>   [noscript] void ListAllOfflineDeletes(in nsMsgKeyArrayPtr offlineDeletes);
>   

Yeah, I didn't think anyone in their right mind would call those from js. I'm not against fixing those as well, since I guess it would allow someone to implement nsIMsgDatabases in js, but I think that's less likely than implementing folders and servers, etc, in js.

Comment 15

[David :Bienvenu]

I should say that the pluggable store code gets rid of some of the need for sub-classing nsMsgDatabase.

Comment 16

[Kent James (:rkent)]

Comment on attachment 539293 [details] [diff] [review]
use SetCapacity

Here's the rest of my review. Because of the number of changes, I should look at it again:

1) MailNewsTypes.h not needed anymore at:

diff --git a/mailnews/base/public/nsIMsgHdr.idl b/mailnews/base/public/nsIMsgHdr.idl
--- a/mailnews/base/public/nsIMsgHdr.idl
+++ b/mailnews/base/public/nsIMsgHdr.idl
@@ -38,22 +38,20 @@
 #include "nsISupports.idl"
 
 %{C++
 #include "MailNewsTypes.h"
 %}
 
2) aNumMarked not set in:
nsMsgDatabase::MarkThreadRead

3) you need a check for null keys (or NS_SUCCEEDED) at:
 nsresult nsMsgDatabase::DumpContents()
 {
   nsMsgKey key;
   PRUint32 i;
 
-  nsTArray<nsMsgKey> keys;
+  nsRefPtr<nsMsgKeyArray> keys = new nsMsgKeyArray;
   nsresult rv = ListAllKeys(keys);
-  for (i = 0; i < keys.Length(); i++) {
-    key = keys[i];
+  PRUint32 numKeys;
+  keys->GetLength(&numKeys);

4) the key array is remade, but you are not updating m_size here:

@@ -297,28 +299,27 @@ nsFolderCompactState::Init(nsIMsgFolder 
   m_file->InitWithFile(path);
   // need to make sure the temp file goes in the same real directory
   // as the original file, so resolve sym links.
   m_file->SetFollowLinks(PR_TRUE);
 
   m_file->SetNativeLeafName(NS_LITERAL_CSTRING("nstmp"));
   m_file->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 00600);   //make sure we are not crunching existing nstmp file
   m_window = aMsgWindow;
-  m_keyArray.Clear();
+  m_keyArray = new nsMsgKeyArray;
   m_totalMsgSize = 0;
   rv = InitDB(db);
   if (NS_FAILED(rv))
   {
     CleanupTempFilesAfterError();
     return rv;
   }
 
-  m_size = m_keyArray.Length();
   m_curIndex = 0;
   
5) this addition is a duplicate:
@@ -77,23 +78,23 @@ nsFolderCompactState::nsFolderCompactSta
   m_curIndex = -1;
   m_status = NS_OK;
   m_compactAll = PR_FALSE;
   m_compactOfflineAlso = PR_FALSE;
   m_compactingOfflineFolders = PR_FALSE;
   m_parsingFolder=PR_FALSE;
   m_folderIndex = 0;
   m_startOfMsg = PR_TRUE;
+  m_size = 0;
   m_needStatusLine = PR_FALSE;
 }
 
 6) missing space at:
+nsresult nsMailboxService::CopyMessages(PRUint32 aNumKeys,
+                                        nsMsgKey*aMsgKeys,

Comment 17

[David :Bienvenu]

Attachment: fix addressing comments

this addresses the previous comments, except for the one about checking for a null msgWindow, in a non-idl method.

I had to add an include of MailNewsTypes.h to mapiimp.cpp since it had been relying on the include in nsIMsgHdr.idl.

Comment 18

[Kent James (:rkent)]

Comment on attachment 539927 [details] [diff] [review]
fix addressing comments

You still have not incorporated the following issues from the first review, but r-me with these items incorporated:

1) nsMsgDatabase::MarkAllRead doesn't set *aNumKeys

2) keyCount is never set in:

@@ -2726,8 +2713,11 @@ NS_IMETHODIMP nsImapMailFolder::UpdateIm
       PR_snprintf(intStrBuf, sizeof(intStrBuf), "%llu",  mailboxHighestModSeq);
       dbFolderInfo->SetCharProperty(kModSeqPropertyName, nsDependentCString(intStrBuf));
     }
-    mDatabase->ListAllKeys(existingKeys);
-    PRInt32 keyCount = existingKeys.Length();
+    PRUint32 keyCount;
+    nsRefPtr<nsMsgKeyArray> keys = new nsMsgKeyArray;
+    rv = mDatabase->ListAllKeys(keys);
+    NS_ENSURE_SUCCESS(rv, rv);
+    existingKeys.AppendElements(keys->m_keys);
     mDatabase->ListAllOfflineDeletes(&existingKeys);
     if (keyCount < existingKeys.Length())

3) You added a duplicate m_size = 0 here:

nsFolderCompactState::nsFolderCompactState()
{
  m_fileStream = nsnull;
  m_size = 0;
  m_curIndex = -1;
  m_status = NS_OK;
  m_compactAll = PR_FALSE;
  m_compactOfflineAlso = PR_FALSE;
  m_compactingOfflineFolders = PR_FALSE;
  m_parsingFolder=PR_FALSE;
  m_folderIndex = 0;
  m_startOfMsg = PR_TRUE;
  m_size = 0;
  m_needStatusLine = PR_FALSE;
}

Comment 19

[David :Bienvenu]

Attachment: fix addressing comments

ugh, sorry for missing the review comments. I believe I've addressed them now.

Comment 20

[David :Bienvenu]

pinging for sr...I'd like to get this baking soon.

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to comment #20)
> pinging for sr...I'd like to get this baking soon.

so pinging for commit :-D

Comment 22

[David :Bienvenu]

http://hg.mozilla.org/comm-central/rev/8f6268c4dc0a