Closed Bug 1188696 Opened 9 years ago Closed 9 years ago

Hoist nsRefPtr into MFBT

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
The main problem here is leak logging, right?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #1)
> The main problem here is leak logging, right?

Where does that live? It does not appear to live in nsRefPtr.h.
Oh, looks like the smart pointer leak logging stuff is nsCOMPtr only.
Attachment #8640317 - Flags: superreview?(jwalden+bmo)
Attachment #8640317 - Flags: review?(nfroyd)
LGTM, though this obviously falls foul of the no-ns-prefixing policy in MFBT.
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.
Attachment #8640316 - Flags: review?(nfroyd) → review+
(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?
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
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.
Attachment #8640317 - Flags: review?(nfroyd) → review+
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.
Attachment #8640317 - Flags: superreview?(jwalden+bmo)
Blocks: 1189528
You need to log in before you can comment on or make changes to this bug.