255792 - nsSmallVoidArray::ReplaceElementAt does not work in the same way as nsVoidArray::ReplaceElementAt

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616

nsSmallVoidArray::ReplaceElementAt does not work in the same way as
nsVoidArray::ReplaceElementAt. It should grow the array size if needed.
Patch coming.

Reproducible: Always
Steps to Reproduce:

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 156293 [details] [diff] [review]
proposed patch

I'm not sure who should review this.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

The reason it doesn't work the same way is that the API's defined before I
rewrote both classes had different semantics for ReplaceElementAt.  Yes it's
annoying.  You'd have to prove that no usage of ReplaceElementAt that depends on
the current behavior.

Also review the commentary in bug 90545.  There are a bunch of historical
weirdnesses in the array classes that I didn't dare remove.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I checked all the cases were nsSmallVoidArray is used
and ReplaceElementAt is not used at all.
http://lxr.mozilla.org/seamonkey/search?string=nsSmallVoidArray
http://lxr.mozilla.org/seamonkey/search?string=ReplaceElementAt

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

(In reply to comment #4)
> I checked all the cases were nsSmallVoidArray is used
> and ReplaceElementAt is not used at all.
> http://lxr.mozilla.org/seamonkey/search?string=nsSmallVoidArray
> http://lxr.mozilla.org/seamonkey/search?string=ReplaceElementAt

You're _probably_ right, but a you need to look at each ReplaceElementAt()
instance and determine that it's not operating on a SmallVoidArray.  You can't
just look at the two lists and say that without at least some code examination.
 If you did verify each one (and verified it can't get indirectly invoked or
invoked by something external like a plugin - and it is exported I think) then
we could consider changing it.  

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

hmm, of course I can't check all possible plugins etc.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Your other option is to create ReplaceElementAt2() or some equivalent, or
otherwise cause old uses (plugins for example) to use the old function, but cause
anything else to use the new one.

Changing API's once they're in the wild for a while is tough and dangerous. 
Typically you need to support the old one.  You could also create
nsNewSmallVoidArray/whatever that differs only in that one routine (and would
probably make nsSmallVoidArray a subclass of it that overrides
ReplaceElementAt()).  That's your safest bet - nothing would get the new API
without some source mods.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

So something like this perhaps.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 157331 [details] [diff] [review]
v2

review + with two caveats before checkin:

Add some comments in the .cpp file about how the new ReplaceElementAt differs
from the original one (a line or two).

Nit: Change

+  if (HasSingleChild())
+  {
+    if (aIndex == 0)
+    {
+      SetSingleChild(aElement);
+      return PR_TRUE;
+    }
+  }
+  else
+  {
+    vector = GetChildVector();
+  }

to

+  if (HasSingleChild())
+  {
+    if (aIndex == 0)
+    {
+      SetSingleChild(aElement);
+      return PR_TRUE;
+    }
+  }
+  vector = GetChildVector();

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2 + comments

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 157425 [details] [diff] [review]
v2 + comments

If this patch is ok, could someone check it in?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 157425 [details] [diff] [review]
v2 + comments

Looks ok from a code perspective, so review=+.

Now comes the question: _should_ this be done?	I agree certainly this is a
better API to start with.  However, given the problems (previously stated) in
rooting out older nsSmallVoidArray() uses, what do we gain by adding this?  How
many of the nsSmallVoidArray uses can/will be changed over?  And how much does
the better interface help compared to the added complexity of
yet-another-slightly-different-class?  While an annoyance, this behavioral
difference between nsVoidArray/etc and nsSmallVoidArray isn't a huge problem,
and alternatively could be dealt with by better comments/documentation/debugs.

We could mark nsSmallVoidArray as effectively obsolete, but with certain uses
that require it to stick around (due to plugins/extensions that might access
it).

Luckily, as reviewer, I won't be making that choice.  :-)

Comment 13

[Benjamin Smedberg]

Plug-ins and extensions are be a moot point. They are not allowed to use
non-frozen private classes like this. If they do, we are likely to break them
again and again (as happened when the string classes changed).

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

I'd really like to use the new version of the ReplaceElementAt for bug 51444.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Well, I'm ok with the code, and if it eases solving another bug, that's a good
reason to consider putting it in.  Using smallvoidarray2 avoids a lot of work
and worries about tracing through all the usages of this (direct and indirect).

Once this is committed, file a bug (or bugs) to convert over old uses.

Confirming, BTW, in case unconf status keeps it off people's radar.

You might want to add a comment to the effect that nsSmallVoidArray has been
superceded by nsSmallVoidArray2.

Comment 16

[Darin Fisher]

here are the places where nsSmallVoidArray is used today.  maybe it would be
better to audit each of these cases to determine if it would be possible to
change the implementation on nsSmallVoidArray::ReplaceElementAt instead of
introducing a new class.  This doesn't look like an unwieldy list of places to
check, right?

/content/base/src/nsDocument.cpp, line 160 -- nsSmallVoidArray mRadioButtons;
/content/base/src/nsDocument.h, line 562 -- nsSmallVoidArray mPresShells;
/content/html/content/src/nsHTMLFormElement.cpp, line 339 -- nsSmallVoidArray
mNotInElements; // Holds WEAK references
/content/svg/content/src/nsSVGValue.h, line 44 -- #include "nsGenericElement.h"
// for nsSmallVoidArray
/content/svg/content/src/nsSVGValue.h, line 81 -- nsSmallVoidArray mObservers;
/content/xul/document/src/nsXULDocument.cpp, line 342 -- nsSmallVoidArray
mListeners; // [OWNING] of BroadcastListener objects
/content/xul/document/src/nsXULDocument.cpp, line 806 -- // constructed the
nsSmallVoidArray object in-place.
/content/xul/document/src/nsXULDocument.cpp, line 807 --
entry->mListeners.~nsSmallVoidArray();
/content/xul/document/src/nsXULDocument.cpp, line 944 -- // N.B. placement new
to construct the nsSmallVoidArray object
/content/xul/document/src/nsXULDocument.cpp, line 946 -- new
(&entry->mListeners) nsSmallVoidArray();
/layout/html/base/src/nsReflowPath.h, line 227 -- nsSmallVoidArray mChildren;

Comment 17

[Boris Zbarsky [:bzbarsky]]

Note that all those users are in XP code.  So assuming non-gecko consumers can't
use nsSmallVoidArray (is that true?) just commenting out this function and
checking all places that fail to compile should work.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: change the nsSmallVoidArray::ReplaceElementAt,

I commented out the nsSmallVoidArray::ReplaceElementAt and the compilation
was still ok. So afaik nsSmallVoidArray::ReplaceElementAt is not
used at all. I think r=rjesup, but who would give the sr?

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sorry, this is for nsSmallVoidArray::ReplaceElementAt

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

So which one would be better 
https://bugzilla.mozilla.org/attachment.cgi?id=157425&action=view
or 
https://bugzilla.mozilla.org/attachment.cgi?id=159021&action=view ?
Who could make the decision?

Comment 21

[Doug Turner (:dougt)]

rjesup, can you review?

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 159021 [details] [diff] [review]
sorry, this is for nsSmallVoidArray::ReplaceElementAt

If we've verified it's not currently depended on, r=[e-mail]

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 159021 [details] [diff] [review]
sorry, this is for nsSmallVoidArray::ReplaceElementAt

alec, got time to sr this?

Comment 24

[Alec Flett]

Comment on attachment 159021 [details] [diff] [review]
sorry, this is for nsSmallVoidArray::ReplaceElementAt

sr=alecf - yes, lets do this way, not the other. I'll "approve" it. assuming
you guys have done the work - thanks darin for listing out the cases to make
that easier!

Comment 25

[Doug Turner (:dougt)]

rjesup, can you check this into the trunk?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

checked in.

Comment 27

[Alec Flett]

Comment on attachment 157425 [details] [diff] [review]
v2 + comments

clearing out old review