1515419 - fixing ToNewCString (and ToNewUnicode as well) xpcom/string/nsReadableUtils.cpp

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nicklas Boman [:smurfd]]

Followup from https://bugzilla.mozilla.org/show_bug.cgi?id=1122740#c50

Quote from froydnj :
----
I see that on inspection, the allocation function it calls is AllocateStringCopy:

https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#30-35

which calls moz_xmalloc, which can't actually fail.  
---

Task : 
fixing ToNewCString (and ToNewUnicode as well) in xpcom/string/nsReadableUtils.cpp

Comment 1

[Eric Rahm [:erahm]]

It would be nice to preserve the "allocations are infallible by default" ethos while still providing an option for infallible allocations. We do this throughout the code base, for example when appending to strings [1] we provide two options:

> void Append(char_type aChar);
> MOZ_MUST_USE bool Append(char_type aChar, const fallible_t& aFallible);

I propose that we explicitly make `ToNewCString(aSrc)` infallible and add a variant that it is explicitly fallible `ToNewCString(aSrc, aFallible)`.

We would then update callers [2,3] that currently check the return value to use the fallible version. We would then do the same for `ToNewUnicode`.

Nathan, what do you think?

[1] https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/xpcom/string/nsTSubstring.h#569-571
[2] https://searchfox.org/mozilla-central/search?q=symbol:_Z12ToNewCStringRK12nsTSubstringIDsE&redirect=false
[3] https://searchfox.org/mozilla-central/search?q=symbol:_Z12ToNewCStringRK12nsTSubstringIcE&redirect=false

Comment 2

[Nathan Froyd [:froydnj]]

Yes, that seems better than what we currently do.

To be explicit, we'd want to change the moz_xmalloc uses to moz_malloc:

https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#34
https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#65
https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#133

so that the allocations are then able to actually fail.  (If you also wanted to rewrite AllocateStringCopy to be something more like:

template <class FromStringT>
inline typename FromStringT::char_type* AllocateStringCopy(const FromStringT& aSource) {
  typedef typename FromStringT::char_type char_type;
  return static_cast<char_type*>(moz_malloc((size_t(aSource.Length()) + 1) * sizeof(char_type)));
}

that'd be nice too.)

Then we'd want to rewrite ToNewCString to something like:

char* ToNewCString(const nsACString& aSource, const fallible_t& aFallible) {
  // Current code of ToNewCString
}

char* ToNewCString(const nsACString& aSource) {
  char* str = ToNewCString(aSource, mozilla::fallible);
  if (!str) {
    MOZ_CRASH("Unable to allocate memory");
  }
  return str;
}

And then audit all the callers of ToNewCString for places that actually check the return value in some way; for such places, we'd make them use the fallible variant.

Does that all make sense?

Comment 3

[Nicklas Boman [:smurfd]]

Thanks guys, yeah i think i follow. 
And im abit suprised that ToNewCString() was not used on more places.

One question i think pops up, but i will think some about it and needinfo any of you if i need to.
If i have something like [1] i would change that to : char* str = ToNewCString(aNewHash, mozilla::fallible);

[1] https://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#10560

Comment 4

[Eric Rahm [:erahm]]

(In reply to Nicklas Boman [:smurfd] from comment #3)
> Thanks guys, yeah i think i follow. 
> And im abit suprised that ToNewCString() was not used on more places.

We try to avoid using raw strings if we can, so hopefully the number of uses will go down over time :)

> One question i think pops up, but i will think some about it and needinfo
> any of you if i need to.
> If i have something like [1] i would change that to : char* str =
> ToNewCString(aNewHash, mozilla::fallible);

That's correct!

Comment 5

[Nicklas Boman [:smurfd]]

When you get back in next year ill need info you :)

Have some follow up questions.

1) I was not able to find any mentioning of moz_malloc in the codebase, but found this, so i used regular 'malloc' instead, thats allright i guess?
https://bugzilla.mozilla.org/show_bug.cgi?id=1138293

2) We wanted to change the moz_xmalloc at https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.cpp#133
I also thought i would do a fallible/infallible version of UTF8ToNewUnicode(const nsACString& aSource, uint32_t* aUTF16Count)
Or is that not needed?

3) Is there some tests that i can run before i submit a patch?

Comment 6

[Nicklas Boman [:smurfd]]

Happy new year :)
See Comment 5

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Nicklas Boman [:smurfd] from comment #5)
> 1) I was not able to find any mentioning of moz_malloc in the codebase, but
> found this, so i used regular 'malloc' instead, thats allright i guess?
> https://bugzilla.mozilla.org/show_bug.cgi?id=1138293

Plain malloc would be fine!

> 2) We wanted to change the moz_xmalloc at
> https://searchfox.org/mozilla-central/source/xpcom/string/nsReadableUtils.
> cpp#133
> I also thought i would do a fallible/infallible version of
> UTF8ToNewUnicode(const nsACString& aSource, uint32_t* aUTF16Count)
> Or is that not needed?

Adding a fallible/infallible split for UTF8ToNewUnicode would be great!

> 3) Is there some tests that i can run before i submit a patch?

I'm not sure if there are complete tests for all the code you're going to touch.  A good start would be:

  ./mach gtest Strings.*

(Note that you may need to backslash-escape the `*` character.)

But I see that only covers ToNewCString...and also includes a bunch of performance tests that you don't really care about.  Still, it's better than nothing, every if it might take a little while.

Comment 8

[Nicklas Boman [:smurfd]]

Attachment: Bug 1515419 - fixing ToNewCString (and ToNewUnicode as well) xpcom/string/nsReadableUtils.cpp r=froydnj

Adding a fallible variant of ToNewCString, ToNewUnicode and UTF8ToNewUnicode

Comment 9

[Nicklas Boman [:smurfd]]

Cool, seem to have been able to do something with Phabricator :)
So please let me know how that looks, if i missed something about that part...

Also, i was able to, after applying this patch run the
$ ./mach gtest String*

with all passes.

Comment 10

[Nicklas Boman [:smurfd]]

hmm seem to have mixed together 2 patches somehow.
Great :(

Comment 11

[Nicklas Boman [:smurfd]]

Sorry anout this, let me look at it and add a correct patch

Comment 12

[Nicklas Boman [:smurfd]]

Attachment: Bug 1515419 - fixing ToNewCString (and ToNewUnicode as well) xpcom/string/nsReadableUtils.cpp

Adding a fallible variant of ToNewCString, ToNewUnicode and UTF8ToNewUnicode

Comment 13

[Nicklas Boman [:smurfd]]

Now this time it worked out as i had planed :)
(arc diff --preview) is a good command, lets you see what you are about to put up.

Comment 14

[Nicklas Boman [:smurfd]]

I see reviewbot mentions 18 issues with the code (format issues).
i have run './mach clang-format' locally so i can easily 'arc diff' with these fixes.

I figure there might be some thoughts about the code, so the clang-format fixes will follow next patch :)
(hope that it is okay?)

Comment 15

[Nicklas Boman [:smurfd]]

Sorry for spamming, forgot to Need info. Was unsure whether or not to need info since you where tagged as a reviewer in Phabricator, Nathan.

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to Nicklas Boman [:smurfd] from comment #15)

Sorry for spamming, forgot to Need info. Was unsure whether or not to need info since you where tagged as a reviewer in Phabricator, Nathan.

Generally flagging someone for review in Phabricator is fine. I'm planning on doing your review this afternoon.

(In reply to Nicklas Boman [:smurfd] from comment #14)

I figure there might be some thoughts about the code, so the clang-format fixes will follow next patch :)
(hope that it is okay?)

That's fine!

Comment 17

[Nicklas Boman [:smurfd]]

Attachment: Bug 1515419 - fixing ToNewCString (and ToNewUnicode as well) xpcom/string/nsReadableUtils.cpp

Adding a fallible variant of ToNewCString, ToNewUnicode and UTF8ToNewUnicode

Comment 18

[Nicklas Boman [:smurfd]]

Attachment: Bug 1515419 - fixing ToNewCString (and ToNewUnicode as well) xpcom/string/nsReadableUtils.cpp r=froydnj

Adding a fallible variant of ToNewCString, ToNewUnicode and UTF8ToNewUnicode

Comment 19

[Nicklas Boman [:smurfd]]

With some luck its okay now.
Sometimes you do things like the big F in "A Fallible..." without thinking. Atleast i did it the same way all the time ;)

Will ask in #phabricator if there is some way to "amend" the Phabricator revision, especially when you have accepted it...

thanks for reviewing :)

Comment 20

[Nicklas Boman [:smurfd]]

Hi, ill need info you this, so it doesnt fall between the chairs.
afaik you where about to push it to try... and no rush!

Comment 21

[Nicklas Boman [:smurfd]]

Noticed today that this was waiting for me :( ... will look at it asap.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d1e6afac6d0b
fixing ToNewCString (and ToNewUnicode as well) xpcom/string/nsReadableUtils.cpp r=froydnj,necko-reviewers,valentin

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/d1e6afac6d0b