Closed Bug 1316556 Opened 3 years ago Closed 3 years ago

Remove zeroing allocation in class nsIPresShell

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

(Depends on 4 open bugs)

Details

Attachments

(1 file, 2 obsolete files)

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.
WIP patch.  Removes the offending memset calls and adds missing field
initialisations for about ten child classes of nsFrame.
Attachment #8809348 - Attachment is obsolete: true
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
dbaron: who would be a good person to review this?
Flags: needinfo?(dbaron)
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. :-)
Flags: needinfo?(dbaron)
Attachment #8811607 - Flags: review+
Assignee: nobody → 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?
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
(separate patch in a separate bug)
Depends on: 1320094
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #9)
> (separate patch in a separate bug)

Done.  Bug 1320094.
No longer depends on: 1320094
Is this ready to land?
Flags: needinfo?(jseward)
I just started a hopefully-final set of test runs.  If nothing
bad shows up I'll land it in the morning.
Flags: needinfo?(jseward)
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db70aca765dc
Remove zeroing allocation in class nsIPresShell.  r=dbaron.
https://hg.mozilla.org/mozilla-central/rev/db70aca765dc
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Depends on: 1349816
Depends on: 1361499
Depends on: 1361509
Depends on: 1361513
Depends on: 1361517
Depends on: 1361580
Depends on: 1361589
Depends on: 1361590
Depends on: 1361591
Depends on: 1361593
Depends on: 1361596
Depends on: 1361597
Depends on: 1361599
Depends on: 1361604
Depends on: 1361612
Depends on: 1361647
Depends on: 1361651
Depends on: 1361656
Depends on: 1361659
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.