Auto merge of #18549 - nnethercote:fix-enclosing-crash, r=heycam

Fix a panic in Stylo memory reporting.

`MallocSizeOfOps::enclosing_size_of_op` is an `Option<>` type, and the panic in
question is caused by not providing a value in a case where it's needed for
measuring a HashSet.

HashMaps and HashSets are common enough that it makes sense to make
`enclosing_size_of_op` non-optional, which this patch does.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [X] These changes do not require tests because tests are on the Gecko side.

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/18549)
<!-- Reviewable:end -->
This commit is contained in:
bors-servo 2017-09-17 23:19:54 -05:00 committed by GitHub
commit bb998dbdf3
3 changed files with 20 additions and 11 deletions

View file

@ -82,9 +82,8 @@ pub struct MallocSizeOfOps {
/// A function that returns the size of a heap allocation. /// A function that returns the size of a heap allocation.
size_of_op: VoidPtrToSizeFn, size_of_op: VoidPtrToSizeFn,
/// Like `size_of_op`, but can take an interior pointer. Optional, because /// Like `size_of_op`, but can take an interior pointer.
/// many places don't need it. enclosing_size_of_op: VoidPtrToSizeFn,
enclosing_size_of_op: Option<VoidPtrToSizeFn>,
/// Check if a pointer has been seen before, and remember it for next time. /// Check if a pointer has been seen before, and remember it for next time.
/// Useful when measuring `Rc`s and `Arc`s. Optional, because many places /// Useful when measuring `Rc`s and `Arc`s. Optional, because many places
@ -93,8 +92,7 @@ pub struct MallocSizeOfOps {
} }
impl MallocSizeOfOps { impl MallocSizeOfOps {
pub fn new(size_of: VoidPtrToSizeFn, pub fn new(size_of: VoidPtrToSizeFn, malloc_enclosing_size_of: VoidPtrToSizeFn,
malloc_enclosing_size_of: Option<VoidPtrToSizeFn>,
have_seen_ptr: Option<Box<VoidPtrToBoolFnMut>>) -> Self { have_seen_ptr: Option<Box<VoidPtrToBoolFnMut>>) -> Self {
MallocSizeOfOps { MallocSizeOfOps {
size_of_op: size_of, size_of_op: size_of,
@ -129,8 +127,7 @@ impl MallocSizeOfOps {
/// Call `enclosing_size_of_op` on `ptr`, which must not be empty. /// Call `enclosing_size_of_op` on `ptr`, which must not be empty.
pub unsafe fn malloc_enclosing_size_of<T>(&self, ptr: *const T) -> usize { pub unsafe fn malloc_enclosing_size_of<T>(&self, ptr: *const T) -> usize {
assert!(!MallocSizeOfOps::is_empty(ptr)); assert!(!MallocSizeOfOps::is_empty(ptr));
let enclosing_size_of_op = self.enclosing_size_of_op.expect("missing enclosing_size_of_op"); (self.enclosing_size_of_op)(ptr as *const c_void)
enclosing_size_of_op(ptr as *const c_void)
} }
/// Call `have_seen_ptr_op` on `ptr`. /// Call `have_seen_ptr_op` on `ptr`.

View file

@ -1898,7 +1898,10 @@ extern "C" {
pub fn Servo_Element_ClearData(node: RawGeckoElementBorrowed); pub fn Servo_Element_ClearData(node: RawGeckoElementBorrowed);
} }
extern "C" { extern "C" {
pub fn Servo_Element_SizeOfExcludingThisAndCVs(arg1: MallocSizeOf, pub fn Servo_Element_SizeOfExcludingThisAndCVs(malloc_size_of:
MallocSizeOf,
malloc_enclosing_size_of:
MallocSizeOf,
seen_ptrs: *mut SeenPtrs, seen_ptrs: *mut SeenPtrs,
node: node:
RawGeckoElementBorrowed) RawGeckoElementBorrowed)
@ -1964,6 +1967,8 @@ extern "C" {
} }
extern "C" { extern "C" {
pub fn Servo_StyleSheet_SizeOfIncludingThis(malloc_size_of: MallocSizeOf, pub fn Servo_StyleSheet_SizeOfIncludingThis(malloc_size_of: MallocSizeOf,
malloc_enclosing_size_of:
MallocSizeOf,
sheet: sheet:
RawServoStyleSheetContentsBorrowed) RawServoStyleSheetContentsBorrowed)
-> usize; -> usize;

View file

@ -781,13 +781,16 @@ pub extern "C" fn Servo_Element_ClearData(element: RawGeckoElementBorrowed) {
#[no_mangle] #[no_mangle]
pub extern "C" fn Servo_Element_SizeOfExcludingThisAndCVs(malloc_size_of: GeckoMallocSizeOf, pub extern "C" fn Servo_Element_SizeOfExcludingThisAndCVs(malloc_size_of: GeckoMallocSizeOf,
malloc_enclosing_size_of:
GeckoMallocSizeOf,
seen_ptrs: *mut SeenPtrs, seen_ptrs: *mut SeenPtrs,
element: RawGeckoElementBorrowed) -> usize { element: RawGeckoElementBorrowed) -> usize {
let element = GeckoElement(element); let element = GeckoElement(element);
let borrow = element.borrow_data(); let borrow = element.borrow_data();
if let Some(data) = borrow { if let Some(data) = borrow {
let have_seen_ptr = move |ptr| { unsafe { Gecko_HaveSeenPtr(seen_ptrs, ptr) } }; let have_seen_ptr = move |ptr| { unsafe { Gecko_HaveSeenPtr(seen_ptrs, ptr) } };
let mut ops = MallocSizeOfOps::new(malloc_size_of.unwrap(), None, let mut ops = MallocSizeOfOps::new(malloc_size_of.unwrap(),
malloc_enclosing_size_of.unwrap(),
Some(Box::new(have_seen_ptr))); Some(Box::new(have_seen_ptr)));
(*data).size_of_excluding_cvs(&mut ops) (*data).size_of_excluding_cvs(&mut ops)
} else { } else {
@ -1087,11 +1090,14 @@ pub extern "C" fn Servo_StyleSheet_Clone(
#[no_mangle] #[no_mangle]
pub extern "C" fn Servo_StyleSheet_SizeOfIncludingThis( pub extern "C" fn Servo_StyleSheet_SizeOfIncludingThis(
malloc_size_of: GeckoMallocSizeOf, malloc_size_of: GeckoMallocSizeOf,
malloc_enclosing_size_of: GeckoMallocSizeOf,
sheet: RawServoStyleSheetContentsBorrowed sheet: RawServoStyleSheetContentsBorrowed
) -> usize { ) -> usize {
let global_style_data = &*GLOBAL_STYLE_DATA; let global_style_data = &*GLOBAL_STYLE_DATA;
let guard = global_style_data.shared_lock.read(); let guard = global_style_data.shared_lock.read();
let mut ops = MallocSizeOfOps::new(malloc_size_of.unwrap(), None, None); let mut ops = MallocSizeOfOps::new(malloc_size_of.unwrap(),
malloc_enclosing_size_of.unwrap(),
None);
StylesheetContents::as_arc(&sheet).size_of(&guard, &mut ops) StylesheetContents::as_arc(&sheet).size_of(&guard, &mut ops)
} }
@ -3751,7 +3757,8 @@ pub extern "C" fn Servo_StyleSet_AddSizeOfExcludingThis(
) { ) {
let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut(); let data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
let mut ops = MallocSizeOfOps::new(malloc_size_of.unwrap(), let mut ops = MallocSizeOfOps::new(malloc_size_of.unwrap(),
malloc_enclosing_size_of, None); malloc_enclosing_size_of.unwrap(),
None);
let sizes = unsafe { sizes.as_mut() }.unwrap(); let sizes = unsafe { sizes.as_mut() }.unwrap();
data.add_size_of_children(&mut ops, sizes); data.add_size_of_children(&mut ops, sizes);
} }