413413 - Replace nsMsgKeyArray with nsTArray<nsMsgKey>

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail]]

See bug 258018

Comment 1

[[e-mail]]

Attachment: Work in progess

Comment 2

[[e-mail]]

Attachment: Pass arrays by reference

I was going to start by converting GetAt to operator[] but that doesn't work so nicely for pointers so instead I started by turning them into references.

I added an overload for a writable operator[], this allows me to convert SetAt to operator[] too, but so far I have only used it for these references.

nsTArray's operator [] is also writable of course.

I also removed a pointless-looking override of DeleteMessages().

Comment 3

[[e-mail]]

Attachment: Replace GetAt with operator[]

Comment 4

[[e-mail]]

Attachment: Don't create arrays of one key

Since taking the address of a variable results in an array of one element.

Comment 5

[David :Bienvenu]

Comment on attachment 303419 [details] [diff] [review]
Pass arrays by reference

nice, thx.

Comment 6

[David :Bienvenu]

Comment on attachment 304833 [details] [diff] [review]
Don't create arrays of one key

coulda swore I reviewed this...but bugzilla doesn't lie.

Comment 7

[[e-mail]]

Attachment: Replace SetAt with operator[]

I've also removed SetAt and made SetAtGrow and GetAt protected (operator[] uses it) which turned up a case missed by attachment 304804 [details] [diff] [review].

Comment 8

[[e-mail]]

Attachment: Miscellaneous fixes

* Simplify nsMsgSearchNews::CollateHits
* Simplify one use of SetAt in a different way
* Switch nsImapMoveCoalescer to use nsTArray instead of nsVoidArray
* Fix a one-key array in nsMsgComposeSendListener I missed earlier

Comment 9

[[e-mail]]

Attachment: The Big One

25% down, this is the last 75%!

Comment 10

[[e-mail]]

Comment on attachment 309508 [details] [diff] [review]
The Big One

i see a lot of instances of |somearray.Length() > 0| and variations throughout the patch... can you switch them to use IsEmpty() instead? you should be able to search through the diff for .Length() > 0, .Length() == 0. there are also a few cases of |if (array.Length())| which you should be able to catch by searching for .Length())...

>Index: mailnews/base/search/src/nsMsgSearchNews.cpp
>===================================================================
>-  m_candidateHits.QuickSort(CompareArticleNumbers);
>+  m_candidateHits.Sort();

ohh, such beautiful template syntactic sugar ;)

>Index: mailnews/base/src/nsMsgDBView.cpp
>===================================================================
>@@ -764,7 +764,7 @@ nsresult nsMsgDBView::SaveAndClearSelect
>-  aMsgKeyArray.SetSize(numIndices);
>+  aMsgKeyArray.SetLength(numIndices);

are we sure not to run into more topcrashiness with this? as long as you've manually verified that this, and each case below, is safe then i'm happy.

>@@ -4844,7 +4830,7 @@ nsresult nsMsgDBView::ListIdsInThread(ns
>-  m_keys.InsertAt(viewIndex, 0, numChildren);
>+  m_keys.InsertElementsAt(viewIndex, numChildren);

topcrashiness again, maybe |m_keys.InsertElementsAt(viewIndex, numChildren, 0);| ?

>@@ -5229,7 +5215,7 @@ nsresult nsMsgDBView::MarkThreadRead(nsI
>-    idsMarkedRead.AllocateSpace(numChildren);
>+    idsMarkedRead.SetCapacity(numChildren);

topcrashiness - AllocateSpace zeros the array too, is it safe?

>Index: mailnews/base/src/nsMsgGroupThread.cpp
>===================================================================
>@@ -298,7 +298,7 @@ nsresult nsMsgGroupThread::RemoveChild(n
> {
>   PRUint32 childIndex = m_keys.IndexOf(msgKey);
>   if (childIndex != kNotFound)
>-    m_keys.RemoveAt(childIndex);
>+    m_keys.RemoveElementAt(childIndex);

|m_keys.RemoveElement(msgKey);|

>Index: mailnews/base/src/nsMsgGroupView.cpp
>===================================================================
>@@ -539,7 +539,7 @@ nsresult  nsMsgQuickSearchDBView::ListId
>-  if ( index > ((nsMsgViewIndex) m_keys.GetSize()))
>+  if (index > ((nsMsgViewIndex) m_keys.Length()))
>     return NS_MSG_MESSAGE_NOT_FOUND;

looks suspicious - does this want >=?

>Index: mailnews/base/src/nsMsgThreadedDBView.cpp
>===================================================================
>@@ -186,7 +186,7 @@ nsresult nsMsgThreadedDBView::SortThread
>-  m_keys.SetSize(numThreads);
>+  m_keys.SetLength(numThreads);

>@@ -238,7 +238,7 @@ nsresult nsMsgThreadedDBView::AddKeys(ns
>-  m_keys.AllocateSpace(numKeysToAdd+m_keys.GetSize());
>+  m_keys.SetCapacity(m_keys.Length() + numKeysToAdd);

>Index: mailnews/base/util/nsMsgDBFolder.cpp
>===================================================================
>@@ -1996,7 +1996,6 @@ nsMsgDBFolder::CallFilterPlugins(nsIMsgW
>-  m_saveNewMsgs.RemoveAll();

is this right?

>Index: mailnews/imap/src/nsImapFlagAndUidState.cpp
>===================================================================
>@@ -120,7 +120,7 @@ nsImapFlagAndUidState::nsImapFlagAndUidS
>-  fUids.SetSize(fNumberOfMessageSlotsAllocated);
>+  fUids.SetLength(fNumberOfMessageSlotsAllocated);

>@@ -224,7 +224,7 @@ NS_IMETHODIMP nsImapFlagAndUidState::Add
>-    fUids.SetSize(fNumberOfMessageSlotsAllocated);
>+    fUids.SetLength(fNumberOfMessageSlotsAllocated);

>Index: mailnews/imap/src/nsImapMailFolder.cpp
>===================================================================
>@@ -2507,12 +2505,12 @@ NS_IMETHODIMP nsImapMailFolder::UpdateIm
>-  if (keysToDelete.GetSize() && mDatabase)
>+  if (keysToDelete.Length() && mDatabase)

pointing this one out since the search won't catch it.

r=me with those fixes!

Comment 11

[[e-mail]]

(In reply to comment #10)
>(From update of attachment 309508 [details] [diff] [review])
>i see a lot of instances of |somearray.Length() > 0| and variations throughout
>the patch... can you switch them to use IsEmpty() instead?
Done.

>>Index: mailnews/base/search/src/nsMsgSearchNews.cpp
>>===================================================================
>>-  m_candidateHits.QuickSort(CompareArticleNumbers);
>>+  m_candidateHits.Sort();
>ohh, such beautiful template syntactic sugar ;)
Ironically QuickSort has a default argument...

>>Index: mailnews/base/src/nsMsgDBView.cpp
>>===================================================================
>>@@ -764,7 +764,7 @@ nsresult nsMsgDBView::SaveAndClearSelect
>>-  aMsgKeyArray.SetSize(numIndices);
>>+  aMsgKeyArray.SetLength(numIndices);
>are we sure not to run into more topcrashiness with this? as long as you've
>manually verified that this, and each case below, is safe then i'm happy.
This is safe, we overwrite all the indices.

>>@@ -4844,7 +4830,7 @@ nsresult nsMsgDBView::ListIdsInThread(ns
>>-  m_keys.InsertAt(viewIndex, 0, numChildren);
>>+  m_keys.InsertElementsAt(viewIndex, numChildren);
> topcrashiness again, maybe |m_keys.InsertElementsAt(viewIndex, numChildren,
> 0);| ?
Good catch.

>>@@ -5229,7 +5215,7 @@ nsresult nsMsgDBView::MarkThreadRead(nsI
>>-    idsMarkedRead.AllocateSpace(numChildren);
>>+    idsMarkedRead.SetCapacity(numChildren);
>topcrashiness - AllocateSpace zeros the array too, is it safe?
Again, we overwrite all the entries here.

>>Index: mailnews/base/src/nsMsgGroupThread.cpp
>>===================================================================
>>@@ -298,7 +298,7 @@ nsresult nsMsgGroupThread::RemoveChild(n
>> {
>>   PRUint32 childIndex = m_keys.IndexOf(msgKey);
>>   if (childIndex != kNotFound)
>>-    m_keys.RemoveAt(childIndex);
>>+    m_keys.RemoveElementAt(childIndex);
>|m_keys.RemoveElement(msgKey);|
Nice.

> 
>>Index: mailnews/base/src/nsMsgQuickSearchDBView.cpp
>>===================================================================
>>@@ -539,7 +539,7 @@ nsresult  nsMsgQuickSearchDBView::ListId
>>-  if ( index > ((nsMsgViewIndex) m_keys.GetSize()))
>>+  if (index > ((nsMsgViewIndex) m_keys.Length()))
>looks suspicious - does this want >=?
Not my bug but I'll fix it ;-)

>>Index: mailnews/base/src/nsMsgThreadedDBView.cpp
>>===================================================================
>>@@ -186,7 +186,7 @@ nsresult nsMsgThreadedDBView::SortThread
>>-  m_keys.SetSize(numThreads);
>>+  m_keys.SetLength(numThreads);
This is shrinking the array.

>>@@ -238,7 +238,7 @@ nsresult nsMsgThreadedDBView::AddKeys(ns
>>-  m_keys.AllocateSpace(numKeysToAdd+m_keys.GetSize());
>>+  m_keys.SetCapacity(m_keys.Length() + numKeysToAdd);
Allocate space doesn't change the length, it just makes appending faster.

>>Index: mailnews/base/util/nsMsgDBFolder.cpp
>>===================================================================
>>@@ -1996,7 +1996,6 @@ nsMsgDBFolder::CallFilterPlugins(nsIMsgW
>>-  m_saveNewMsgs.RemoveAll();
>is this right?
Earlier I swapped it with an empty array so it's already empty.

>>Index: mailnews/imap/src/nsImapFlagAndUidState.cpp
>>===================================================================
>>@@ -120,7 +120,7 @@ nsImapFlagAndUidState::nsImapFlagAndUidS
>>-  fUids.SetSize(fNumberOfMessageSlotsAllocated);
>>+  fUids.SetLength(fNumberOfMessageSlotsAllocated);
>>@@ -224,7 +224,7 @@ NS_IMETHODIMP nsImapFlagAndUidState::Add
>>-    fUids.SetSize(fNumberOfMessageSlotsAllocated);
>>+    fUids.SetLength(fNumberOfMessageSlotsAllocated);
Fixed to use InsertElementsAt (3-arg version that initialises the elements).

>>Index: mailnews/imap/src/nsImapMailFolder.cpp
>>===================================================================
>>@@ -2507,12 +2505,12 @@ NS_IMETHODIMP nsImapMailFolder::UpdateIm
>>-  if (keysToDelete.GetSize() && mDatabase)
>>+  if (keysToDelete.Length() && mDatabase)
>pointing this one out since the search won't catch it.
My search was slightly different so I did in fact spot it but thanks anyway.

Comment 12

[[e-mail]]

Attachment: Addressed review comments

(See comment #11)

Comment 13

[[e-mail]]

Comment on attachment 309685 [details] [diff] [review]
Addressed review comments

>Index: mailnews/base/src/nsMsgDBView.cpp
>===================================================================
>@@ -4245,7 +4234,7 @@ nsresult nsMsgDBView::ExpansionDelta(nsM
>-  if ( index > ((nsMsgViewIndex) m_keys.GetSize()))
>+  if ( index > ((nsMsgViewIndex) m_keys.Length()))
>     return NS_MSG_MESSAGE_NOT_FOUND;

just spotted this one too, since you're fixing one you should fix the other :)

nice stuff!

Comment 14

[David :Bienvenu]

Comment on attachment 309685 [details] [diff] [review]
Addressed review comments

whew, that was a big patch!

Comment 15

[[e-mail]]

Fix checked in.

Comment 16

[Joe Sabash [:JoeS1]]

Tried an hourly, since this was so big a checkin:
Crash on startup
http://crash-stats.mozilla.com/report/index/4bb9531c-f6f5-11dc-a575-001a4bd43ed6?date=2008-03-21-03
This checkin?

Comment 17

[Robert Kaiser]

http://crash-stats.mozilla.com/report/list?range_unit=days&query_search=signature&query_type=contains&product=SeaMonkey%2CThunderbird&branch=1.9&signature=%400x0&range_value=1 all seem to be the same stack and I heavily suspect this patch as the cause.

Comment 18

[Ben Campbell]

Attachment: 413413-remove-nsMsgKeyArray-1.patch

Still vestiges of nsMsgKeyArray in there.
Here's another patch to remove it, then we can revisit it in another 13 years :-)

Comment 19

[Ben Campbell]

try run (not the exact patch, but close enough - I just tweaked some comments and rebased it):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=995f912a449fefffc196570780220a3ddfb495c2

Comment 20

[Magnus Melin [:mkmelin]]

Comment on attachment 9197657 [details] [diff] [review]
413413-remove-nsMsgKeyArray-1.patch

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

It's confusing reopening a 13yr old bug. Let's move the patch to a new bug...

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +297,5 @@
>     * Returns all message keys stored in the database.
>     * Keys are returned in the order as stored in the database.
>     * The caller should sort them if it needs to.
>     */
> +  Array<nsMsgKey> ListAllKeys();

might as well change to listAllKeys() while we're changing

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +4465,1 @@
>    nsresult rv = ListAllKeys(keys);

this rv shouldn't be ignored

@@ +4477,5 @@
>               subject.get());
>      }
>    }
>    nsTArray<nsMsgKey> threads;
>    rv = ListAllThreads(&threads);

and this

::: mailnews/local/src/nsPop3IncomingServer.cpp
@@ +135,1 @@
>                    rv = subFolderDB->ListAllKeys(keys);

this rv shouldn't be ignored