Closed Bug 1409227 Opened 2 years ago Closed 2 years ago

Reduce usage of nsMemory::Clone()

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: njn, Assigned: njn)

Details

Attachments

(4 files, 1 obsolete file)

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.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8919107 - Flags: review?(mstange) → review+
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.
Attachment #8919108 - Flags: review?(erahm)
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.
Attachment #8919111 - Flags: review?(nfroyd)
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.
Attachment #8919112 - Flags: review?(nfroyd)
Attachment #8919111 - Attachment is obsolete: true
Attachment #8919111 - Flags: review?(nfroyd)
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.)
Attachment #8919113 - Flags: review?(continuation)
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.
Attachment #8919113 - Flags: review?(continuation) → review+
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 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 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.
Attachment #8919108 - Flags: review?(erahm) → 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.

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?
Flags: needinfo?(nfroyd)
Sure, I trust you to follow up. :)
Flags: needinfo?(nfroyd)
Attachment #8919112 - Flags: review?(nfroyd) → review+
(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!
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.
You need to log in before you can comment on or make changes to this bug.