stylo: crashtests report "negative leaks" on StringAdopt

RESOLVED FIXED in Firefox 54

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: bholley, Assigned: emilio)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(2 attachments)

I bet I know why this is, give me a second...
This is either because we sometimes give rust-allocated buffers and stick them into the contents property holder, or because the nsstring bindings don't take into account the leak checking.
I should prefix this with _I think_
(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> This is either because we sometimes give rust-allocated buffers and stick
> them into the contents property holder, or because the nsstring bindings
> don't take into account the leak checking.
Actually, I was wrong, and this is because of the set_len calls that we do in the nsStyleContentData array, which ends up calling EnsureTArrayCapacity (without running the constructors), and then modifying the length manually.

This is ok, because nsStyleContentData is POD, except for the leak checking code, so probably we should specialize that function so it's typed and calls the constructors.
That explains the nsStyleContentData, the StringAdopt ones are almost sure because of the two nsString::new calls we do from rust, which aren't integrated with the leak checking code (https://github.com/mystor/nsstring/blob/master/src/lib.rs#L292)
I'm not sure there's an easy way to integrate that leak checking in the nsstring bindings easily, we could make a debug-only ffi call, I guess. What do you think Michael?
Flags: needinfo?(michael)
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> I'm not sure there's an easy way to integrate that leak checking in the
> nsstring bindings easily, we could make a debug-only ffi call, I guess. What
> do you think Michael?

Yeah, I think the debug-only FFI call is probably our best bet. 

I'd be happy to look at a patch if you threw one together, or I can write it myself if you're OK with waiting for a few days.
Flags: needinfo?(michael)
Comment on attachment 8831271 [details]
Bug 1334579: Integrate nsstring bindings with leak logging.

https://reviewboard.mozilla.org/r/107828/#review109044

::: xpcom/rust/nsstring/src/lib.rs:366
(Diff revision 1)
>                  // code, meaning that our box can be legally freed with
>                  // libc::free().
>                  let length = s.len() as u32;
>                  let ptr = s.as_ptr();
>                  mem::forget(s);
> +                if cfg!(debug_assertions) {

I think this might be nicer as:

    unsafe {
        Gecko_IncrementStringAdoptCount(ptr as *mut _)
    }
    
With a:

    #[cfg(debug_assertions)]
    extern "C" {
        fn Gecko_IncrementStringAdoptCount(data: *mut c_void); 
    }
    #[cfg(not(debug_assertions))]
    unsafe fn Gecko_IncrementStringAdoptCount(_: *mut c_void) {}

As that means that we can't accidentally call the method when it isn't defined. It feels bad to define a method which isn't present in non-debug builds, even though we don't call it.

::: xpcom/rust/nsstring/src/lib.rs:367
(Diff revision 1)
>                  // libc::free().
>                  let length = s.len() as u32;
>                  let ptr = s.as_ptr();
>                  mem::forget(s);
> +                if cfg!(debug_assertions) {
> +                    unsafe { Gecko_IncrementStringAdoptCount(ptr as *mut _) };

Line break after the unsafe { please :)

::: xpcom/rust/nsstring/src/lib.rs:772
(Diff revision 1)
>  }
>  
>  // NOTE: These bindings currently only expose infallible operations. Perhaps
>  // consider allowing for fallible methods?
>  extern "C" {
> +    fn Gecko_IncrementStringAdoptCount(data: *mut c_void);

Please add a `#[cfg(debug_assertions)]` to this declaration.
Attachment #8831271 - Flags: review?(michael) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/df5bdfec12e0
Integrate nsstring bindings with leak logging. r=mystor
Still pending the contentdata changes, but I got distracted from this :)
Whiteboard: leave-open
Assignee: nobody → emilio+bugs
Attachment #8831343 - Flags: review?(bobbyholley)
Attachment #8831343 - Flags: review?(bobbyholley) → review+
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce597d1c063e
Ensure constructors for nsStyleContentData run. r=bholley
Whiteboard: leave-open
https://hg.mozilla.org/mozilla-central/rev/df5bdfec12e0
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
See Also: → 1335203
This seems to still happen (only with StringAdopt): https://treeherder.mozilla.org/logviewer.html#?job_id=76159638&repo=stylo&lineNumber=77687.

But I bet it's because stylo is using the components/style/gecko_bindings/nsstring_vendor crate (which doesn't contain my patch). Manish, I don't know the history of that directory, I guess it's just a matter of syncing the crates?

Also, I'm wary that with the changes in bug 1316696, syncing this may lead to double-frees? (there's no nsStringRepr anymore, which is what we use inside style structs, etc), so AFAIK destructing the style struct will free strings from the C++ side, and then rust will free it too.
Status: RESOLVED → REOPENED
Flags: needinfo?(manishearth)
Resolution: FIXED → ---
(In reply to Emilio Cobos Álvarez [:emilio] from comment #17)
> This seems to still happen (only with StringAdopt):
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=76159638&repo=stylo&lineNumber=77687.
> 
> But I bet it's because stylo is using the
> components/style/gecko_bindings/nsstring_vendor crate (which doesn't contain
> my patch). Manish, I don't know the history of that directory, I guess it's
> just a matter of syncing the crates?
> 
> Also, I'm wary that with the changes in bug 1316696, syncing this may lead
> to double-frees? (there's no nsStringRepr anymore, which is what we use
> inside style structs, etc), so AFAIK destructing the style struct will free
> strings from the C++ side, and then rust will free it too.

Yes, it's just a matter of syncing the crates. I was intending to do so right after 1316696, but I was having trouble figuring out how to run bindgen without making a large number of unrelated changes.

I'm not sure what you were doing with nsStringRepr which would have worked before and doesn't now. I removed it because I was under the impression that nsString<'static> was strictly safer. Can you elaborate on that?
> I'm not sure what you were doing with nsStringRepr which would have worked
> before and doesn't now. I removed it because I was under the impression that
> nsString<'static> was strictly safer. Can you elaborate on that?

So the way we handle our style structs is really similar to nsStringRepr. We have something like:

impl Drop for ${style_struct.gecko_struct_name} {
    fn drop(&mut self) {
        unsafe {
            Gecko_Destroy_${style_struct.gecko_ffi_name}(&mut self.gecko);
        }
    }
}


Where Destroy does the obvious thing (invoking the relevant destructor).

I'd have to check, but as far as I know, if any of the structs contains a nsString, we'd free it once using that FFI function, and then rust will also run the nsString::drop function, which will also invoke the nsString destructor, which means we'd double-free it. Maybe I'm wrong though.
Yeah, we just need a sync, the bindings are a bit muddy now is all.
Flags: needinfo?(manishearth)
Michael, are you the right person to do this? It's not totally clear to me why we're manually vendoring things here, but I'd generally like to fix these leaks we have on autoland.
Flags: needinfo?(michael)
We're manually vendoring for the same reason we check in bindings -- it's impossible to run CI on pure Servo without it.
Blocks: 1338994
(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #21)
> Michael, are you the right person to do this? It's not totally clear to me
> why we're manually vendoring things here, but I'd generally like to fix
> these leaks we have on autoland.

I filed bug 1339218 to handle the double leak issue, and emilio has said that he'll handle the re-vendoring once that patch has landed into m-c.
Flags: needinfo?(michael)
Thanks!
Summary: stylo: crashtests report "negative leaks" on StringAdopt and nsStyleContentData → stylo: crashtests report "negative leaks" on StringAdopt
Status: REOPENED → RESOLVED
Closed: 3 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.