Jesse Ruderman Reporter
	Description • 17 years ago

nsTArray<nsAutoString> is unsafe (see bug 427941 comment 0).  It should either be made safe (by changing nsTArray, nsAutoString, or both) or refuse to compile.

	David Baron :dbaron:
	Comment 1 • 17 years ago

Note that nsAutoString is not the only class that can't be moved with memmove.  nsAutoTArray is another example, and there are really many more -- including any class that puts itself into a linked list.  Normally one calls copy-constructors, and then destructors, when lacking specific knowledge that they don't need to be called.

	neil@parkwaycc.co.uk
	Comment 3 • 17 years ago

(In reply to comment #1)
> Normally one calls copy-constructors, and then destructors, when lacking
> specific knowledge that they don't need to be called.
So, nsTArrayElementTraits should define an array resize operator, which is then specialised to realloc for known safe types?

	Benjamin Smedberg
	Comment 4 • 17 years ago

Well, for moz2 we can use static checking to enforce that either

1) the element type is POD
2) nsTArrayElementTraits is specialized

Is there anything about this bug that we absolutely need to fix for 1.9?

	neil@parkwaycc.co.uk
	Comment 5 • 17 years ago

Oh, I just noticed roc said as much in bug 427602.

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 17 years ago

We probably want a wanted-next+ here.

	Samuel Sidler (old account; do not CC)
	Comment 7 • 16 years ago

Let's get this in 1.9.1 or later.

	Jeff Walden [:Waldo] Assignee
	Comment 8 • 15 years ago

Bug 503952 reminded me of this bug, and it seemed a little template specialization could win the day here.  We're already using it in JS, so it's not like we're going to break anyone who isn't already broken.  After some effort and dead-end work trying to do this in nsStringAPI.h, voila!  This patch works for me, as evidenced by the following with it applied, then with a little bit of change atop it:

host-6-230:~/moz/2 jwalden$ hg di -U 3
diff --git a/layout/build/nsLayoutModule.cpp b/layout/build/nsLayoutModule.cpp
--- a/layout/build/nsLayoutModule.cpp
+++ b/layout/build/nsLayoutModule.cpp
@@ -155,6 +155,8 @@ static NS_METHOD
 nsEditorControllerConstructor(nsISupports *aOuter, REFNSIID aIID,
                                             void **aResult)
 {
+  nsTArray<nsAutoString> a;
+
   nsresult rv;
   nsCOMPtr<nsIController> controller = do_CreateInstance("@mozilla.org/embedcomp/base-command-controller;1", &rv);
   if (NS_FAILED(rv)) return rv;
host-6-230:~/moz/2 jwalden$ make -s -C obj-i386-apple-darwin8.11.1/layout/build/
nsLayoutModule.cpp
/Users/jwalden/moz/2/content/base/src/nsXHTMLContentSerializer.h:114: warning: ‘virtual void nsXHTMLContentSerializer::SerializeAttributes(nsIContent*, nsIDOMElement*, nsAString_internal&, const nsAString_internal&, nsIAtom*, nsAString_internal&, PRUint32, PRBool)’ was hidden
/Users/jwalden/moz/2/content/base/src/nsHTMLContentSerializer.h:76: warning:   by ‘virtual void nsHTMLContentSerializer::SerializeAttributes(nsIContent*, nsIDOMElement*, nsAString_internal&, const nsAString_internal&, nsIAtom*, nsAString_internal&)’
../../dist/include/nsTArray.h: In member function ‘void nsTArray<E>::DestructRange(PRUint32, PRUint32) [with E = nsAutoString]’:
../../dist/include/nsTArray.h:663:   instantiated from ‘void nsTArray<E>::RemoveElementsAt(PRUint32, PRUint32) [with E = nsAutoString]’
../../dist/include/nsTArray.h:674:   instantiated from ‘void nsTArray<E>::Clear() [with E = nsAutoString]’
../../dist/include/nsTArray.h:267:   instantiated from ‘nsTArray<E>::~nsTArray() [with E = nsAutoString]’
/Users/jwalden/moz/2/layout/build/nsLayoutModule.cpp:158:   instantiated from here
../../dist/include/nsTArray.h:862: error: no matching function for call to ‘nsTArrayElementTraits<nsAutoString>::Destruct(nsAutoString*&)’
../../dist/include/nsTString.h:568: note: candidates are: static nsTArrayElementTraits<nsAutoString>::Dont_Instantiate_nsTArray_of<nsAutoString>* nsTArrayElementTraits<nsAutoString>::Destruct(nsTArrayElementTraits<nsAutoString>::Dont_Instantiate_nsTArray_of<nsAutoString>*)
make[1]: *** [nsLayoutModule.o] Error 1
make: *** [default] Error 2

Add a smidgen of cleverness and you even get an informative error message, with gcc at least (hopefully other compilers are also supported, if not similar hacks likely exist).  Win all around!

I don't want to waste the time to recompile right now, but I could tweak the patch and add:

      template<class A>
      class Instead_Use_nsTArray_of {
        int i;
      };

and use Instead_Use_nsTArray_of<nsTString_CharT> in arguments, to also identify the alternative.  If that sounds cool, I can easily make the change, just don't want to spend forever and a day recompiling it if it's not wanted.

	neil@parkwaycc.co.uk
	Comment 9 • 15 years ago

Comment on attachment 392420 [details] [diff] [review]
Patch

You can probably get away with incomplete types e.g.

>+      template<class A>
>+      struct Dont_Instantiate_nsTArray_of;

>+      static Dont_Instantiate_nsTArray_of<nsTAutoString_CharT> *
>+      Construct(Dont_Instantiate_nsTArray_of<nsTAutoString_CharT> e);
etc.

	Jeff Walden [:Waldo] Assignee
	Comment 10 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/9d3f9621b2f2
http://hg.mozilla.org/mozilla-central/rev/51f02ed639c9

Fixed in m-c.

	neil@parkwaycc.co.uk
	Comment 11 • 11 years ago

Bah, after all that this prevents you from using nsTArray<ns/*C*/String> outside of libxul, the problem being that nsStringAPI.h typedefs nsAuto/*C*/String to ns/*C*/String.

	neil@parkwaycc.co.uk
	Comment 12 • 11 years ago

Ignore me, I think someone changed an nsStringGlue.h to an nsString.h include.