Closed
Bug 1316556
Opened 8 years ago
Closed 8 years ago
Remove zeroing allocation in class nsIPresShell
Categories
(Core :: Layout, defect)
Core
Layout
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)
(deleted),
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
WIP patch. Removes the offending memset calls and adds missing field
initialisations for about ten child classes of nsFrame.
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8809348 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8809849 -
Attachment is obsolete: true
Assignee | ||
Comment 4•8 years ago
|
||
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
Assignee | ||
Comment 5•8 years ago
|
||
dbaron: who would be a good person to review this?
Flags: needinfo?(dbaron)
Comment 6•8 years ago
|
||
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+
Updated•8 years ago
|
Assignee: nobody → jseward
Assignee | ||
Comment 7•8 years ago
|
||
(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)
Comment 8•8 years ago
|
||
(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)
Comment 9•8 years ago
|
||
(separate patch in a separate bug)
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #9)
> (separate patch in a separate bug)
Done. Bug 1320094.
Assignee | ||
Comment 12•8 years ago
|
||
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)
Comment 13•8 years ago
|
||
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/db70aca765dc
Remove zeroing allocation in class nsIPresShell. r=dbaron.
Comment 14•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•8 years ago
|
status-firefox52:
affected → ---
Updated•8 years ago
|
Depends on: CVE-2017-7769
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•