servo: Merge #18732 - Revert #18668 - Add mprotect diagnostics for HashMap crash (except for round-up-to-page behavior) (from bholley:revert_mprotect_diagnostics); r=bholley

These diagnostics have served their purpose, time to remove them.

https://bugzilla.mozilla.org/show_bug.cgi?id=1403397

Source-Repo: https://github.com/servo/servo
Source-Revision: 0b69887387567d1bbaa93fe874014a925ef9fe05

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 048d7a45ecf6d963209a252d4dbde30a5e890489
This commit is contained in:
Bobby Holley 2017-10-03 14:45:33 -05:00
parent c61872d2ba
commit ff4aa007bc
17 changed files with 9 additions and 448 deletions

2
servo/Cargo.lock generated
View File

@ -1061,7 +1061,6 @@ dependencies = [
"atomic_refcell 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)",
"cssparser 0.21.2 (registry+https://github.com/rust-lang/crates.io-index)",
"env_logger 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",
"hashglobe 0.1.0",
"libc 0.2.23 (registry+https://github.com/rust-lang/crates.io-index)",
"log 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)",
"malloc_size_of 0.0.1",
@ -3243,7 +3242,6 @@ dependencies = [
"env_logger 0.4.3 (registry+https://github.com/rust-lang/crates.io-index)",
"euclid 0.15.3 (registry+https://github.com/rust-lang/crates.io-index)",
"geckoservo 0.0.1",
"hashglobe 0.1.0",
"libc 0.2.23 (registry+https://github.com/rust-lang/crates.io-index)",
"log 0.3.8 (registry+https://github.com/rust-lang/crates.io-index)",
"malloc_size_of 0.0.1",

View File

@ -1027,12 +1027,6 @@ impl<K, V, S> HashMap<K, V, S>
self.table.size()
}
/// Access to the raw buffer backing this hashmap.
pub fn raw_buffer(&self) -> (*const (), usize) {
assert!(self.raw_capacity() != 0);
self.table.raw_buffer()
}
/// Returns true if the map contains no elements.
///
/// # Examples

View File

@ -13,7 +13,6 @@ extern crate heapsize;
pub mod alloc;
pub mod hash_map;
pub mod hash_set;
pub mod protected;
mod shim;
mod table;
@ -52,6 +51,3 @@ impl fmt::Display for FailedAllocationError {
self.reason.fmt(f)
}
}
// The size of memory pages on this system. Set when initializing geckolib.
pub static SYSTEM_PAGE_SIZE: ::std::sync::atomic::AtomicUsize = ::std::sync::atomic::ATOMIC_USIZE_INIT;

View File

@ -1,235 +0,0 @@
use hash_map::{Entry, HashMap, Iter, IterMut, Keys, RandomState, Values};
use std::borrow::Borrow;
use std::hash::{BuildHasher, Hash};
use FailedAllocationError;
#[derive(Clone, Debug)]
pub struct ProtectedHashMap<K, V, S = RandomState>
where K: Eq + Hash,
S: BuildHasher
{
map: HashMap<K, V, S>,
readonly: bool,
}
impl<K: Hash + Eq, V, S: BuildHasher> ProtectedHashMap<K, V, S>
where K: Eq + Hash,
S: BuildHasher
{
#[inline(always)]
pub fn inner(&self) -> &HashMap<K, V, S> {
&self.map
}
#[inline(always)]
pub fn begin_mutation(&mut self) {
assert!(self.readonly);
self.unprotect();
self.readonly = false;
}
#[inline(always)]
pub fn end_mutation(&mut self) {
assert!(!self.readonly);
self.protect();
self.readonly = true;
}
#[inline(always)]
pub fn with_hasher(hash_builder: S) -> Self {
Self {
map: HashMap::<K, V, S>::with_hasher(hash_builder),
readonly: true,
}
}
#[inline(always)]
pub fn len(&self) -> usize {
self.map.len()
}
#[inline(always)]
pub fn is_empty(&self) -> bool {
self.map.is_empty()
}
#[inline(always)]
pub fn contains_key<Q: ?Sized>(&self, k: &Q) -> bool
where K: Borrow<Q>,
Q: Hash + Eq
{
self.map.contains_key(k)
}
#[inline(always)]
pub fn keys(&self) -> Keys<K, V> {
self.map.keys()
}
#[inline(always)]
pub fn values(&self) -> Values<K, V> {
self.map.values()
}
#[inline(always)]
pub fn get<Q: ?Sized>(&self, k: &Q) -> Option<&V>
where K: Borrow<Q>,
Q: Hash + Eq
{
self.map.get(k)
}
#[inline(always)]
pub fn iter(&self) -> Iter<K, V> {
self.map.iter()
}
#[inline(always)]
pub fn iter_mut(&mut self) -> IterMut<K, V> {
assert!(!self.readonly);
self.map.iter_mut()
}
#[inline(always)]
pub fn entry(&mut self, key: K) -> Entry<K, V> {
assert!(!self.readonly);
self.map.entry(key)
}
#[inline(always)]
pub fn try_entry(&mut self, key: K) -> Result<Entry<K, V>, FailedAllocationError> {
assert!(!self.readonly);
self.map.try_entry(key)
}
#[inline(always)]
pub fn insert(&mut self, k: K, v: V) -> Option<V> {
assert!(!self.readonly);
self.map.insert(k, v)
}
#[inline(always)]
pub fn try_insert(&mut self, k: K, v: V) -> Result<Option<V>, FailedAllocationError> {
assert!(!self.readonly);
self.map.try_insert(k, v)
}
#[inline(always)]
pub fn remove<Q: ?Sized>(&mut self, k: &Q) -> Option<V>
where K: Borrow<Q>,
Q: Hash + Eq
{
assert!(!self.readonly);
self.map.remove(k)
}
#[inline(always)]
pub fn clear(&mut self) where K: 'static, V: 'static {
// We handle scoped mutations for the caller here, since callsites that
// invoke clear() don't benefit from the coalescing we do around insertion.
self.begin_mutation();
self.map.clear();
self.end_mutation();
}
fn protect(&mut self) {
if self.map.capacity() == 0 {
return;
}
let buff = self.map.raw_buffer();
if buff.0 as usize % ::SYSTEM_PAGE_SIZE.load(::std::sync::atomic::Ordering::Relaxed) != 0 {
// Safely handle weird allocators like ASAN that return
// non-page-aligned buffers to page-sized allocations.
return;
}
unsafe {
Gecko_ProtectBuffer(buff.0 as *mut _, buff.1);
}
}
fn unprotect(&mut self) {
if self.map.capacity() == 0 {
return;
}
let buff = self.map.raw_buffer();
if buff.0 as usize % ::SYSTEM_PAGE_SIZE.load(::std::sync::atomic::Ordering::Relaxed) != 0 {
// Safely handle weird allocators like ASAN that return
// non-page-aligned buffers to page-sized allocations.
return;
}
unsafe {
Gecko_UnprotectBuffer(buff.0 as *mut _, buff.1);
}
}
}
impl<K, V> ProtectedHashMap<K, V, RandomState>
where K: Eq + Hash,
{
pub fn new() -> Self {
Self {
map: HashMap::new(),
readonly: true,
}
}
pub fn with_capacity(capacity: usize) -> Self {
let mut result = Self {
map: HashMap::with_capacity(capacity),
readonly: true,
};
result.protect();
result
}
}
impl<K, V, S> PartialEq for ProtectedHashMap<K, V, S>
where K: Eq + Hash,
V: PartialEq,
S: BuildHasher
{
fn eq(&self, other: &Self) -> bool {
self.map.eq(&other.map)
}
}
impl<K, V, S> Eq for ProtectedHashMap<K, V, S>
where K: Eq + Hash,
V: Eq,
S: BuildHasher
{
}
impl<K, V, S> Default for ProtectedHashMap<K, V, S>
where K: Eq + Hash,
S: BuildHasher + Default
{
fn default() -> Self {
Self {
map: HashMap::default(),
readonly: true,
}
}
}
impl<K: Hash + Eq, V, S: BuildHasher> Drop for ProtectedHashMap<K, V, S>
where K: Eq + Hash,
S: BuildHasher
{
fn drop(&mut self) {
debug_assert!(self.readonly, "Dropped while mutating");
self.unprotect();
}
}
// Manually declare the FFI functions since we don't depend on the crate with
// the bindings.
extern "C" {
pub fn Gecko_ProtectBuffer(buffer: *mut ::std::os::raw::c_void,
size: usize);
}
extern "C" {
pub fn Gecko_UnprotectBuffer(buffer: *mut ::std::os::raw::c_void,
size: usize);
}

View File

@ -777,7 +777,7 @@ impl<K, V> RawTable<K, V> {
// FORK NOTE: Uses alloc shim instead of Heap.alloc
let buffer = alloc(round_up_to_page_size(size), alignment);
let buffer = alloc(size, alignment);
if buffer.is_null() {
@ -813,24 +813,6 @@ impl<K, V> RawTable<K, V> {
}
}
/// Access to the raw buffer backing this table.
pub fn raw_buffer(&self) -> (*const (), usize) {
debug_assert!(self.capacity() != 0);
let buffer = self.hashes.ptr() as *const ();
let size = {
let hashes_size = self.capacity() * size_of::<HashUint>();
let pairs_size = self.capacity() * size_of::<(K, V)>();
let (_, _, size, _) = calculate_allocation(hashes_size,
align_of::<HashUint>(),
pairs_size,
align_of::<(K, V)>());
round_up_to_page_size(size)
};
(buffer, size)
}
/// Creates a new raw table from a given capacity. All buckets are
/// initially empty.
pub fn new(capacity: usize) -> Result<RawTable<K, V>, FailedAllocationError> {
@ -1219,19 +1201,3 @@ impl<K, V> Drop for RawTable<K, V> {
}
}
}
// Force all allocations to fill their pages for the duration of the mprotect
// experiment.
#[inline]
fn round_up_to_page_size(size: usize) -> usize {
let page_size = ::SYSTEM_PAGE_SIZE.load(::std::sync::atomic::Ordering::Relaxed);
debug_assert!(page_size != 0);
let mut result = size;
let remainder = size % page_size;
if remainder != 0 {
result += page_size - remainder;
}
debug_assert!(result % page_size == 0);
debug_assert!(result - size < page_size);
result
}

View File

@ -339,25 +339,6 @@ impl<K, V, S> MallocSizeOf for hashglobe::hash_map::HashMap<K, V, S>
}
}
impl<K, V, S> MallocShallowSizeOf for hashglobe::protected::ProtectedHashMap<K, V, S>
where K: Eq + Hash,
S: BuildHasher
{
fn shallow_size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
self.inner().shallow_size_of(ops)
}
}
impl<K, V, S> MallocSizeOf for hashglobe::protected::ProtectedHashMap<K, V, S>
where K: Eq + Hash + MallocSizeOf,
V: MallocSizeOf,
S: BuildHasher,
{
fn size_of(&self, ops: &mut MallocSizeOfOps) -> usize {
self.inner().size_of(ops)
}
}
// XXX: we don't want MallocSizeOf to be defined for Rc and Arc. If negative
// trait bounds are ever allowed, this code should be uncommented.
// (We do have a compile-fail test for this:

View File

@ -1562,14 +1562,6 @@ extern "C" {
extern "C" {
pub fn Gecko_GetSystemPageSize() -> usize;
}
extern "C" {
pub fn Gecko_ProtectBuffer(buffer: *mut ::std::os::raw::c_void,
size: usize);
}
extern "C" {
pub fn Gecko_UnprotectBuffer(buffer: *mut ::std::os::raw::c_void,
size: usize);
}
extern "C" {
pub fn Gecko_Construct_Default_nsStyleFont(ptr: *mut nsStyleFont,
pres_context:
@ -2999,10 +2991,6 @@ extern "C" {
*const ServoRawOffsetArc<RustString>)
-> ServoRawOffsetArc<RustString>;
}
extern "C" {
pub fn Servo_CorruptRuleHashAndCrash(set: RawServoStyleSetBorrowed,
index: usize);
}
extern "C" {
pub fn Gecko_CreateCSSErrorReporter(sheet: *mut ServoStyleSheet,
loader: *mut Loader, uri: *mut nsIURI)

View File

@ -13,16 +13,11 @@ use fnv;
pub use hashglobe::hash_map::HashMap;
#[cfg(feature = "gecko")]
pub use hashglobe::hash_set::HashSet;
#[cfg(feature = "gecko")]
pub use hashglobe::protected::ProtectedHashMap;
#[cfg(feature = "servo")]
pub use hashglobe::fake::{HashMap, HashSet};
/// Alias to use regular HashMaps everywhere in Servo.
#[cfg(feature = "servo")]
pub type ProtectedHashMap<K, V, S> = HashMap<K, V, S>;
/// Appropriate reexports of hash_map types
pub mod map {
#[cfg(feature = "gecko")]

View File

@ -299,24 +299,6 @@ impl InvalidationMap {
Ok(())
}
/// Allows mutation of this InvalidationMap.
#[cfg(feature = "gecko")]
pub fn begin_mutation(&mut self) {
self.class_to_selector.begin_mutation();
self.id_to_selector.begin_mutation();
self.state_affecting_selectors.begin_mutation();
self.other_attribute_affecting_selectors.begin_mutation();
}
/// Disallows mutation of this InvalidationMap.
#[cfg(feature = "gecko")]
pub fn end_mutation(&mut self) {
self.class_to_selector.end_mutation();
self.id_to_selector.end_mutation();
self.state_affecting_selectors.end_mutation();
self.other_attribute_affecting_selectors.end_mutation();
}
}
/// A struct that collects invalidations for a given compound selector.

View File

@ -10,7 +10,7 @@ use applicable_declarations::ApplicableDeclarationBlock;
use context::QuirksMode;
use dom::TElement;
use fallible::FallibleVec;
use hash::{HashMap, HashSet, ProtectedHashMap};
use hash::{HashMap, HashSet};
use hash::map as hash_map;
use hashglobe::FailedAllocationError;
use pdqsort::sort_by;
@ -38,9 +38,6 @@ impl Default for PrecomputedHasher {
/// A simple alias for a hashmap using PrecomputedHasher.
pub type PrecomputedHashMap<K, V> = HashMap<K, V, BuildHasherDefault<PrecomputedHasher>>;
/// A simple alias for a hashmap using PrecomputedHasher.
pub type PrecomputedProtectedHashMap<K, V> = ProtectedHashMap<K, V, BuildHasherDefault<PrecomputedHasher>>;
/// A simple alias for a hashset using PrecomputedHasher.
pub type PrecomputedHashSet<K> = HashSet<K, BuildHasherDefault<PrecomputedHasher>>;
@ -105,7 +102,7 @@ pub struct SelectorMap<T: 'static> {
/// A hash from a class name to rules which contain that class selector.
pub class_hash: MaybeCaseInsensitiveHashMap<Atom, SmallVec<[T; 1]>>,
/// A hash from local name to rules which contain that local name selector.
pub local_name_hash: PrecomputedProtectedHashMap<LocalName, SmallVec<[T; 1]>>,
pub local_name_hash: PrecomputedHashMap<LocalName, SmallVec<[T; 1]>>,
/// Rules that don't have ID, class, or element selectors.
pub other: SmallVec<[T; 1]>,
/// The number of entries in this map.
@ -126,7 +123,7 @@ impl<T: 'static> SelectorMap<T> {
SelectorMap {
id_hash: MaybeCaseInsensitiveHashMap::new(),
class_hash: MaybeCaseInsensitiveHashMap::new(),
local_name_hash: ProtectedHashMap::default(),
local_name_hash: HashMap::default(),
other: SmallVec::new(),
count: 0,
}
@ -150,30 +147,6 @@ impl<T: 'static> SelectorMap<T> {
pub fn len(&self) -> usize {
self.count
}
/// Allows mutation of this SelectorMap.
#[cfg(feature = "gecko")]
pub fn begin_mutation(&mut self) {
self.id_hash.begin_mutation();
self.class_hash.begin_mutation();
self.local_name_hash.begin_mutation();
}
/// Allows mutation of this SelectorMap. Not enforced in Servo.
#[cfg(feature = "servo")]
pub fn begin_mutation(&mut self) {}
/// Disallows mutation of this SelectorMap.
#[cfg(feature = "gecko")]
pub fn end_mutation(&mut self) {
self.id_hash.end_mutation();
self.class_hash.end_mutation();
self.local_name_hash.end_mutation();
}
/// Disallows mutation of this SelectorMap. Not enforced in Servo.
#[cfg(feature = "servo")]
pub fn end_mutation(&mut self) {}
}
impl SelectorMap<Rule> {
@ -490,7 +463,7 @@ fn find_bucket<'a>(mut iter: SelectorIter<'a, SelectorImpl>) -> Bucket<'a> {
#[derive(Debug)]
#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
#[cfg_attr(feature = "servo", derive(HeapSizeOf))]
pub struct MaybeCaseInsensitiveHashMap<K: PrecomputedHash + Hash + Eq, V: 'static>(PrecomputedProtectedHashMap<K, V>);
pub struct MaybeCaseInsensitiveHashMap<K: PrecomputedHash + Hash + Eq, V: 'static>(PrecomputedHashMap<K, V>);
// FIXME(Manishearth) the 'static bound can be removed when
// our HashMap fork (hashglobe) is able to use NonZero,
@ -498,7 +471,7 @@ pub struct MaybeCaseInsensitiveHashMap<K: PrecomputedHash + Hash + Eq, V: 'stati
impl<V: 'static> MaybeCaseInsensitiveHashMap<Atom, V> {
/// Empty map
pub fn new() -> Self {
MaybeCaseInsensitiveHashMap(PrecomputedProtectedHashMap::default())
MaybeCaseInsensitiveHashMap(PrecomputedHashMap::default())
}
/// HashMap::entry
@ -539,17 +512,5 @@ impl<V: 'static> MaybeCaseInsensitiveHashMap<Atom, V> {
self.0.get(key)
}
}
/// ProtectedHashMap::begin_mutation
#[cfg(feature = "gecko")]
pub fn begin_mutation(&mut self) {
self.0.begin_mutation();
}
/// ProtectedHashMap::end_mutation
#[cfg(feature = "gecko")]
pub fn end_mutation(&mut self) {
self.0.end_mutation();
}
}

View File

@ -159,15 +159,6 @@ impl<T> PerPseudoElementMap<T> {
*self = Self::default();
}
/// Invokes a callback on each non-None entry.
pub fn for_each<F: FnMut(&mut T)>(&mut self, mut f: F) {
for entry in self.entries.iter_mut() {
if entry.is_some() {
f(entry.as_mut().unwrap());
}
}
}
/// Set an entry value.
///
/// Returns an error if the element is not a simple pseudo.

View File

@ -1508,17 +1508,6 @@ impl Stylist {
pub fn shutdown() {
UA_CASCADE_DATA_CACHE.lock().unwrap().clear()
}
/// Temporary testing method. See bug 1403397.
pub fn corrupt_rule_hash_and_crash(&self, index: usize) {
let mut origin_iter = self.cascade_data.iter_origins();
let d = origin_iter.next().unwrap().0;
let mut it = d.element_map.local_name_hash.iter();
let nth = index % it.len();
let entry = it.nth(nth).unwrap();
let ptr = entry.0 as *const _ as *const usize as *mut usize;
unsafe { *ptr = 0; }
}
}
/// This struct holds data which users of Stylist may want to extract
@ -1881,32 +1870,6 @@ impl CascadeData {
}
}
#[cfg(feature = "gecko")]
fn begin_mutation(&mut self, rebuild_kind: &SheetRebuildKind) {
self.element_map.begin_mutation();
self.pseudos_map.for_each(|m| m.begin_mutation());
if rebuild_kind.should_rebuild_invalidation() {
self.invalidation_map.begin_mutation();
self.selectors_for_cache_revalidation.begin_mutation();
}
}
#[cfg(feature = "servo")]
fn begin_mutation(&mut self, _: &SheetRebuildKind) {}
#[cfg(feature = "gecko")]
fn end_mutation(&mut self, rebuild_kind: &SheetRebuildKind) {
self.element_map.end_mutation();
self.pseudos_map.for_each(|m| m.end_mutation());
if rebuild_kind.should_rebuild_invalidation() {
self.invalidation_map.end_mutation();
self.selectors_for_cache_revalidation.end_mutation();
}
}
#[cfg(feature = "servo")]
fn end_mutation(&mut self, _: &SheetRebuildKind) {}
/// Collects all the applicable media query results into `results`.
///
/// This duplicates part of the logic in `add_stylesheet`, which is
@ -1970,7 +1933,6 @@ impl CascadeData {
self.effective_media_query_results.saw_effective(stylesheet);
}
self.begin_mutation(&rebuild_kind);
for rule in stylesheet.effective_rules(device, guard) {
match *rule {
CssRule::Style(ref locked) => {
@ -2007,11 +1969,8 @@ impl CascadeData {
None => &mut self.element_map,
Some(pseudo) => {
self.pseudos_map
.get_or_insert_with(&pseudo.canonical(), || {
let mut map = Box::new(SelectorMap::new());
map.begin_mutation();
map
}).expect("Unexpected tree pseudo-element?")
.get_or_insert_with(&pseudo.canonical(), || Box::new(SelectorMap::new()))
.expect("Unexpected tree pseudo-element?")
}
};
@ -2105,7 +2064,6 @@ impl CascadeData {
_ => {}
}
}
self.end_mutation(&rebuild_kind);
Ok(())
}

View File

@ -17,7 +17,6 @@ gecko_debug = ["style/gecko_debug"]
atomic_refcell = "0.1"
cssparser = "0.21.1"
env_logger = {version = "0.4", default-features = false} # disable `regex` to reduce code size
hashglobe = {path = "../../components/hashglobe"}
libc = "0.2"
log = {version = "0.3.5", features = ["release_max_level_info"]}
malloc_size_of = {path = "../../components/malloc_size_of"}

View File

@ -182,10 +182,6 @@ pub extern "C" fn Servo_Initialize(dummy_url_data: *mut URLExtraData) {
// Initialize the dummy url data
unsafe { DUMMY_URL_DATA = dummy_url_data; }
// Set the system page size.
let page_size = unsafe { bindings::Gecko_GetSystemPageSize() };
::hashglobe::SYSTEM_PAGE_SIZE.store(page_size, ::std::sync::atomic::Ordering::Relaxed);
}
#[no_mangle]
@ -4113,12 +4109,6 @@ pub extern "C" fn Servo_HasPendingRestyleAncestor(element: RawGeckoElementBorrow
false
}
#[no_mangle]
pub extern "C" fn Servo_CorruptRuleHashAndCrash(set: RawServoStyleSetBorrowed, index: usize) {
let per_doc_data = PerDocumentStyleData::from_ffi(set).borrow();
per_doc_data.stylist.corrupt_rule_hash_and_crash(index);
}
#[no_mangle]
pub unsafe extern "C" fn Servo_SelectorList_Parse(
selector_list: *const nsACString,

View File

@ -6,7 +6,6 @@
extern crate cssparser;
extern crate env_logger;
extern crate hashglobe;
extern crate libc;
#[macro_use] extern crate log;
extern crate malloc_size_of;

View File

@ -17,7 +17,6 @@ cssparser = "0.21.1"
env_logger = "0.4"
euclid = "0.15"
geckoservo = {path = "../../../ports/geckolib"}
hashglobe = {path = "../../../components/hashglobe"}
libc = "0.2"
log = {version = "0.3.5", features = ["release_max_level_info"]}
malloc_size_of = {path = "../../../components/malloc_size_of"}

View File

@ -6,7 +6,6 @@ extern crate atomic_refcell;
extern crate cssparser;
extern crate env_logger;
extern crate geckoservo;
extern crate hashglobe;
#[macro_use] extern crate log;
extern crate malloc_size_of;
extern crate selectors;