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.
Created attachment 8803784 [details] [diff] [review] bug1232696-2-minimal.cset 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+
(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 email@example.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.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.