78388 - Fix uses of |new nsAutoString / nsCAutoString|

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

We have a bunch of nsAutoString explicitly allocated on the heap (in addition to
the ones that are member variables, see bug 26622):

http://lxr.mozilla.org/seamonkey/search?string=new+nsAutoString

We should fix these.  (They're probably also good places to look for other bad
string usage.)


Once we clean this up, we could also probably do a little funny math and make
nsAutoString assert if it's not allocated on the stack.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: quick hack for printf (not ready for assertion) when not allocated on stack

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

scc:  What's our policy here?  Do we want to prevent *all* heap-allocated
nsAutoStrings?  There are a number of places where people use heap-allocated
nsAutoStrings to fuse allocation (at least, I think that was the intent,
although it could have been ignorance) in objects that I think are shortlived.

Comment 3

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1) 2 <ns*RuleProcessor.*>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080819081634 SeaMonkey/2.0a1pre] (home, optim default) (W2Ksp4)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 334455 [details] [diff] [review]
(Bv1) 2 <ns*RuleProcessor.*>

Given that this should share string buffers, auto strings aren't useful here anymore, so this is fine.

However, you should add an:

NS_ASSERTION(hasAttr || mLanguage.IsEmpty(), "GetAttr that returns false should not make string non-empty"); before the final "if (hasAttr)".

r+sr=dbaron with that

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Bv1a) 2 <ns*RuleProcessor.*> [Checkin: Comment 15]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080828170525 Minefield/3.1a2pre] (home, debug default) (W2Ksp4)

Seems to run without triggering the assertion... :-)

Comment 6

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1) <nsDataObj.cpp>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1a2pre) Gecko/20080828200904 Minefield/3.1a2pre] (home, debug default) (W2Ksp4)

*|mDataFlavors| items: s/nsCAutoString/nsCString/g.
*Some related code/comment rewrites.
 *s/hangled/handled/

Iiuc, this will add an allocation+deallocation for each flavor.
I assume this D&D path/feature is not (perf) critical so this is acceptable.

I wonder if |mDataFlavors| itself really needs to be a pointer,
or if it could simply be a direct part of the |nsDataObj| class ?
<http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>

Comment 7

[Ere Maijala (slow)]

Comment on attachment 335972 [details] [diff] [review]
(Cv1) <nsDataObj.cpp>

+  for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) {

Í'd place the decrement operation to the third field of the for clause for readability. Other than that, r=me.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #7)
> (From update of attachment 335972 [details] [diff] [review])
> +  for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) {
> 
> Í'd place the decrement operation to the third field of the for clause for

That wouldn't work.

> readability. Other than that, r=me.

I could move it to the beginning of the loop, if you prefer. (I don't.)

Comment 9

[Ere Maijala (slow)]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 335972 [details] [diff] [review] [details])
> > +  for (PRUint32 idx = mDataEntryList.Length(); idx-- > 0;) {
> > 
> > Í'd place the decrement operation to the third field of the for clause for
> 
> That wouldn't work.

Please enlighten me as I'm failing miserably to see why this wouldn't work:

  for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)
>   for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {

Because |idx| is used inside the loop:
*that would "need" to add a |- 1| to the first field.
*even so, the last iteration, with the |0| value, would not happen.

Comment 11

[Ere Maijala (slow)]

(In reply to comment #10)
> (In reply to comment #9)
> >   for (PRUint32 idx = mDataEntryList.Length(); idx > 0; idx--) {
> 
> Because |idx| is used inside the loop:
> *that would "need" to add a |- 1| to the first field.
> *even so, the last iteration, with the |0| value, would not happen.

Sorry, I meant to make it:

  for (PRUint32 idx = mDataEntryList.Length() - 1; idx >= 0; idx--) {

Comment 12

[Ere Maijala (slow)]

Oops, it's unsigned. Never mind.

Comment 13

[Serge Gautherie (:sgautherie)]

(In reply to comment #12)
> Oops, it's unsigned. Never mind.

Yeah ;-> Then, is it all right with you to leave it as I wrote it ?

Comment 14

[Ere Maijala (slow)]

Yeah :)

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 335899 [details] [diff] [review]
(Bv1a) 2 <ns*RuleProcessor.*>
[Checkin: Comment 15]

http://hg.mozilla.org/mozilla-central/rev/837cacb67631

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 335972 [details] [diff] [review]
(Cv1) <nsDataObj.cpp>

> nsDataObj::~nsDataObj()

This method isn't perf sensitive and the arrays are small
so I see no reason to complicate the loops here to save
a few calls to Count()/Length().
I prefer the current code since it's more readable.
Feel free to move the "PRInt32" and add the spaces
in the first loop though.

sr=mats

Comment 17

[Serge Gautherie (:sgautherie)]

Attachment: (Cv1a) <nsDataObj.cpp> [Checkin: Comment 18]

Cv1, with comment 16 suggestion(s) :-|

Comment 18

[Serge Gautherie (:sgautherie)]

Comment on attachment 336745 [details] [diff] [review]
(Cv1a) <nsDataObj.cpp>
[Checkin: Comment 18]

http://hg.mozilla.org/mozilla-central/rev/3632d232b2c5

Comment 19

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> I wonder if |mDataFlavors| itself really needs to be a pointer,
> or if it could simply be a direct part of the |nsDataObj| class ?
> <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>

Ere, Mats, what about my question ?

Comment 20

[Ere Maijala (slow)]

(In reply to comment #19)
> (In reply to comment #6)
> > I wonder if |mDataFlavors| itself really needs to be a pointer,
> > or if it could simply be a direct part of the |nsDataObj| class ?
> > <http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsDataObj.h>
> 
> Ere, Mats, what about my question ?

Oh, right, sorry about that. I can't see any reason why it should be a pointer.

Comment 21

[Serge Gautherie (:sgautherie)]

(In reply to comment #20)
> I can't see any reason why it should be a pointer.

I filed bug 455557.

Comment 22

[Serge Gautherie (:sgautherie)]

Attachment: (Dv1) <ojiapitests.h> [Superseded by bug 485984]

Uncompiled...

Comment 23

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1) <nsWindow.*>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081012 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

(compiled; assuming run...)

Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto" string ?

Benjamin: in case the "Auto" is kept, should I do something else than the (3) comment I added ?

Comment 24

[Serge Gautherie (:sgautherie)]

Attachment: (Fv1) <xpcwrappednative.cpp>

Mike: I assume the "Auto" is/are really wanted as is ?

Benjamin: in case the "Auto" is kept, should I do something else than the (2)
comment I added ?

Comment 25

[Ere Maijala (slow)]

(In reply to comment #23)
> Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto"
> string ?

I'm not that good with the string classes, but I can't see the reason for using Auto string. It's not performance critical code though, and perhaps there is a reason for this (some specific use pattern or such) so another opinion would be appreciated.

Comment 26

[Serge Gautherie (:sgautherie)]

(In reply to comment #25)
> (In reply to comment #23)
> > Ere, Mats: would it be an (unwanted) pref regression to not use an "Auto"
> > string ?
> 
> I'm not that good with the string classes, but I can't see the reason for using
> Auto string.

http://mxr.mozilla.org/mozilla-central/search?string=sIMECompUnicode&case=on&find=%2Fwidget%2Fsrc%2Fwindows%2FnsWindow%5C

(afaict) The only reason is that there is 2 |Truncate()| and the memory space is reused.

Considering code like
7460     PRInt32 len = sIMECompUnicode->Length();
7461     if (len > IME_MAX_CHAR_POS)
I wonder if |char sIMECompUnicode[64]| would not even do the job !?

Comment 27

[Ere Maijala (slow)]

(In reply to comment #26)
> I wonder if |char sIMECompUnicode[64]| would not even do the job !?

Or |char sIMECompUnicode[IME_MAX_CHAR_POS + 1]| if it's really a string with terminating \0. I think IME isn't performance critical though, so perhaps it isn't worth it.

Comment 28

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1a) <nsWindow.*>

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.1b2pre) Gecko/20081012 SeaMonkey/2.0a2pre] (home, optim default) (W2Ksp4)

(compiled, but not run)

NB: I would have done it with "char []", but the string being Unicode would make it "needlessly" more complicated, so I concur with comment 28.

Ftr, ImmGetCompositionString() documentation is at
http://msdn.microsoft.com/en-us/library/ms776186(VS.85).aspx

Comment 29

[Serge Gautherie (:sgautherie)]

Attachment: (Ev1b) <nsWindow.*> [Checkin: See comment 32]

Ev1a, with 2 "nits".

Comment 30

[Ere Maijala (slow)]

Comment on attachment 343415 [details] [diff] [review]
(Ev1b) <nsWindow.*>
[Checkin: See comment 32]

It could have been a wide char array, but I like this better.

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 343415 [details] [diff] [review]
(Ev1b) <nsWindow.*>
[Checkin: See comment 32]


sr=mats with the following nits fixed:

> -  NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null");
> +nsWindow::HandleTextEvent(HIMC hIMEContext, PRBool aCheckAttr)
> +{
> +  if (NS_UNLIKELY(!sIMECompUnicode)) {
> +    NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null");
> +    return;
> +  }

I think the current code is more readable and I don't see a compelling
reason to change it.

> nsWindow::GetTextRangeList
> -  NS_ASSERTION(sIMECompUnicode, "sIMECompUnicode is null");

same

> +  if (NS_UNLIKELY(lRtn < 0 ||
> +                  !EnsureStringLength(*sIMECompUnicode, (lRtn / sizeof(WCHAR)) + 1)))

This line is longer than 80 chars. (You can just drop the NS_UNLIKELY,
this code isn't perf sensitive).

> -      PRUint32 i = 0;
> -      for (i = 0; i < sIMECompUnicode->Length(); i++) {
> -        if (PT_IN_RECT(*ptPos, sIMECompCharPos[i]))
> -          break;
> +      PRUint32 i = 0, len = sIMECompUnicode->Length();
> +      while (i < len && !PT_IN_RECT(*ptPos, sIMECompCharPos[i])) {
> +        ++i;

I prefer the old loop construct which I find easier to read.
Feel free to lift out the Length() call to a separate line if you want:
  PRUint32 len = sIMECompUnicode->Length();
but 'len' is guaranteed to be <= IME_MAX_CHAR_POS (64) at this point
so I don't think it's a perf problem to leave it as is.

Comment 32

[Serge Gautherie (:sgautherie)]

Comment on attachment 343415 [details] [diff] [review]
(Ev1b) <nsWindow.*>
[Checkin: See comment 32]

http://hg.mozilla.org/mozilla-central/rev/039a107a3cec
with comment 31 suggestion(s).

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 342828 [details] [diff] [review]
(Fv1) <xpcwrappednative.cpp>

This is rotted, but the comment in the first chunk seems vacuous (it duplicates the content of the preceding and more detailed comment) and the second one doesn't mean anything to me, so it's probably not clear to others reading the code either!