1334579 - stylo: crashtests report "negative leaks" on StringAdopt

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

See https://treeherder.mozilla.org/logviewer.html#?job_id=72470562&repo=stylo&lineNumber=364874

Comment 1

[Emilio Cobos Álvarez (:emilio)]

I bet I know why this is, give me a second...

Comment 2

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

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)

Comment 6

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 7

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1334579: Integrate nsstring bindings with leak logging.

Review commit: https://reviewboard.mozilla.org/r/107828/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107828/

Comment 9

[Nika Layzell [:nika] (ni? for response)]

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.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8831271 [details]
Bug 1334579: Integrate nsstring bindings with leak logging.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107828/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df5bdfec12e0
Integrate nsstring bindings with leak logging. r=mystor

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Still pending the contentdata changes, but I got distracted from this :)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Ensure constructos for nsStyleContentData are run.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce597d1c063e
Ensure constructors for nsStyleContentData run. r=bholley

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/df5bdfec12e0

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ce597d1c063e

Comment 17

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 18

[Nika Layzell [:nika] (ni? for response)]

(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?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

> 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.

Comment 20

[Manish Goregaokar [:manishearth]]

Yeah, we just need a sync, the bindings are a bit muddy now is all.

Comment 21

[Bobby Holley (:bholley)]

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.

Comment 22

[Manish Goregaokar [:manishearth]]

We're manually vendoring for the same reason we check in bindings -- it's impossible to run CI on pure Servo without it.

Comment 23

[Nika Layzell [:nika] (ni? for response)]

(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.

Comment 24

[Bobby Holley (:bholley)]

Thanks!

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Should be fixed with https://github.com/servo/servo/pull/15582