1408853 - Remove nsID::ToString()

Status: RESOLVED WONTFIX

(Core :: XPCOM, enhancement)

Description

[Nicholas Nethercote [inactive]]

nsID::ToString() uses heap allocation. nsId::ToProvidedString() uses stack allocation and is usually as or more convenient.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Use moz_xstrdup() in nsJSID

NS_Result() is infallible, but not obviously so. moz_xstrdup() is more
obviously infallible, so this patch replaces the former with the latter and
removes redundant failure checks.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsID::ToString()

nsID::ToProvidedString() is just as convenient, sometimes more so, and it
avoids a heap allocation.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove nsID::ToString()

nsID::ToProvidedString() is just as convenient, sometimes more so (esp. in
combination with nsIDToCString) and it avoids a heap allocation.

Comment 4

[Eric Rahm [:erahm]]

Comment on attachment 8918780 [details] [diff] [review]
(part 1) - Use moz_xstrdup() in nsJSID

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

I have a slight preference for just swapping in `strdup` and keeping fallibility. The code itself looks fine.

::: js/xpconnect/src/XPCJSID.cpp
@@ +55,5 @@
>  nsJSID::SetName(const char* name)
>  {
>      MOZ_ASSERT(!mName || mName == gNoString ,"name already set");
>      MOZ_ASSERT(name,"null name");
> +    mName = moz_xstrdup(name);

I'm inclined to just make this `strdup` and keep fallibility. Is there a better reason to make this infallible other than "it's technically not fallible already because `NS_strdup` is confusing?" Clearly this was *intended* to be fallible.

@@ +584,5 @@
>          NS_ENSURE_TRUE(registrar, NS_ERROR_FAILURE);
>  
>          nsCID* cid;
>          if (NS_FAILED(registrar->ContractIDToCID(str, &cid)))
>              return NS_ERROR_FAILURE;

It's weird to me that now we handle allocation failure here, but not on the following line.

Comment 5

[Eric Rahm [:erahm]]

Comment on attachment 8918789 [details] [diff] [review]
(part 2) - Remove nsID::ToString()

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

Looks good, just a question about options for the `*reinterpret_cast<char(*)[NSID_LENGTH]>` casts.

::: js/xpconnect/src/XPCJSID.cpp
@@ +81,5 @@
>          return NS_ERROR_NULL_POINTER;
>      }
>  
>      if (!mNumber) {
> +        mNumber = new char[NSID_LENGTH];

If we keep this it should use `moz_xmalloc` which would presumably match an eventual `free`.

@@ +82,5 @@
>      }
>  
>      if (!mNumber) {
> +        mNumber = new char[NSID_LENGTH];
> +        mID.ToProvidedString(*reinterpret_cast<char(*)[NSID_LENGTH]>(mNumber));

Oh dear. WDYT of:

> mNumber = moz_xstrdup(nsIDToCString(mID).get());

It's worse perf, but more readable. Otherwise I think we should add a helper function to hide the cast:

> nsID::ToProvidedString(const Span<char>& aStr) const {
>   MOZ_ASSERT(aStr.Length() >= 39);
>   ToProvidedString(...awful cast...);
> }
> // ...
> mID.ToProvidedString(MakeSpan(mNumber, NSID_LENGTH));

::: js/xpconnect/src/XPCWrappedJS.cpp
@@ +709,5 @@
>          XPC_LOG_ALWAYS(("interface name is %s", name));
>          if (name)
>              free(name);
> +        XPC_LOG_ALWAYS(("IID number is %s",
> +                        nsIDToCString(GetClass()->GetIID()).get()));

Nice cleanup, all of these are *much* better.

::: xpcom/ds/nsSupportsPrimitives.cpp
@@ +77,5 @@
>  {
>    NS_ASSERTION(aResult, "Bad pointer");
>  
>    if (mData) {
> +    *aResult = new char[NSID_LENGTH];

Same comment as above.

Comment 6

[Nicholas Nethercote [inactive]]

> I'm inclined to just make this `strdup` and keep fallibility. Is there a
> better reason to make this infallible other than "it's technically not
> fallible already because `NS_strdup` is confusing?" Clearly this was
> *intended* to be fallible.

Infallible is something of a default because it's simpler -- less code, fewer cases to worry about. The exception is of course for potentially large allocations, but these are not potentially large.

Comment 7

[Nicholas Nethercote [inactive]]

> If we keep this it should use `moz_xmalloc` which would presumably match an
> eventual `free`.

Good point.

> > +        mNumber = moz_xmalloc(NSID_LENGTH);
> > +        mID.ToProvidedString(*reinterpret_cast<char(*)[NSID_LENGTH]>(mNumber));
> 
> Oh dear. WDYT of:
> 
> > mNumber = moz_xstrdup(nsIDToCString(mID).get());
> 
> It's worse perf, but more readable. Otherwise I think we should add a helper
> function to hide the cast:
> 
> > nsID::ToProvidedString(const Span<char>& aStr) const {
> >   MOZ_ASSERT(aStr.Length() >= 39);
> >   ToProvidedString(...awful cast...);
> > }
> > // ...
> > mID.ToProvidedString(MakeSpan(mNumber, NSID_LENGTH));

Hmm, I don't particularly like either of those. I'll think about this a bit more.

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8918780 [details] [diff] [review]
(part 1) - Use moz_xstrdup() in nsJSID

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

Okay. Please update the comment to say "NS_strdup() is infallible..."

Comment 9

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Update_affecteds