Closed Bug 661682 Opened 8 years ago Closed 8 years ago

make backend [noscript] methods scriptable where feasible

Categories

(MailNews Core :: Backend, defect)

x86
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 7.0

People

(Reporter: Bienvenu, Assigned: Bienvenu)

Details

Attachments

(2 files, 5 obsolete files)

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.
Attached patch wip (obsolete) — Splinter Review
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)?
looks like I might have messed up folder compaction...
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.
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.
Attachment #537652 - Attachment is obsolete: true
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).
(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.
Attached patch wip on nsMsgKeyArray approach (obsolete) — Splinter Review
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...
Attachment #539118 - Attachment is patch: true
Attachment #539118 - Attachment mime type: text/x-patch → text/plain
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.
(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 :-)
(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).
Attached patch fix for review (obsolete) — Splinter 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.
Attachment #539118 - Attachment is obsolete: true
Attachment #539283 - Flags: superreview?(mbanner)
Attachment #539283 - Flags: review?(kent)
Attached patch use SetCapacity (obsolete) — Splinter Review
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.
Attachment #539283 - Attachment is obsolete: true
Attachment #539283 - Flags: superreview?(mbanner)
Attachment #539283 - Flags: review?(kent)
Attachment #539293 - Flags: superreview?(mbanner)
Attachment #539293 - Flags: review?(kent)
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,
(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.
I should say that the pluggable store code gets rid of some of the need for sub-classing nsMsgDatabase.
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,
Attachment #539293 - Flags: review?(kent) → review-
Attached patch fix addressing comments (obsolete) — Splinter Review
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.
Attachment #539293 - Attachment is obsolete: true
Attachment #539293 - Flags: superreview?(mbanner)
Attachment #539927 - Flags: superreview?(mbanner)
Attachment #539927 - Flags: review?(kent)
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;
}
Attachment #539927 - Flags: review?(kent) → review+
ugh, sorry for missing the review comments. I believe I've addressed them now.
Attachment #539927 - Attachment is obsolete: true
Attachment #539927 - Flags: superreview?(mbanner)
Attachment #540127 - Flags: superreview?(mbanner)
Attachment #540127 - Flags: review+
pinging for sr...I'd like to get this baking soon.
Status: NEW → ASSIGNED
Attachment #540127 - Flags: superreview?(mbanner) → superreview+
(In reply to comment #20)
> pinging for sr...I'd like to get this baking soon.

so pinging for commit :-D
http://hg.mozilla.org/comm-central/rev/8f6268c4dc0a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 7.0
You need to log in before you can comment on or make changes to this bug.