Closed Bug 1312344 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

Details

Attachments

(1 file)

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.
Minimal fix.  Gives explicit initialisation for the classes
nsContentSink and nsXMLContentSink.
Attachment #8803784 - Flags: review?(nfroyd)
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.
Attachment #8803784 - Flags: review?(nfroyd) → review+
Component: Build Config → DOM
(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.
Pushed by jseward@mozilla.com:
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.
https://hg.mozilla.org/mozilla-central/rev/990964802af8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Blocks: 1316555
Duplicate of this bug: 1331363
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.