1400510 - stylo: Add refcount logging for servo_arc so we can detect reference cycles that cause leaks

Status: NEW

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Gecko has the ability to log addref/release, complete with stacks and whatnot, to enable detection of leaks of refcounted objects.

We don't seem to have anything like that for servo_arc.

Note that due to Rust's semantics for Arc I'm not terribly worried about leaks due to manually unbalanced release/addref, because those don't happen manually.  What I _am_ worried about is leaks due to reference cycles.  Right now I don't believe we have any real visibility into this, right?

Comment 1

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #0)
> Gecko has the ability to log addref/release, complete with stacks and
> whatnot, to enable detection of leaks of refcounted objects.
> 
> We don't seem to have anything like that for servo_arc.
> 
> Note that due to Rust's semantics for Arc I'm not terribly worried about
> leaks due to manually unbalanced release/addref, because those don't happen
> manually.  What I _am_ worried about is leaks due to reference cycles. 
> Right now I don't believe we have any real visibility into this, right?

Well, anything that does MOZ_COUNT_CTOR/MOZ_COUNT_DTOR will be fine, right? That's certainly the case for style contexts and style structs.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Well, anything that does MOZ_COUNT_CTOR/MOZ_COUNT_DTOR will be fine, right?

Yes, that's true.

> That's certainly the case for style contexts and style structs.

I'm not terribly worried about style contexts and style structs, because they should really not have any cyclical anything going on.  The fact that we do lock their construction/destruction does help.  I was a bit more worried about the data structures the CSS parser produces, say.

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

I guess we can reuse Gecko's NS_LogCtor and NS_LogDtor on Arc::new and Arc::drop_slow respectively.

There are three blockers, though:
1. We cannot get type name string from Rust. Rust has an intrinsic "type_name" which returns a string representing the type, but intrinsics are unstable, and there is no stable interface exposed for that.
2. Even if we can get the type string somehow, the Gecko functions want a C-style string, which Rust isn't going to give us anyway, so we may need to either duplicate some of the code, or teach the existing code to accept slice.
3. Arc supports unsized types, but GetBloatEntry asserts that type with the same name has the same size. This would only be a problem if we use Arc with slice or str, I guess, and IIRC we don't, so it's probably not a big issue.

Comment 4

[Daniel Holbert [:dholbert]]

(This sounds like something we might be able to uplift without too much risk, so let's call this "fix-optional" for 57.)

Comment 5

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57