1188696 - Hoist nsRefPtr into MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Bobby Holley (:bholley)]

There are actually very few XPCOM dependencies in nsRefPtr.h. We decided a while back to migrate everything to nsRefPtr, at which point we can kill RefPtr.

One of the current advantages of RefPtr is that it lives in mfbt, so I'm going to see if I can get us parity there.

This is a necessary step along the road to bug 1188690.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The main problem here is leak logging, right?

Comment 2

[Bobby Holley (:bholley)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> The main problem here is leak logging, right?

Where does that live? It does not appear to live in nsRefPtr.h.

Comment 3

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=539c90722429

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Oh, looks like the smart pointer leak logging stuff is nsCOMPtr only.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Remove the XPCOM dependencies in nsRefPtr.h. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Hoist nsRefPtr.h into MFBT. v1

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

LGTM, though this obviously falls foul of the no-ns-prefixing policy in MFBT.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8640316 [details] [diff] [review]
Part 1 - Remove the XPCOM dependencies in nsRefPtr.h. v1

Review of attachment 8640316 [details] [diff] [review]:
-----------------------------------------------------------------

This patch seems good enough to go in regardless of what we do with part 2.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (:bholley) from comment #0)
> There are actually very few XPCOM dependencies in nsRefPtr.h. We decided a
> while back to migrate everything to nsRefPtr, at which point we can kill
> RefPtr.

Who is "we" here and when was this decided?  (It's possible that I was involved in "we" and just don't remember!)

> One of the current advantages of RefPtr is that it lives in mfbt, so I'm
> going to see if I can get us parity there.

I was kind of thinking about going the other way, and having RefPtr be the source of truth, and removing nsRefPtr.  I think we could even make the nsCOMPtr helpers work properly with RefPtr without exposing its innards.  (RefCounted<T> should go away, as much as possible, but RefPtr is not *so* bad.)

What are we gaining by hoisting nsRefPtr into MFBT now, versus using RefPtr for whatever AbstractThread &co needs, and waiting for the mass-conversion later on?

Comment 10

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #7)
> LGTM, though this obviously falls foul of the no-ns-prefixing policy in MFBT.

Yeah, but de-prefixing would make it conflict with the existing RefPtr.

IMO, we should get this into mfbt, switch everything over to nsRefPtr, remove RefPtr, and then rename nsRefPtr/nsCOMPtr to RefPtr/COMPtr.

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #0)
> > There are actually very few XPCOM dependencies in nsRefPtr.h. We decided a
> > while back to migrate everything to nsRefPtr, at which point we can kill
> > RefPtr.
> 
> Who is "we" here and when was this decided?  (It's possible that I was
> involved in "we" and just don't remember!)

https://groups.google.com/forum/#!topic/mozilla.dev.platform/Xsuwy4h7lNE

> I was kind of thinking about going the other way, and having RefPtr be the
> source of truth, and removing nsRefPtr.  I think we could even make the
> nsCOMPtr helpers work properly with RefPtr without exposing its innards. 
> (RefCounted<T> should go away, as much as possible, but RefPtr is not *so*
> bad.)

I don't so much care about the internals, but insofar as there are any semantic differences, we should prefer nsRefPtr semantics because:
* There are 13200 uses of nsRefPtr, 26610 uses of nsCOMPtr, and 3398 uses of RefPtr in our tree.
* People have been ripping out RefPtr in accordance with the outcome of the mailing list thread above.

If at some point nsRefPtr and RefPtr are perfectly semantically equivalent, and we can just rm nsRefPtr and replace all the uses with RefPtr, that's totally fine with me.

> What are we gaining by hoisting nsRefPtr into MFBT now, versus using RefPtr
> for whatever AbstractThread &co needs, and waiting for the mass-conversion
> later on?

Because we just changed all the code in media from RefPtr to nsRefPtr in accordance with the decision above.

This isn't an issue of one thing being a better or worse impl than the other. But the impedence mismatch between the two classes is a huge problem (or at least has been - things may be better with the recent removal of TemporaryRef). What I'm proposing seems like the quickest path to consistency.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8640317 [details] [diff] [review]
Part 2 - Hoist nsRefPtr.h into MFBT. v1

Review of attachment 8640317 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation.  I think we all want the same things and are just talking about slightly different paths to get there.

In hopes of hastening the day when we have a single concrete refptr class, let's do this.

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8640317 [details] [diff] [review]
Part 2 - Hoist nsRefPtr.h into MFBT. v1

Thanks Nathan!

Looks like Nathan is a peer of MFBT too, so I don't think we need additional review from Waldo.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dd6950f8f2ba
https://hg.mozilla.org/integration/mozilla-inbound/rev/81beff07c6dc

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dd6950f8f2ba
https://hg.mozilla.org/mozilla-central/rev/81beff07c6dc