1409227 - Reduce usage of nsMemory::Clone()

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

nsMemory::Clone() is mostly used to clone C strings and nsIDs. These can be replaced with alternatives that are more concise and easier to understand.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove needless duplication of the profile in nsProfiler::GetProfile()

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Replace nsMemory::Clone(s, strlen(s)+1) with moz_xstrdup()

They are equivalent -- both infallible, both requiring measuring the length of
the string -- but moz_xstrdup is much more readable. (One place deals with
16-bit strings and so uses NS_strdup instead, which is also infallible.)

The patch also removes some failure-path code that will never execute due to
the infallibility.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace nsMemory::Clone(s, sizeof(s)) with moz_xstrdup(s)

The new code is slightly less efficient because it requires measuring the
string length, but these strings are all short so it shouldn't matter.

Note that the case in DataToString() is a little different. The std::min() that
was there appears to be excessive caution -- this code is always printf'ing
some kind of number, so 32 chars should never be reached -- but it was bogus
anyway, because if 32 was exceeded then (a) we would have overflowed `buf`, and
(b) we'd be returning a non-null-terminated string.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Replace nsMemory::Clone(s, sizeof(s)) with moz_xstrdup(s)

The new code is slightly less efficient because it requires measuring the
string length, but these strings are all short so it shouldn't matter.

Note that the case in DataToString() is a little different. The std::min() that
was there appears to be excessive caution -- this code is always printf'ing
some kind of number, so 32 chars should never be reached -- but it was bogus
anyway, because if 32 was exceeded then (a) we would have overflowed `buf`, and
(b) we'd be returning a non-null-terminated string.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Replace nsMemory::Clone(id, sizeof(nsID)) with nsID::Clone(id)

This change requires introducing nsID::Clone(). Because it's infallible, the
patch also removes some redundant failure-handling code. (nsMemory::Clone() is
also infallible, so this code was redundant even before this change.)

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8919113 [details] [diff] [review]
(part 4) - Replace nsMemory::Clone(id, sizeof(nsID)) with nsID::Clone(id)

Review of attachment 8919113 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCComponents.cpp
@@ +127,2 @@
>      nsIID** array;
> +    *aArray = array = static_cast<nsIID**>(moz_xmalloc(2 * sizeof(nsIID*)));

While you are here, could you split this into two statements? (maybe initialize array with the call, then do |*aArray = array|)

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +84,2 @@
>      nsIID** array;
> +    *aArray = array = static_cast<nsIID**>(moz_xmalloc(2 * sizeof(nsIID*)));

Likewise.

@@ +86,1 @@
>      if (!array)

You can drop this null check.

::: xpcom/base/nsID.cpp
@@ +132,5 @@
> +
> +nsID*
> +nsID::Clone() const
> +{
> +  auto id = (nsID*)moz_xmalloc(sizeof(nsID));

static_cast here please.

@@ +136,5 @@
> +  auto id = (nsID*)moz_xmalloc(sizeof(nsID));
> +  id->m0 = m0;
> +  id->m1 = m1;
> +  id->m2 = m2;
> +  memcpy(id->m3, m3, 8);

You should use ArrayLength() here rather than the literal 8.

Can you use the default copy constructor rather than manually copying each field? eg id = *this or something.

::: xpcom/ds/nsVariant.cpp
@@ -438,5 @@
>    *aOutType = aInType;
>    *aOutCount = aInCount;
>    return NS_OK;
> -
> -bad:

Nice.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8919112 [details] [diff] [review]
(part 3) - Replace nsMemory::Clone(s, sizeof(s)) with moz_xstrdup(s)

Review of attachment 8919112 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems OK...but I wonder if it's not better to change the IDL definitions for the methods that return `string` and have them return `ACString` instead.  Doing so would be a more involved patch, but we'd be doing fewer things with `char*` and such, which seems like a win.  And with using XPCOM strings, we could get the best of both worlds: we'd have no nsMemory::Clone usage, but we'd also be able to derive the lengths of the strings to assign at compile time and avoid useless work.

WDYT?

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8919113 [details] [diff] [review]
(part 4) - Replace nsMemory::Clone(id, sizeof(nsID)) with nsID::Clone(id)

Review of attachment 8919113 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/ds/nsVariant.cpp
@@ -250,5 @@
>    NS_ASSERTION(aOutType, "bad param");
>    NS_ASSERTION(aOutCount, "bad param");
>    NS_ASSERTION(aOutValue, "bad param");
>  
>    uint32_t allocatedValueCount = 0;

Bonus points if you remove this as well.

Comment 9

[Eric Rahm [:erahm]]

Comment on attachment 8919108 [details] [diff] [review]
(part 2) - Replace nsMemory::Clone(s, strlen(s)+1) with moz_xstrdup()

Review of attachment 8919108 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks a lot better.

Comment 10

[Nicholas Nethercote [inactive]]

> This patch seems OK...but I wonder if it's not better to change the IDL
> definitions for the methods that return `string` and have them return
> `ACString` instead.

It's a good idea, and I've been doing some wstring/AString IDL changes recently anyway (e.g. bug 1407103).

But I'd prefer to do it as a follow-up, because there are some changes in this patch that don't involve that IDL. Sound ok?

Comment 11

[Nathan Froyd [:froydnj]]

Sure, I trust you to follow up. :)

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> Sure, I trust you to follow up. :)

Bug 1409598, for which you have the privilege of reviewing!

Comment 13

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eac4fd0f048122c0efd3da04ba0c97d4ea54ac7
Bug 1409227 (part 1) - Remove needless duplication of the profile in nsProfiler::GetProfile(). r=mstange.

https://hg.mozilla.org/integration/mozilla-inbound/rev/97d960b3912d889e5e6ddf3ffe913938c11593be
Bug 1409227 (part 2) - Replace nsMemory::Clone(s, strlen(s)+1) with moz_xstrdup(s). r=erahm.

https://hg.mozilla.org/integration/mozilla-inbound/rev/48349c3ea33dab78bb7579a03ac5dd785c6fe46d
Bug 1409227 (part 3) - Replace nsMemory::Clone(s, sizeof(s)) with moz_xstrdup(s). r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec3abaf3421e48bc1bb8f7c720075d5fa486e7a6
Bug 1409227 (part 4) - Replace nsMemory::Clone(id, sizeof(nsID)) with nsID::Clone(id). r=mccr8.

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/3eac4fd0f048
https://hg.mozilla.org/mozilla-central/rev/97d960b3912d
https://hg.mozilla.org/mozilla-central/rev/48349c3ea33d
https://hg.mozilla.org/mozilla-central/rev/ec3abaf3421e