1312344 - Remove just enough NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW uses to stop segfaulting for GCC 6 -O2 builds

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Julian Seward [:jseward]]

This is split out from bug 1232696; see that for further details.

It appears that NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW interacts badly
with GCC 6's new Lifetime-DSE optimization, in that the optimisation
in some cases removes the zeroing, resulting in many/all of the class
fields being undefined.

Removing NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW will be some work, but
fortunately it appears that only a small subset of them trigger this
behaviour, at least for GCC 6 at -O2.  This bug fixes those.  Bug 1232696
tracks the removal of the rest.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1232696-2-minimal.cset

Minimal fix.  Gives explicit initialisation for the classes
nsContentSink and nsXMLContentSink.

Comment 2

[Julian Seward [:jseward]]

Try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9235dd969bc&duplicate_jobs=visible

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8803784 [details] [diff] [review]
bug1232696-2-minimal.cset

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

::: dom/base/nsContentSink.h
@@ -267,5 @@
>  
> -private:
> -  // People shouldn't be allocating this class directly.  All subclasses should
> -  // be allocated using a zeroing operator new.
> -  void* operator new(size_t sz) CPP_THROW_NEW;  // Not to be implemented

Removing this makes me a little nervous, but I guess since this class already has a protected destructor and pure virtual methods, none of the subclasses are going to be instantiating it directly anyway.

Comment 4

[Julian Seward [:jseward]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> Removing this makes me a little nervous, [..]

Yes, fair comment, and it made me nervous too.  What I can say is
that, with the patch applied, I ran all of mochitests on valgrind
and didn't see any (new) complaints of uninitialisedness.  So that
makes me fairly confident that it is OK.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/990964802af8
Remove just enough NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW uses to stop segfaulting for GCC 6 -O2 builds.  r=froydnj.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/990964802af8