1207245 - get rid of nsRefPtr / RefPtr split

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

Seems as good a time as any to do this.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - move RefCounted<T> to its own file

Various bits depend on RefPtr.h to provide RefCounted<T> and RefPtr<T>.
It will be easier to manage an automatic conversion from RefPtr<T> to
nsRefPtr<T> if we split out the dependency on RefCounted<T> first.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - move MakeAndAddRef to nsRefPtr.h

A number of places depend on RefPtr.h providing this function.  When we
s/RefPtr/nsRefPtr/, such places still need to be able to see this
function.  Moving it to nsRefPtr.h makes it still visible before we
switch (since RefPtr.h includes nsRefPtr.h), and after we switch (since
every place that #includes RefPtr.h will now be #including nsRefPtr.h).

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - switch all uses of mozilla::RefPtr<T> to nsRefPtr<T>

# Don't modify select files in mfbt/ because it's not worth trying to
 # tease out the dependencies currently.
 #
 # Don't modify anything in media/gmp-clearkey/0.1/ because those files
 # use their own RefPtr, defined in their own RefCounted.h.
find . -name '*.cpp' -o -name '.h' -o -name '*.mm' -o -name '*.idl'| \
    grep -v 'mfbt/RefPtr.h' | \
    grep -v 'mfbt/nsRefPtr.h' | \
    grep -v 'mfbt/RefCounted.h' | \
    grep -v 'media/gmp-clearkey/0.1/' | \
    xargs perl -p -i -e '
 s/mozilla::RefPtr/nsRefPtr/g; # handle declarations in headers
 s/\bRefPtr</nsRefPtr</g; # handle local variables in functions
 s#mozilla/RefPtr.h#mozilla/nsRefPtr.h#; # handle #includes
 s#mfbt/RefPtr.h#mfbt/nsRefPtr.h#;       # handle strange #includes
'

 # |using mozilla::RefPtr;| is OK; |using nsRefPtr;| is invalid syntax.
find . -name '*.cpp' | xargs sed -i -e '/using nsRefPtr/d'

 # RefPtr.h used |byRef| for dealing with COM-style outparams.
 # nsRefPtr.h uses |getter_AddRefs|.
 # Fixup that mismatch.
find . -name '*.cpp' | \
    xargs perl -p -i -e 's/byRef/getter_AddRefs/g'

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 4 - remove RefPtr.h

part 4 - remove RefPtr.h

It has been superseded by the availability of nsRefPtr.h.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 5 - rename template parameters for nsRefPtrHashtable

Having a template parameter conflict with a global name is terribly
inconvenient, so let's try to avoid that by renaming the 'RefPtr'
template parameter to something else.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 6 - rename nsRefPtr<T> to RefPtr<T>

The bulk of this commit was generated with a script, executed at the top
level of a typical source code checkout.  The only non-machine-generated
part was modifying MFBT's moz.build to reflect the new naming.

find . -name '*.cpp' -o -name '*.cc' -o -name '*.h' -o -name '*.mm' -o -name '*.idl'| \
    xargs perl -p -i -e '
 s/nsRefPtr\.h/RefPtr\.h/g; # handle includes
 s/nsRefPtr ?</RefPtr</g;   # handle declarations and variables
'

 # Handle nsRefPtr.h itself, a couple places that define constructors
 # from nsRefPtr, and code generators specially.  We do this here, rather
 # than indiscriminantly s/nsRefPtr/RefPtr/, because that would rename
 # things like nsRefPtrHashtable.
perl -p -i -e 's/nsRefPtr/RefPtr/g' \
     mfbt/nsRefPtr.h \
     xpcom/glue/nsCOMPtr.h \
     xpcom/base/OwningNonNull.h \
     ipc/ipdl/ipdl/lower.py \
     ipc/ipdl/ipdl/builtin.py \
     dom/bindings/Codegen.py \
     python/lldbutils/lldbutils/utils.py

 # In our indiscriminate substitution above, we renamed
 # nsRefPtrGetterAddRefs, the class behind getter_AddRefs.  Fix that up.
find . -name '*.cpp' -o -name '*.h' -o -name '*.idl' | \
    xargs perl -p -i -e 's/nsRefPtrGetterAddRefs/RefPtrGetterAddRefs/g'

git mv mfbt/nsRefPtr.h mfbt/RefPtr.h

Comment 7

[Nathan Froyd [:froydnj]]

It's worth noting that the "new" RefPtr<T> is RefPtr *at global scope*; it does not live in the mozilla namespace.  I am slightly bummed out by that, but I don't have the tools or fortitude to fix all the instances of RefPtr<T> to be properly qualified (or not, depending).  Suggestions or clever ideas on how to do that welcome.

I'm also going to assume that I have rs+ for fixing up any problems that a wider set of platforms and/or rebases turn up.  Given that the bulk of the patches are machine-generated, I don't think this will be too much of a problem.

Comment 8

[(no longer active)]

Comment on attachment 8664436 [details] [diff] [review]
part 1 - move RefCounted<T> to its own file

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

::: mfbt/RefCounted.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* CRTP refcounting templates.  Do not use unless you are an Expert. */

I really wish you just converted the above to "Do not use."  :-)

Comment 9

[(no longer active)]

Comment on attachment 8664438 [details] [diff] [review]
part 3 - switch all uses of mozilla::RefPtr<T> to nsRefPtr<T>

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

rs=me

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 0 - fix why-did-we-allow-that tests in TestRefPtr.cpp

RefPtr.h's byRef permits callees to see the incoming value of the
outparam; XPCOM's getter_AddRefs zeros outparams prior to the call, so
information doesn't leak through inadvertently.  Given this difference,
we need to eliminate tests that depended on this (arguably dangerous)
behavior.  The numerous assertion fixups are required because we're
removing construction and destruction of objects along the way.

Comment 11

[(no longer active)]

Comment on attachment 8665065 [details] [diff] [review]
part 0 - fix why-did-we-allow-that tests in TestRefPtr.cpp

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

Crap.  Looks like we're using byRef() all over the place.  :(  We should start a new project to get rid of that.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #11)
> Crap.  Looks like we're using byRef() all over the place.  :(  We should
> start a new project to get rid of that.

byRef() gets completely removed by part 3 in this patch; its uses are replaced by getter_AddRefs.  Investigation on try suggests that nobody is depending on this behavior, which is a *good thing*.

Comment 13

[(no longer active)]

Comment on attachment 8664442 [details] [diff] [review]
part 6 - rename nsRefPtr<T> to RefPtr<T>

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

Thanks a lot for doing this, Nathan!  You rock!

Comment 14

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #12)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #11)
> > Crap.  Looks like we're using byRef() all over the place.  :(  We should
> > start a new project to get rid of that.
> 
> byRef() gets completely removed by part 3 in this patch; its uses are
> replaced by getter_AddRefs.  Investigation on try suggests that nobody is
> depending on this behavior, which is a *good thing*.

Awesome!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86cd8c6a23d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ee8cbe5e73
https://hg.mozilla.org/integration/mozilla-inbound/rev/86fb8a462b04

Comment 16

[Nathan Froyd [:froydnj]]

Landed the first three parts:

https://hg.mozilla.org/integration/mozilla-inbound/rev/86cd8c6a23d1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ee8cbe5e73
https://hg.mozilla.org/integration/mozilla-inbound/rev/86fb8a462b04

I think I may delay landing the others so as not to mess with people doing end-of-the-quarter crash landings.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/86cd8c6a23d1
https://hg.mozilla.org/mozilla-central/rev/a4ee8cbe5e73
https://hg.mozilla.org/mozilla-central/rev/86fb8a462b04

Comment 18

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/bfb9436756a0
https://hg.mozilla.org/mozilla-central/rev/1f51d1614b9a
https://hg.mozilla.org/mozilla-central/rev/ddcc05491282
https://hg.mozilla.org/mozilla-central/rev/93828e13ad63
https://hg.mozilla.org/mozilla-central/rev/bcc1bcac2320
https://hg.mozilla.org/mozilla-central/rev/91d4539e00ce

Comment 19

[Pulsebot]

https://hg.mozilla.org/mozilla-central/rev/263526a3368d

Comment 20

[Pulsebot]

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

Comment 21

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

I had to back this out because I was hitting a lot of merge conflicts trying to get inbound merged to m-c. Feel free to rebase and reland this whenever you get to it.
https://hg.mozilla.org/mozilla-central/rev/1831ba1be747

Comment 22

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

Hrm, bugherder didn't list them all, but I backed them all out:
https://hg.mozilla.org/mozilla-central/rev/1831ba1be747
https://hg.mozilla.org/mozilla-central/rev/6f47f75d3136
https://hg.mozilla.org/mozilla-central/rev/9f660e994268
https://hg.mozilla.org/mozilla-central/rev/5108c4505520
https://hg.mozilla.org/mozilla-central/rev/11db5387d083
https://hg.mozilla.org/mozilla-central/rev/41dea9df27ed
https://hg.mozilla.org/mozilla-central/rev/482930b9667a
https://hg.mozilla.org/mozilla-central/rev/711d402ed650

Comment 23

[(no longer active)]

Nathan, you really should do the merges yourself.  I don't think sheriffs can ever do it for you successfully.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 8664440 [details] [diff] [review]
part 5 - rename template parameters for nsRefPtrHashtable

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

::: xpcom/glue/nsRefPtrHashtable.h
@@ +15,5 @@
>   * templated hashtable class maps keys to reference pointers.
>   * See nsBaseHashtable for complete declaration.
>   * @param KeyClass a wrapper-class for the hashtable key, see nsHashKeys.h
>   *   for a complete specification.
>   * @param RefPtr the reference-type being wrapped

You should change this RefPtr in the comment to PtrType, too.

Comment 25

[Nathan Froyd [:froydnj]]

(In reply to Wes Kocher (:KWierso) from comment #22)
> Hrm, bugherder didn't list them all, but I backed them all out:

Thanks for taking care of this.  I'm going to try landing the standalone patches separately, and then have another go at landing the automated ones later this week or early next.

Comment 26

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0654ecff2a

Comment 27

[Nathan Froyd [:froydnj]]

(In reply to Pulsebot from comment #26)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0654ecff2a

This was additional "mozilla/RefCounted.h" includes as a followup for part 2, which I felt could land without additional review.

Comment 28

[Nathan Froyd [:froydnj]]

Attachment: part 6a - call AddRef/Release from nsRefPtr itself, rather than a helper

Might as well have Botond review this properly before trying to land everything
else.

The problematic class is AtomicRefCountedWithFinalize, from gfx/layers/;
I don't know all the details for why that class is written the way it
is, but it is simpler to make nsRefPtr work the same way as
mozilla::RefPtr in this case.

Comment 29

[Nathan Froyd [:froydnj]]

Attachment: part 6a - call AddRef/Release from nsRefPtr itself, rather than a helper

The problematic class is AtomicRefCountedWithFinalize, from gfx/layers/;
I don't know all the details for why that class is written the way it
is, but it is simpler to make nsRefPtr work the same way as
mozilla::RefPtr in this case.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c0654ecff2a

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c93b4b5ef5f

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1c93b4b5ef5f

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3aaa4ba24cb

Comment 34

[Carsten Book [:Tomcat]]

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

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca86c21a96b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/12d944fcb09d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c7dfe727cd

Comment 36

[Nathan Froyd [:froydnj]]

(In reply to Pulsebot from comment #35)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ca86c21a96b4
> https://hg.mozilla.org/integration/mozilla-inbound/rev/12d944fcb09d
> https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c7dfe727cd

These look like they're gonna stick, so removing leave-open. \o/

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ca86c21a96b4
https://hg.mozilla.org/mozilla-central/rev/12d944fcb09d
https://hg.mozilla.org/mozilla-central/rev/e8c7dfe727cd