use non-null-checking operator new in xpcom/

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(1 attachment)

The standard placement new function is declared to not throw, which
means that, per spec, a null check on its result is required.  There are
a number of places throughout xpcom/ where we know that we are passing
non-null pointers to placement new (and receiving them as a return
value), and we are therefore doing useless work performing these null
checks.

Therefore, we should be using an operator new overload that doesn't
require the null check.  MFBT has just such an overload, so use that.
Comment on attachment 8781177 [details] [diff] [review]
use non-null-checking operator new in xpcom/

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

This looks good, but you're not actually removing any null checks. Maybe update the commit comment?
Attachment #8781177 - Flags: review?(erahm) → review+
(In reply to Eric Rahm [:erahm] from comment #2)
> This looks good, but you're not actually removing any null checks. Maybe
> update the commit comment?

The null checks are implicitly generated by the compiler prior to the patch.
(In reply to Nathan Froyd [:froydnj] from comment #3)
> (In reply to Eric Rahm [:erahm] from comment #2)
> > This looks good, but you're not actually removing any null checks. Maybe
> > update the commit comment?
> 
> The null checks are implicitly generated by the compiler prior to the patch.

Ah ok, reword with that language?
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8d33a975228
use non-null-checked operator new in xpcom/; r=erahm
Backed out because it might have caused 32-bit linux debug crashtest assertions like https://treeherder.mozilla.org/logviewer.html#?job_id=33957663&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f52630cfbe8
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57753be3e421
use non-null-checked operator new in xpcom/; r=erahm
That landing has the nsTArray parts removed, which are probably the most beneficial part of the patch, but also the part that seems to cause crashiness. :(
https://hg.mozilla.org/mozilla-central/rev/57753be3e421
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.