481881 - use better template arguments for nsTArray<T> after bug 474369

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Arpad Borsos [:Swatinem]]

Attachment: widget part

Per bug 474369 comment 33:

> +    nsTPtrArray<nsCString> mDataFlavors;
> 
> Mmm, that looks like a good candidate for conversion to nsTArray<nsCString>.
> Later.
> 
> +    nsTArray<nsString*> mDataFlavors;
> 
> That too!
> 
> +    nsTPtrArray<IDataObject> mDataObjects;

I changed some other cases to nsTArray<T> directly, where the elements are owned by the nsTArray.

Patch runs reftest fine on Linux, but is untested on windows however.

Comment 1

[Arpad Borsos [:Swatinem]]

Attachment: layout part

I found some more cases in layout.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great stuff!

+  nsCOMPtr<nsISupports> mData;   // OWNER - some varient of primitive wrapper
+  PRUint32 mDataLen;
+  const nsCAutoString mFlavor;
+  char *   mCacheFileName;

There's a subtle bug here. You can't put objects containing an nsCAutoString directly into an nsTArray. The array can move its contents around in memory, and ns(C)AutoString objects (and nsAutoTArray objects) can contain an internal pointer to themselves, which is not updated when the object moves. Here I think you should just use an nsCString.

-  nsTArray<DataStruct*> * mDataArray;
+  nsTArray<DataStruct> * mDataArray;

Here, there is no benefit to heap-allocating the nsTArray. It should just be a direct member of the class.

Comment 3

[Arpad Borsos [:Swatinem]]

Attachment: widget part, v2

Addressed the comments.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    DR_FrameTypeInfo info = mFrameTypeTable.ElementAt(i);

Use "DR_FrameTypeInfo& info" to avoid making a copy (2 occurrences)

+    DeepTreeStackItem top = mStack.ElementAt(mStack.Length()-1);

Similar here

+  if(!mBCInfo->mRightBorders.SetLength(aRowIndex+1))

Missing space after 'if'

You'd better document that nsTableCellMap::GetBCData returns a pointer into the array and that pointer should not be used after anything that could change the array (since that could move the data in memory). Actually, it would be better to remove GetBCData and replace it with a method that just sets SetTopStart(PR_FALSE) directly on the data, so we don't have to worry about what happens after we return the pointer.

+  if (!mCols.AppendElements(aNumCols))
+    NS_WARNING("Could not AppendElement");
+  if (mBCInfo) {
+    if (!mBCInfo->mBottomBorders.AppendElements(aNumCols))
+      NS_WARNING("Could not AppendElement");

The NS_WARNINGs should be in {} since they're not control transfers.

+            if(!mBCInfo->mRightBorders.InsertElementAt(rowX))

Need space after 'if' (2 occurrences)

+    nsColInfo colInfo = mCols.ElementAt(colX);

Use "nsColInfo& colInfo" to avoid a copy

+        BCData cd = mBCInfo->mBottomBorders.ElementAt(colIndex);

"BCData& cd" (2 occurrences)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-    nsCString * df = mDataFlavors.SafeElementAt(dfInx);
-    if ( df ) {

I don't think we should get rid of this if. Just check dfInx >= 0 && dfInx < mDataFlavours.Length()

+    nsCString df = mDataFlavors.SafeElementAt(dfInx, nsCString());

You want
  nsCString& df = mDataFlavors[dfInx];
now that we've checked the index is in bounds. Avoid copies!

+    nsCString df = mDataFlavors.SafeElementAt(dfInx, nsCString());

Here you can use nsCString& df = mDataFlavours.SafeElementAt(dfInx, EmptyCString()). Can't use nsCString() as the default value if we're going to hold a reference to it, since it will go away after this statement.

+#include "nsAutoPtr.h"

nsRefPtr.h?

+    ScreenListItem curr = mScreenList[i];

ScreenListItem& curr

+    DataStruct data = pArray->ElementAt (i);

DataStruct& data

+      return &pArray->ElementAt (i);

return &data. Although actually, I think GetDataForFlavor would work better if it just returned the index in the array or NoIndex if not found. Then RemoveDataFlavor can keep calling GetDataForFlavor.

+    DataStruct data = mDataArray.ElementAt(i);

DataStruct& data (many occurrences!)

Comment 6

[Arpad Borsos [:Swatinem]]

Attachment: layout part, v2

Comment 7

[Arpad Borsos [:Swatinem]]

Attachment: widget part, v3 [pushed]

> +#include "nsAutoPtr.h"
> 
> nsRefPtr.h?

It actually is in nsAutoPtr.h :-P
http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsAutoPtr.h#918

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 366082 [details] [diff] [review]
layout part, v2

+  if (bcData)
+    bcData->SetTopStart(PR_FALSE);

Need {} around this statement since it's not a control transfer

There's a few "if(" you've added here too.

Comment 9

[Arpad Borsos [:Swatinem]]

Attachment: layout part, v3

Comment 10

[Arpad Borsos [:Swatinem]]

Changing Component to General for the cases identified by Jonas in bug 474369 comment 60:
> Do you even need this Clear() call? Or even better, could you make this into
> 
> nsAutoTArray<olState, 8> mOLStateStack;
> 
> That would avoid all manual memory management. Feel free to leave this for
> later

> I think you can make mNameSpaces into an nsTArray<nsNameSpaceEntry> to avoid
> the manual memory management. Feel free to leave this for later

> Seems like mObservers should be an nsCOMArray<nsIWeakReference>, that will
> avoid the manual refcounting. Feel free to leave this for later

> You can make this into nsTArray<SortKey> instead. Feel free to leave this for
> later

> This one could be converted to nsTArray<nsTemplateRule> too, though it's quite
> a bit of work so probably better for a separate patch.

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 366083 [details] [diff] [review]
widget part, v3 [pushed]

http://hg.mozilla.org/mozilla-central/rev/3ca2646bf687

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 366215 [details] [diff] [review]
layout part, v3

http://hg.mozilla.org/mozilla-central/rev/dac7c3176b33

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 366083 [details] [diff] [review]
widget part, v3 [pushed]

And
http://hg.mozilla.org/mozilla-central/rev/7caeec780633

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 366215 [details] [diff] [review]
layout part, v3

Backed out due to -ve leaks in DR_FrameTypeInfo:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1236740534.1236740722.1036.gz

      |<----------------Class--------------->|<-----Bytes------>|<----------------Objects---------------->|<--------------References-------------->|
                                              Per-Inst   Leaked    Total      Rem      Mean       StdDev     Total      Rem      Mean       StdDev
-  23 DR_FrameTypeInfo                               56        0       33        0 (   16.50 +/-     9.61)        0        0 (    0.00 +/-     0.00)
+  23 DR_FrameTypeInfo                               56 -2147483648       33      -33 (    0.00 +/-     0.00)        0        0 (    0.00 +/-     0.00)

Comment 15

[[e-mail]]

Comment on attachment 366215 [details] [diff] [review]
layout part, v3

>   for (PRInt32 i = 0; i < numEntries; i++) {
>+    DR_FrameTypeInfo& info = mFrameTypeTable.ElementAt(i);
>+    if (info.mType == aFrameType) {
>+      return &mFrameTypeTable.ElementAt(i);
Why not return &info (twice)?

>+    FrameData frameData = mFrames.ElementAt(i);
>+
>+    if (frameData.mFrame == aFrame) {
You only use frameData once, so you could write
if (mFrames.ElementAt[i].mFrame == aFrame) {

[If you want to be really lazy, write a comparator for FrameData:
template<class FrameData, class nsIFrame*>
class nsDefaultComparator {
  public:
    PRBool Equals(const FrameData& a, const nsIFrame* b) {
      return a.mFrame == b;
    }
};
and use mFrames.RemoveElement(aFrame); ]

>+    FrameData frameData = mFrames.ElementAt(i);
FrameData& frameData?

>-  DeepTreeStackItem()  { MOZ_COUNT_CTOR(DeepTreeStackItem); }
>-  ~DeepTreeStackItem() { MOZ_COUNT_DTOR(DeepTreeStackItem); }
>+  DeepTreeStackItem()  { MOZ_COUNT_CTOR(DeepTreeStackItem); }
>+  ~DeepTreeStackItem() { MOZ_COUNT_DTOR(DeepTreeStackItem); }
Do we still need to count CTOR/DTOR here or for DR_FrameTypeInfo?

Comment 16

[Arpad Borsos [:Swatinem]]

Attachment: layout part, v4 [pushed, comment 17]

> Do we still need to count CTOR/DTOR here or for DR_FrameTypeInfo?

Indeed, when not doing manual memory management, there is no need for that. This also fixes the negative leak without implementing a copy constructor (the default copy constructor works now).

I also integrated the other comments.

Comment 17

[Arpad Borsos [:Swatinem]]

Comment on attachment 366831 [details] [diff] [review]
layout part, v4 [pushed, comment 17]

http://hg.mozilla.org/mozilla-central/rev/9c36304c687f

Comment 18

[Arpad Borsos [:Swatinem]]

Attachment: content part

Based on top of attachment 366833 [details] [diff] [review].
SortKey caused me quite a headache, but I managed to work around nsTArrays copy construction behavior.

> Seems like mObservers should be an nsCOMArray<nsIWeakReference>, that will
> avoid the manual refcounting. Feel free to leave this for later

I rather made this a nsTArray<nsCOMPtr<...> >

Comment 19

[Arpad Borsos [:Swatinem]]

Attachment: content part, v2

The mac boxes had problems with the template specialization. This should work I hope.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 369242 [details] [diff] [review]
content part, v2

Some random drive-by comments:

>@@ -337,14 +331,12 @@ nsXMLContentSerializer::PushNameSpaceDec
>                                           const nsAString& aURI,
>                                           nsIDOMElement* aOwner)
> {
>-  NameSpaceDecl* decl = new NameSpaceDecl();
>-  if (!decl) return NS_ERROR_OUT_OF_MEMORY;
>-
>-  decl->mPrefix.Assign(aPrefix);
>-  decl->mURI.Assign(aURI);
>+  NameSpaceDecl decl;
>+  decl.mPrefix.Assign(aPrefix);
>+  decl.mURI.Assign(aURI);
>   // Don't addref - this weak reference will be removed when
>   // we pop the stack
>-  decl->mOwner = aOwner;
>+  decl.mOwner = aOwner;
> 
>   mNameSpaceStack.AppendElement(decl);

You could avoid the copy constructor (and keep the out-of-memory check):

  NameSpaceDecl* decl = mNameSpaceStack.AppendElement();
  if (!decl) return NS_ERROR_OUT_OF_MEMORY;

  decl->mPrefix.Assign(aPrefix);
  ...

>diff --git a/content/base/src/nsXMLNameSpaceMap.cpp b/content/base/src/nsXMLNameSpaceMap.cpp

>+template<>

You should probably use NS_SPECIALIZE_TEMPLATE.

> nsXMLNameSpaceMap::AddPrefix(nsIAtom *aPrefix, PRInt32 aNameSpaceID)

>+  if (!mNameSpaces.Contains(aPrefix)) {
>+    if (!mNameSpaces.AppendElement(aPrefix))

Combine these with &&

>diff --git a/content/svg/content/src/nsSVGValue.h b/content/svg/content/src/nsSVGValue.h

>+  nsAutoTArray<nsCOMPtr<nsIWeakReference>, 1> mObservers;

Maybe nsAutoTArray<nsWeakPtr, 1>?

Comment 21

[Arpad Borsos [:Swatinem]]

Attachment: content part, v3

With Peters comments fixed.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 369317 [details] [diff] [review]
content part, v3

>diff --git a/content/svg/content/src/nsSVGValue.cpp b/content/svg/content/src/nsSVGValue.cpp
>@@ -99,7 +92,7 @@ NS_IMETHODIMP
> NS_IMETHODIMP
> nsSVGValue::AddObserver(nsISVGValueObserver* observer)
> {
>-  nsIWeakReference* wr = NS_GetWeakReference(observer);
>+  nsCOMPtr<nsIWeakReference> wr = do_GetWeakReference(observer);

Use nsWeakRef instead.


>diff --git a/content/xul/templates/src/nsTemplateRule.cpp b/content/xul/templates/src/nsTemplateRule.cpp
>--- a/content/xul/templates/src/nsTemplateRule.cpp
>+++ b/content/xul/templates/src/nsTemplateRule.cpp
>@@ -293,14 +293,11 @@ nsTemplateRule::nsTemplateRule(nsIConten
>           mBindings(nsnull),
>           mConditions(nsnull)
> {
>-    MOZ_COUNT_CTOR(nsTemplateRule);
>     mRuleNode = do_QueryInterface(aRuleNode);
> }
> 
> nsTemplateRule::~nsTemplateRule()
> {
>-    MOZ_COUNT_DTOR(nsTemplateRule);
>-
>     while (mBindings) {
>         Binding* doomed = mBindings;
>         mBindings = mBindings->mNext;

It doesn't hurt to keep these macros. We might end up using this class elsewhere, or we might end up leaking the full array at which point it'd be helpful to know which array is leaking.

>diff --git a/content/xul/templates/src/nsTemplateRule.h b/content/xul/templates/src/nsTemplateRule.h

>+    void RemoveRule(nsTemplateRule *aRule)
>+    {
>+        for (PRUint32 i = 0, n = mRules.Length(); i < n; i++) {
>+            if (&mRules[i] == aRule) {
>+                mRules.RemoveElementAt(i);
>+                break;
>+            }
>+        }
>     }

This should work faster:

PRUint32 index = aRule - mRules.Elements();
if (index < mRules.Length()) {
  mRules.RemoveElementAt(index);
}

You should even be able to do
mRules.RemoveElementAt(aRule - mRules.Elements());
if you're sure the rule actually belongs to the array. Which it looks like you do.

r/sr=me with that fixed.

Comment 23

[Arpad Borsos [:Swatinem]]

Attachment: content part, for checkin [pushed: comment 24]

> It doesn't hurt to keep these macros. We might end up using this class
> elsewhere, or we might end up leaking the full array at which point it'd be
> helpful to know which array is leaking.
That required me to write a copy-constructor. I added a comment that it should only be used by nsTArray.
I will push the patch as soon as the tree reopens.

Comment 24

[Arpad Borsos [:Swatinem]]

Comment on attachment 369468 [details] [diff] [review]
content part, for checkin [pushed: comment 24]

http://hg.mozilla.org/mozilla-central/rev/52c7758d5a4d

Comment 25

[jag (Peter Annema)]

You can make that copy constructor private and make nsTArrayElementTraits<nsTemplateRule> a friend class to prevent accidental use.

Comment 26

[Arpad Borsos [:Swatinem]]

Attachment: make the copy constructor private

jag: are you doing reviews?

Comment 27

[Arpad Borsos [:Swatinem]]

Comment on attachment 369696 [details] [diff] [review]
make the copy constructor private

Try server is generating the following error. Works on my machine however. Don't quite now what should be wrong there.
nsTemplateRule.h:334 is the AppendElement(nsTemplateRule())
/builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/content/xul/templates/src/nsTemplateRule.h:237: error: ‘nsTemplateRule::nsTemplateRule(const nsTemplateRule&)’ is protected
/builds/buildbot/sendchange-slave/sendchange-linux-unittest/mozilla/content/xul/templates/src/nsTemplateRule.h:334: error: within this context

Comment 28

[jag (Peter Annema)]

Hrm, that's odd. It compiles with gcc 4.0.1 on Mac and gcc 4.1.2 on Linux. It could be the rules have been tightened in gcc and so that even when binding to a const Foo& you now need access to Foo's copy constructor.

Try adding friend class nsTArray<nsTemplateRule> to your previous patch.

Comment 29

[Arpad Borsos [:Swatinem]]

(In reply to comment #28)
> Try adding friend class nsTArray<nsTemplateRule> to your previous patch.

Tryserver is still failing with a compile error, whereas with gcc 4.3 on my machine it works fine.

Comment 30

[jag (Peter Annema)]

Hrm, I wonder what the tryserver is compiling with.

Comment 31

[jag (Peter Annema)]

After downloading a build and checking buildconfig.html:

gcc version 4.1.2 20061011 (Red Hat 4.1.1-29)

I tested using gcc version 4.1.2 20070626 (Red Hat 4.1.2-14)

I wonder what's different.

Comment 32

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) Remove leftover 'i' [Checkin: Comment 33]

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 373486 [details] [diff] [review]
(Ev1) Remove leftover 'i'
[Checkin: Comment 33]


http://hg.mozilla.org/mozilla-central/rev/9c4ad3fbb218