Various allocation fix-ups

RESOLVED FIXED in Firefox 63

Status

()

enhancement
RESOLVED FIXED
9 months ago
9 months ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(8 attachments)

Assignee

Description

9 months ago
This is mostly about removing unnecessary checks after infallible allocations, but I have a few other clean-ups too.
Assignee

Comment 1

9 months ago
It's unused, and MFBT has MOZ_ALIGNOF.
Attachment #9004463 - Flags: review?(mh+mozilla)
Assignee

Comment 2

9 months ago
They appear to be unused; jprof builds without them. Also jprof.h is included
prior to their definition in contradiction to the comment above them.
Attachment #9004464 - Flags: review?(rjesup)
Assignee

Comment 3

9 months ago
There are surprisingly many of them.

(Plus a couple of unnecessary checks after `new` calls that were nearby.)
Attachment #9004465 - Flags: review?(mh+mozilla)
Assignee

Comment 5

9 months ago
I tried moving these into mozalloc but the compiler didn't like having two
functions with these signatures:

 MFBT_API char* moz_xstrdup(const char* str) MOZ_ALLOCATOR;
 MFBT_API char16_t* moz_xstrdup(const char16_t* str) MOZ_ALLOCATOR;

And likewise for moz_xstrndup(). And the 8-bit and 16-bit versions need to have
the same names because there is code that relies on that.
Attachment #9004467 - Flags: review?(mh+mozilla)
Assignee

Comment 6

9 months ago
The 'x' in the new name makes it clearer that it's infallible.
Attachment #9004468 - Flags: review?(mh+mozilla)
Attachment #9004464 - Flags: review?(rjesup) → review+
Attachment #9004463 - Flags: review?(mh+mozilla) → review+
Comment on attachment 9004465 [details] [diff] [review]
Remove unnecessary checks after moz_xmalloc() calls

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

There is an interesting underlying question here, though, whether some of those code paths ought to actually try to recover from allocation failure, as in replacing moz_xmalloc with malloc, rather than removing the failure handling. It's fine to do the latter, but maybe a note to related module owners that maybe they intended to gracefully handle fallability but misused moz_xmalloc and may want to fix that.

::: extensions/spellcheck/hunspell/glue/mozHunspell.cpp
@@ +470,5 @@
>    *aSuggestionCount = static_cast<uint32_t>(suggestions.size());
>  
>    if (*aSuggestionCount) {
>      *aSuggestions  = (char16_t **)moz_xmalloc(*aSuggestionCount * sizeof(char16_t *));
> +    uint32_t index = 0;

This is where mozreview or phabricator would make a significant difference when reviewing...
Attachment #9004465 - Flags: review?(mh+mozilla) → review+
Attachment #9004466 - Flags: review?(mh+mozilla) → review+
Attachment #9004467 - Flags: review?(mh+mozilla) → review+
Attachment #9004468 - Flags: review?(mh+mozilla) → review+
Comment on attachment 9004465 [details] [diff] [review]
Remove unnecessary checks after moz_xmalloc() calls

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

::: docshell/base/nsDocShell.cpp
@@ +14312,5 @@
>    NS_ENSURE_ARG_POINTER(aMatrix);
>    *aMatrix = nullptr;
>  
>    if (mColorMatrix) {
>      *aMatrix = (float*)moz_xmalloc(20 * sizeof(float));

here and in a few other places, there's a moz_xmalloc/memcpy sequence that could be replaced with moz_xmemdup.
Attachment #9004469 - Flags: review?(mh+mozilla) → review+

Comment 10

9 months ago
Pushed by nnethercote@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa3a17103b6
Remove NS_ALIGNMENT_OF. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5734413c06f
Remove moz_x{malloc,free} in jprof. r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/8afde49af9c1
Remove unnecessary checks after moz_xmalloc() calls. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/c330fb64bcd0
Remove unnecessary checks after moz_xrealloc() calls. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ccf1492e71
Rename NS_str{,}dup and remove unnecessary checks after calls to them. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/8257797fd2d8
Rename nsMemory::Clone() and remove unnecessary checks after it. r=glandium
https://hg.mozilla.org/integration/mozilla-inbound/rev/74318e0ebe1e
Remove an unnecessary NS_FREE_XPCOM_ISUPPORTS_POINTER_ARRAY call. r=glandium
Assignee

Comment 11

9 months ago
jorgk, I haven't compiled or tested this but it should be close to what's
needed.
Attachment #9004762 - Flags: review?(jorgk)

Comment 12

9 months ago
Thanks Nick, I'll do what is needed when the platform changes hit M-C.

Comment 14

9 months ago
Comment on attachment 9004762 [details] [diff] [review]
Update Thunderbird for function renamings

Thanks again, compiles for me :-)
Attachment #9004762 - Flags: review?(jorgk) → review+

Comment 15

9 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/310a088dcdec
Update Thunderbird for function renamings. r=jorgk DONTBUILD
You need to log in before you can comment on or make changes to this bug.