1316556 - Remove zeroing allocation in class nsIPresShell

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julian Seward [:jseward]]

nsIPresShell::AllocateFrame and ::AllocateByObjectID implement a pool
allocator that zeroes out returned memory.  Unfortunately this falls foul
of GGC 6's lifetime-dead-store-elimination optimisations.  This detects
the memset(..) as occurring before the lifetime of the allocated object
begins, and so removes the call.  This in turn causes nsFrame and many
child classes to have uninitialised fields after construction, which
leads to multiple segfaults in short order.

For reasons I don't understand, this only seems to manifest in optimised
debug builds.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1316556-1-remove_nsIPresShell_h_memset_zero.cset

WIP patch.  Removes the offending memset calls and adds missing field
initialisations for about ten child classes of nsFrame.

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1316556-2-remove_nsIPresShell_h_memset_zero.cset

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1316556-6-remove_nsIPresShell_h_memset_zero.cset

Comment 4

[Julian Seward [:jseward]]

Removing the memsets in AllocateFrame and AllocateByObjectID and
adding instead missing field initialisations in various constructors
is pretty scary, because of the potential for missing one and hence
destabilising Gecko.  To counter that I've been run mochitest-plain
on Valgrind with the patch, which should detect any fields I missed.

Results are at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a7f2122ea4c386998e08f35e8f8ad076db73d37

20 out of the 40 runs are orange, but those are unrelated failures I think.

A "normal" try run is:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=37f123c64bcfc07ed24fce0973acfc10739d400f

Comment 5

[Julian Seward [:jseward]]

dbaron: who would be a good person to review this?

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8811607 [details] [diff] [review]
bug1316556-6-remove_nsIPresShell_h_memset_zero.cset

nsTextControlFrame.cpp:

Don't initialize mScrollEvent; it's a smart pointer.


nsIFrame.h:

No need to change nsIFrame::Cursor.  It's NS_STACK_CLASS, not allocated
this way, and filled in by the caller when it's used.

nsSubDocumentFrame.cpp:

Please omit mFrameLoader(); RefPtr doesn't need to be in the initializer
list for null-intitialization.

nsTextFrame.h:

Same for mTextRun here.

And please use mAscent(0) rather than mAscent().

nsMathMLmfracFrame.h:

Please use "0" explicitly for mLineThickness.


nsTableColFrame.h:

Pleae initialize mPrefPercent and mSpanPrefPercent as 0.0f, since they're
floats.

nsSliderFrame.cpp:

Again, 0.0f.


r=dbaron with that

I'll trust that you've found all the places that need changing. :-)

Comment 7

[Julian Seward [:jseward]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #6)

Thank you for the review.  Just one query:

> nsIFrame.h:
> 
> No need to change nsIFrame::Cursor.  It's NS_STACK_CLASS, not allocated
> this way, and filled in by the caller when it's used.

The constructor does seem to be necessary, though.  I removed it and
wound up with uninitialised value uses in mozilla::EventStateManager::UpdateCursor,
which you can see (unhelpfully without line numbers) in chunk 36 at

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aba7faa4ca139ee4146ba46d0b87cba80a214b5&selectedJob=31755815

I also remember seeing this locally when developing the patch, which is
why I included the constructor at all.

Are you happy to have that constructor in the patch, or do you prefer me
to chase this further?

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Julian Seward [:jseward] from comment #7)
> > nsIFrame.h:
> > 
> > No need to change nsIFrame::Cursor.  It's NS_STACK_CLASS, not allocated
> > this way, and filled in by the caller when it's used.
> 
> The constructor does seem to be necessary, though.  I removed it and
> wound up with uninitialised value uses in
> mozilla::EventStateManager::UpdateCursor,
> which you can see (unhelpfully without line numbers) in chunk 36 at
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5aba7faa4ca139ee4146ba46d0b87cba80a214b5&selectedJob=31755815

If I'm understanding this correctly, this is in a new test (mochitest-valgrind) that we're not currently running in automation -- or at least a test that you're modifying the way we run.

> I also remember seeing this locally when developing the patch, which is
> why I included the constructor at all.
> 
> Are you happy to have that constructor in the patch, or do you prefer me
> to chase this further?

I don't think it's related to any of the other work in this patch, so I think if we do it, it should be in a separate patch.

I'm also inclined to think it's not the right fix, and that we should fix whichever nsIFrame::GetCursor implementation is not filling in the mLoading member of the out parameter properly.  I suspect the error you're seeing there is a regression from https://hg.mozilla.org/mozilla-central/rev/5f3c1d227089 , which is relatively recent.  I think it's probably the two implementations in layout/generic/nsFrameSetFrame.cpp ; they're the only ones that don't call FillCursorInformationFromStyle, and they're also related to what that particular test is testing.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(separate patch in a separate bug)

Comment 10

[Julian Seward [:jseward]]

(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #9)
> (separate patch in a separate bug)

Done.  Bug 1320094.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this ready to land?

Comment 12

[Julian Seward [:jseward]]

I just started a hopefully-final set of test runs.  If nothing
bad shows up I'll land it in the morning.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db70aca765dc
Remove zeroing allocation in class nsIPresShell.  r=dbaron.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db70aca765dc