461047 - Replace nsStringArray with nsTArray<nsString>

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Maybe I'm missing something, but nsStringArray is just an nsVoidArray of heap-allocated nsStrings. Seems like if we just used nsTArray<nsString> instead, we'd win all around: less code, one less heap allocation per element, ability to use nsAutoTArray when needed.

Comment 1

[Fred Jendrzejewski]

Attachment: Problematic patch

Hi, I don't get forwards anymore. I can compile without any problem and I get no warnings. But the program crashes, because it is calling some never opened functions. The backtrace in the gnu debugger never mentions a function that I changed. I don't know great debugging strategies and I didn't get the patch producing the error really small. Because I am pretty busy, I have to document the stuff done here. The error must be in one of the files:
- content/base/public/nsIXPathEvaluatorInternal.h
- content/base/src/nsContentSink.cpp
- content/base/src/nsContentSink.h
- content/base/src/nsDOMLists.cpp
- content/base/src/nsDOMLists.h
- content/base/src/nsDocument.cpp
- content/base/src/nsNameSpaceManager.cpp
- content/base/src/nsStyleLinkElement.cpp
- content/base/src/nsStyleLinkElement.h
- content/html/content/public/nsIFormProcessor.h
- content/html/content/src/nsHTMLLinkElement.cpp
- content/html/content/src/nsHTMLScriptElement.cpp
- content/html/document/src/nsHTMLContentSink.cpp
- content/xslt/src/main/txTestExpr.cpp
- content/xslt/src/xml/txDOM.h
- content/xslt/src/xml/txNodeDefinition.cpp
- content/xslt/src/xpath/nsXPathEvaluator.cpp
- content/xslt/src/xpath/nsXPathEvaluator.h
- content/xul/templates/src/nsTemplateRule.cpp
- content/xul/templates/src/nsTemplateRule.h
- parser/htmlparser/src/CNavDTD.cpp
- security/manager/ssl/src/nsKeygenHandler.cpp
- security/manager/ssl/src/nsKeygenHandler.h

Are there any obvious mistakes or things I can do ?
Even a great book about really proper c++ programming would be welcome.

Sry for inconvenience,
Fred

Comment 2

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

Attachment: fixed patch

There was a mistake in the changes to nsNameSpaceManager:

-  const nsString* uri = mURIArray.StringAt(aNameSpaceID - 1);
-  if (!mURIToIDTable.Put(uri, aNameSpaceID)) {
-    mURIArray.RemoveStringAt(aNameSpaceID - 1);
+  const nsString uri = mURIArray.ElementAt(aNameSpaceID - 1);
+  if (!mURIToIDTable.Put(&uri, aNameSpaceID)) {
+    mURIArray.RemoveElementAt(aNameSpaceID - 1);

The keys in mURIToIDTable are just a pointer to a string. So this code is stashing a pointer to 'uri' into the table, but 'uri' is on the stack so it gets overwritten with garbage straight away. So all kinds of things go wrong.

We can't just write "const nsString& uri = ...;" and let mURIToIDTable have pointers to the nsStrings in mURIArray, because when you append to mURIArray later the whole array might move. So here we have to make it an nsTArray<nsAutoPtr<nsString> >. This updated patch does that.

However, it's not done yet because there are other things that need to be cleaned up.

Comment 3

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

+  *aResult = (PRInt32) mNames.IndexOf(aString) > -1;

Use a PRint32( ... ) style cast (constructor-style cast).

+    nsString *string =  mNames.AppendElement(aName);
+    if (nsnull != string)
+      return PR_TRUE;
+    return PR_FALSE;

"return mNames.AppendElement(aName) != nsnull;"

+    aResult  = mNamespaceURIs[aIndex];

Unnecessary space

     nsAutoString ns;
-    mNamespaceURIs.StringAt(index, ns);
+    ns = mNamespaceURIs[index];
 
     *aResult = ns.Equals(aNamespaceURI);

You can avoid the temporary string 'ns'. Just write the simple code,
  *aResult = mNamespaceURIs[index].Equals(aNamespaceURI);
This is a performance improvement as well as being simpler.

+    return ((mNames.AppendElement(aName) != nsnull) ? PR_TRUE : PR_FALSE);

Writing ? PR_TRUE : PR_FALSE is never good. Just "return mNames.AppendElement(aName) != nsnull;"

-      nsStringArray linkTypes(4);
+      nsTArray<nsString> linkTypes(4);
       nsStyleLinkElement::ParseLinkTypes(aValue, linkTypes);
-      dropSheet = linkTypes.IndexOf(NS_LITERAL_STRING("stylesheet")) < 0;
+      dropSheet = linkTypes.IndexOf(NS_LITERAL_STRING("stylesheet")) == PRUint32(-1);

Make that
  nsAutoTArray<nsString,4> linkTypes;
That allocates a stack buffer so we can avoid a heap allocation if there are no more than 4 link types.

+  nsTArray<nsString> linkTypes(4);

Same here.

+        nsTArray<nsString> linkTypes;

Same here. Although really nsStyleLinkElement::ParseLinkTypes should return a bitmask of known link types, or something like that, so we don't need to keep wrestling with strings. I'll file a followup bug for that.

+  nsTArray <nsString> requestedDomainArray;

Unnecessary space.

+  for (PRInt32 i = 0; i < array.Length(); ++i ) {
+    const nsString key = array[i];
+    CopyUTF16toUTF8(key, utf8Key);

This should be const nsString& since we should avoid making a copy. Or better still just write
  CopyUTF16toUTF8(array[i], utf8Key);

+          for (theIndex = theContent.Length()-1; theIndex > -1; --theIndex) {
+            nsString theTextValue = theContent[theIndex];

Similar here.

We can eliminate nsCStringArray while we're at it.

If you get rid of the last uses of nsStringArray/nsCStringArray, you should remove those files from the build.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to comment #3)
> +  *aResult = (PRInt32) mNames.IndexOf(aString) > -1;
>
> Use a PRint32( ... ) style cast (constructor-style cast).

*aResult = mNames.Contains(aString);

is available too.

Comment 5

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

Yeah, that's much better! thanks for reminding me.

Comment 6

[Fred Jendrzejewski]

Attachment: replace - v1

It builds fine and my basic testing produced no bugs. I can only build and test on a linux machine, so I expect some bugs on the other architectures.
I didn't replace the nsCStringArray because it has some additional functions (parseString etc). I don't know what to do with them in an intelligent way.
So I'll look out for other bugs to start. Are there other typical candidates for newcomers like me ?
Fred

Comment 7

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

+    nsString *string =  mNames.AppendElement(aName);
+    if (nsnull != string)
+      return PR_TRUE;
+    return PR_FALSE;

"return mNames.AppendElement(aName) != nsnull;"

+  PRBool hasPrefetch = (linkTypes.Contains(NS_LITERAL_STRING("prefetch")));

Lose unnecessary parentheses around linkTypes.Contain

+  if (mNamespaceURIs.InsertElementAt(count, aNamespaceURI) != nsnull) {
+    if (mNames.InsertElementAt(count, aName) != nsnull) {

You don't need these != nsnull tests here.

+        !aStyleSets.AppendElement(title ) == nsnull) {

Lose unnecessary space. Also, the test is just wrong; just !aStyleSets.AppendElement(title) is fine.

   ConvertDomainToArray(const nsAString& aDomain,
-                       nsStringArray* aArray);
+                       nsTArray<nsString> &aArray);

Leave this as a pointer, not a reference --- no need to change that here.

We don't want the change in dom/tests/mochitest/ajax/offline/Makefile.in.

-  nsAutoString word;
-  if ( mSuggestedWordIndex < mSuggestedWordList.Count())
+  nsString word;

Why did you change this to nsString? nsAutoString was better (a short string buffer is stack-allocated so short strings don't need a heap allocation).

-  nsAutoString word;
-  if ( mDictionaryIndex < mDictionaryList.Count())
+  nsString word;

Same here.

-  nsAutoString dictStr;
+  nsString dictStr;

Same here.

Although you can actually avoid all these temporary strings by moving ToNewUnicode up and doing it directly on mSuggestedWordList[mSuggestedWordIndex] (or dictList[i]) instead of using the temporary.

+      nsString valueString = cssValueArray[index];
       nsCOMPtr<nsIDOMElement> theElement = do_QueryInterface(aNode);
       res = SetCSSProperty(theElement, (nsIAtom *)cssPropertyArray.ElementAt(index),
                            valueString, aSuppressTransaction);

Here too you can get rid of the temporary string

+      nsString valueString = cssValueArray[index];
       res = RemoveCSSProperty(theElement, (nsIAtom *)cssPropertyArray.ElementAt(index), valueString,
                               aSuppressTransaction);

And here

+    nsString strp = mStyleSheetURLs[0];
+    RemoveOverrideStyleSheet(strp);

And here

-nsHTMLEditor::GetParentBlockTags(nsStringArray *aTagList, PRBool aGetLists)
-{
-  if (!aTagList) { return NS_ERROR_NULL_POINTER; }
-
+nsHTMLEditor::GetParentBlockTags(nsTArray<nsString> &aTagList, PRBool aGetLists)
+{

Don't change from a pointer to a reference

-      const PRUnichar *charset = (values->StringAt(numOfAttributes-3))->get();
-      const PRUnichar *source =  (values->StringAt(numOfAttributes-2))->get();
+      const PRUnichar *charset = (values->ElementAt(numOfAttributes-3)).get();
+      const PRUnichar *source =  (values->ElementAt(numOfAttributes-2)).get();

+      for (PRUint32 i = 0; i<(numOfAttributes-3); i++)

+        keyStr = (keys->ElementAt(i)).get();

Can you get rid of the unnecessary parentheses while you're here? They drive me crazy

-      nsAutoString srcStr((values->StringAt(numOfAttributes-2))->get());
+      nsAutoString srcStr((values->ElementAt(numOfAttributes-2)).get());

Can you make srcStr (both of them in this file) with const String&? I don't think you want the get() here either. This code is a mess :-(

-              nsString* currentCharset = values->StringAt(numOfAttributes-3);
-              if (!preferred.Equals(NS_LossyConvertUTF16toASCII(*currentCharset)) &&
+              nsString currentCharset = values->ElementAt(numOfAttributes-3);

this should be const nsString& too.

+        nsString fontName =  mFontName[font];

const nsString&

+    nsString list = gInvariantCharArray->ElementAt(aVariant);

const nsString&

+        nsTArray <nsString> keys(theAttrCount+4), values(theAttrCount+4);

Lose the unnecessary space.

-  NS_ASSERTION(mValues.Count() == mComments.Count(), "Arrays out of sync");
-  NS_ASSERTION(mValues.Count() == mImages.Count(),   "Arrays out of sync");
-  NS_ASSERTION(mValues.Count() == mStyles.Count(),   "Arrays out of sync");
+  NS_ASSERTION(mValues.Length() == mComments.Length(), "Arrays out of sync");
+  NS_ASSERTION(mValues.Length() == mImages.Length(),   "Arrays out of sync");
+  NS_ASSERTION(mValues.Length() == mStyles.Length(),   "Arrays out of sync");

Repeating this many times is yuck. How about defining a method in nsAutoCompleteSimpleResult, say void CheckInvariants(), which does these assertions? It can be inline in the class so it will compile away to nothing in non-DEBUG builds.

Although really, it would be better to define a struct containing 4 nsStrings and then have a single array of those structs. Someone should file a new bug for that.

-  nsStringArray     mArgs;
+  nsTArray<nsString>     mArgs;
   PRUint32          mState;

Fix indent.

 static void ParseSearchTermsFromQueries(const nsCOMArray<nsNavHistoryQuery>& aQueries,
-                                        nsTArray<nsStringArray*>* aTerms);
+                                        nsTArray<nsTArray<nsString>*>* aTerms);

This can probably be simplified to an nsTArray<nsTArray<nsString> >* ... but we probably should have a separate bug for that.

Otherwise good ... except that there are additional users of nsStringArray in mailnews a.k.a. "comm-central". Lots :-(.

http://mxr.mozilla.org/comm-central/search?string=nsstringarray

I guess we can do those in a separate bug, but deleting nsStringArray from nsVoidArray.h/.cpp will have to wait until the comm-central users are removed.

Comment 8

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

nsCStringArray::ParseString should be moved to be a method in nsReadableUtils.h. Then we can eliminate nsCStringArray in favour of nsTArray<nsCString>, although that should be done in a separate patch.

I think we should also eliminate nsVoidArray in favour of nsTArray<void*>. (Right, bsmedberg?) Another separate bug.

The only nsVoidArray.h type that is hard to remove is nsSmallVoidArray. For that we should probably define an nsSmallTPtrArray<T> which has the same API as nsTArray but is not a subclass of nsTArray.

Comment 9

[Benjamin Smedberg]

Yes to the above, though the ParseString method should perhaps be moved to nsStringAPI.h so that mailnews can end up using it from frozen-linkage code.

Comment 10

[Fred Jendrzejewski]

Attachment: replace - v2

I cleaned up as proposed.
* I am very unsure about my static_cast statements
* in layout/mathml/base/src/nsMathMLChar.cpp fontName ist used in a strange way. It isn't defined in some functions where it is used and it was defined without being used, so I removed it there.
* GetParentBlockTags is defined, but never used.

Comment 11

[Fred Jendrzejewski]

Removing nsStringArray from mailnews should be possible, there are less then 15
references in this folder.

Comment 12

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

-    word.Truncate();
+    *aSuggestedWord = static_cast<PRUnichar*>(nsMemory::Alloc(sizeof(PRUnichar*)));

This is wrong. You're allocating sizeof(PRUnichar*) bytes when you really want a PRUnichar. And you're not initializing that PRUnichar to zero. Instead just do ToNewUnicode(EmptyString()). This happens twice.

+      res = RemoveCSSProperty(theElement, (nsIAtom *)cssPropertyArray.ElementAt(index), cssValueArray[index],
                               aSuppressTransaction);

Please break at 80 columns

+inline void nsAutoCompleteSimpleResult::CheckInvariants()

I don't think this works. Just move the definition right into the class declaration.

(In reply to comment #10)
> I cleaned up as proposed.

Thanks!!! We're nearly done here :-)

> * I am very unsure about my static_cast statements

They look OK apart from the PRUnichar* issue above.

> * in layout/mathml/base/src/nsMathMLChar.cpp fontName ist used in a strange
> way. It isn't defined in some functions where it is used and it was defined
> without being used, so I removed it there.

Thanks.

> * GetParentBlockTags is defined, but never used.

Feel free to file another bug with a patch to remove it.

Comment 13

[Fred Jendrzejewski]

Attachment: replace - v3

Cleaned up the three issues.

Comment 14

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

Comment on attachment 349936 [details] [diff] [review]
replace - v3

Excellent, thanks. We'll land this for 1.9.2.

Comment 15

[Jesse Ruderman]

Fred has split off the following bugs:
* Bug 466613, Remove nsStringArray from mailnews
* Bug 466622, Replace nsCStringArray with nsTArray<nsCString>
* Bug 466626, nsStyleLinkElement::ParseLinkTypes should return a bitmask

Comment 16

[Fred Jendrzejewski]

Attachment: replace - v4

I accidentally removed NS_NewStringEnumerator from the header. This is cleaned up now.

Comment 17

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

I tried landing this but had to back out due to test failures on Mac. We crashed in reftests. We'll see what the other platforms report.

Comment 18

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

Attachment: v5

Updated patch with fixes for build bustage on Mac and Windows.

Comment 19

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

Attachment: v6

The problem was we changed i to unsigned here in nsMathMLOperators:
    for (PRUint32 i = gInvariantCharArray->Length()-1; i >= 0; --i) {
This doesn't work because i>=0 is always true. The fix is trivial, just leave i signed. I'm rerunning tests with that change. (Our testsuite rules!)

Comment 20

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

Crashtests and reftests pass on my Mac with that patch.

Comment 21

[[e-mail]]

(In reply to comment #19)
> The problem was we changed i to unsigned here in nsMathMLOperators:
>     for (PRUint32 i = gInvariantCharArray->Length()-1; i >= 0; --i) {
> This doesn't work because i>=0 is always true. The fix is trivial, just leave i signed.
for (PRUint32 i = gInvariantCharArray->Length(); i-- > 0; ) also works.
(BTW don't gcc/msvc complain these days about comparing unsigned to zero?)

Comment 22

[Dão Gottwald [:dao]]

This landed a few hours ago:

http://hg.mozilla.org/mozilla-central/rev/9001eaa33e8f

Comment 23

[Daniel Holbert [:dholbert]]

Attachment: followup patch: fix usages of mStyleSheetURLs.IndexOf in nsHTMLEditor.cpp

As mentioned in bug 466622 comment 20, there are some problematic uses of mStyleSheetURLs.IndexOf() in nsHTMLEditor.cpp resulting from this bug's nsStringArray --> nsTArray changeover.

This followup patch addresses these, though there may be other similar problems that should also be addressed...

Comment 24

[Daniel Holbert [:dholbert]]

Attachment: followup patch v2

This followup patch includes the previously-posted nsHTMLEditor.cpp fixes, plus a number of other fixes for issues that I found while going through this bug's landed patch.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 359186 [details] [diff] [review]
followup patch v2

Here's a summary of the changes made in followup patch v2.  I've tested to make sure this patch doesn't break the build, and it doesn't.

>diff --git a/content/base/src/nsDOMLists.cpp b/content/base/src/nsDOMLists.cpp
>   PRUint32 index = mNames.IndexOf(aName);
>-  if (index != PRUint32(-1)) {
>+  if (index != mNames.NoIndex) {

This is just a cleanliness / correctness fix -- nsTArray exposes the "NoIndex" value for the express purpose of being checked against IndexOf, AFAICT, so we should use it.

>diff --git a/content/xslt/src/xml/txDOM.h b/content/xslt/src/xml/txDOM.h

I reverted this bug's changes to the file "txDOM.h" (i.e. I reverted mNamespaces to be a nsStringArray again), because that file still included a bunch of nsStringArray-specific calls (e.g. Count, StringAt, AppendString, and IndexOf() checked against -1).

So in its existing state (before this followup-patch), that file should break any build that includes it. Apparently it's NPODB, so it wasn't breaking normal Firefox builds.  This patch reverts it to a state where it should compile.

>diff --git a/content/xslt/src/xml/txNodeDefinition.cpp b/content/xslt/src/xml/txNodeDefinition.cpp
>-      URIUtils::resolveHref(*baseUrls[--count], aURI, dest);
>+      URIUtils::resolveHref(baseUrls[--count], aURI, dest);

Here, baseURLs used to be a nsStringArray and is now a nsTArray<nsString>.  I think the "*" was necessary before in order to get a nsString& to pass into resolveHref, but it's no longer necessary, now that baseUrls[--count] already returns a nsString&.

I don't imagine that this compiles in its pre-followup-patch state, but it should now.

>diff --git a/editor/libeditor/html/nsHTMLEditor.cpp b/editor/libeditor/html/nsHTMLEditor.cpp

Same fixes as in previous followup patch -- we should check nsTArray::IndexOf against NoIndex, not against -1.

>diff --git a/toolkit/components/commandlines/src/nsCommandLine.cpp b/toolkit/components/commandlines/src/nsCommandLine.cpp
>-  NS_ENSURE_ARG_MAX(aEnd, mArgs.Length() - 1);
>+  NS_ENSURE_ARG_MAX(aEnd + 1, mArgs.Length());

This looked dangerous to me -- mArgs.Length() is now an unsigned value, so when mArgs.Length() returns 0, then mArgs.Length() - 1 evaluates to +infinity.  So in the case where mArgs.Length() == 0, the NS_ENSURE_ARG_MAX call would be completely useless (without this followup patch)

>diff --git a/widget/src/cocoa/nsFilePicker.mm b/widget/src/cocoa/nsFilePicker.mm
>--- a/widget/src/cocoa/nsFilePicker.mm
>+++ b/widget/src/cocoa/nsFilePicker.mm
>@@ -120,18 +120,18 @@ NSView* nsFilePicker::GetAccessoryView()
>-  PRInt32 numMenuItems = mTitles.Length();
>-  for (int i = 0; i < numMenuItems; i++) {
>+  PRUint32 numMenuItems = mTitles.Length();
>+  for (PRUint32 i = 0; i < numMenuItems; i++) {

This is just a fix to avoid unsigned <---> signed compile warnings.

>@@ -418,17 +418,17 @@ nsFilePicker::GenerateFilterList()
>-    for (PRInt32 loop = 0; PRInt32(loop < mFilters.Length()); loop++) {
>+    for (PRUint32 loop = 0; loop < mFilters.Length()); loop++) {

That mildly-buggy "PRInt32(" cast around the entire condition was added in this bug's patch -- I'm assuming it was intended to just go around the Length() call, but there's actually no reason for "loop" to be signed -- it can just be an unsigned value, and we avoid any unnecessary conversions.

Comment 26

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

>+    for (PRUint32 loop = 0; loop < mFilters.Length()); loop++) {

This looks like a syntax error to me

If txDOM.h isn't being built, we should remove it. Maybe that could be a followup bug.

Comment 27

[Daniel Holbert [:dholbert]]

Attachment: followup patch v3

> This looks like a syntax error to me

Oops :) Yeah -- that's in a mac-specific file, so my local Linux build process didn't catch it.  That's fixed in this version of the patch. (other  than that it's the same as prev. version)

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to comment #26)
> If txDOM.h isn't being built, we should remove it. Maybe that could be a
> followup bug.

I filed bug 475833 on that.

Comment 29

[Daniel Holbert [:dholbert]]

followup patch v3 (attachment 359398 [details] [diff] [review]) pushed:
http://hg.mozilla.org/mozilla-central/rev/70ae20e89604