Closed
Bug 1334579
Opened 8 years ago
Closed 8 years ago
stylo: crashtests report "negative leaks" on StringAdopt
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: bholley, Assigned: emilio)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
Assignee | ||
Comment 2•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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)
Assignee | ||
Comment 6•8 years ago
|
||
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)
Comment 7•8 years ago
|
||
(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 hidden (mozreview-request) |
Comment 9•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment 11•8 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/df5bdfec12e0 Integrate nsstring bindings with leak logging. r=mystor
Assignee | ||
Comment 12•8 years ago
|
||
Still pending the contentdata changes, but I got distracted from this :)
Whiteboard: leave-open
Assignee | ||
Comment 13•8 years ago
|
||
Assignee: nobody → emilio+bugs
Attachment #8831343 -
Flags: review?(bobbyholley)
Reporter | ||
Updated•8 years ago
|
Attachment #8831343 -
Flags: review?(bobbyholley) → review+
Comment 14•8 years ago
|
||
Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ce597d1c063e Ensure constructors for nsStyleContentData run. r=bholley
Assignee | ||
Updated•8 years ago
|
Whiteboard: leave-open
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df5bdfec12e0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 17•8 years ago
|
||
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 → ---
Comment 18•8 years ago
|
||
(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?
Assignee | ||
Comment 19•8 years ago
|
||
> 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•8 years ago
|
||
Yeah, we just need a sync, the bindings are a bit muddy now is all.
Flags: needinfo?(manishearth)
Reporter | ||
Comment 21•8 years ago
|
||
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)
Comment 22•8 years ago
|
||
We're manually vendoring for the same reason we check in bindings -- it's impossible to run CI on pure Servo without it.
Updated•8 years ago
|
Blocks: stylo-reftest
Comment 23•8 years ago
|
||
(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)
Assignee | ||
Updated•8 years ago
|
Summary: stylo: crashtests report "negative leaks" on StringAdopt and nsStyleContentData → stylo: crashtests report "negative leaks" on StringAdopt
Updated•8 years ago
|
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•