get rid of nsRefPtr / RefPtr split

RESOLVED FIXED in Firefox 44

Status

()

Core
MFBT
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Seems as good a time as any to do this.
(Assignee)

Comment 1

2 years ago
Created attachment 8664436 [details] [diff] [review]
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.
Attachment #8664436 - Flags: review?(ehsan)
(Assignee)

Comment 2

2 years ago
Created attachment 8664437 [details] [diff] [review]
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).
Attachment #8664437 - Flags: review?(ehsan)
(Assignee)

Comment 3

2 years ago
Created attachment 8664438 [details] [diff] [review]
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'
Attachment #8664438 - Flags: review?(ehsan)
(Assignee)

Comment 4

2 years ago
Created attachment 8664439 [details] [diff] [review]
part 4 - remove RefPtr.h

part 4 - remove RefPtr.h

It has been superseded by the availability of nsRefPtr.h.
Attachment #8664439 - Flags: review?(ehsan)
(Assignee)

Comment 5

2 years ago
Created attachment 8664440 [details] [diff] [review]
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.
Attachment #8664440 - Flags: review?(ehsan)
(Assignee)

Comment 6

2 years ago
Created attachment 8664442 [details] [diff] [review]
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
Attachment #8664442 - Flags: review?(ehsan)
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Updated

2 years ago
Attachment #8664439 - Attachment description: .# Attachment to Bug 1207245 - get rid of nsRefPtr / RefPtr split → part 4 - remove RefPtr.h

Comment 8

2 years ago
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."  :-)
Attachment #8664436 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8664437 - Flags: review?(ehsan) → review+

Comment 9

2 years ago
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
Attachment #8664438 - Flags: review?(ehsan) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8665065 [details] [diff] [review]
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.
Attachment #8665065 - Flags: review?(ehsan)

Comment 11

2 years ago
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.
Attachment #8665065 - Flags: review?(ehsan) → review+
(Assignee)

Comment 12

2 years ago
(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*.

Updated

2 years ago
Attachment #8664439 - Flags: review?(ehsan) → review+

Updated

2 years ago
Attachment #8664440 - Flags: review?(ehsan) → review+

Comment 13

2 years ago
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!
Attachment #8664442 - Flags: review?(ehsan) → review+

Comment 14

2 years ago
(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

2 years ago
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
(Assignee)

Comment 16

2 years ago
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → nfroyd
Keywords: leave-open
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

2 years ago
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

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

Comment 20

2 years ago
https://hg.mozilla.org/mozilla-central/rev/a45d680f073f
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
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
Flags: needinfo?(nfroyd)

Updated

2 years ago
Blocks: 1212462

Comment 23

2 years ago
Nathan, you really should do the merges yourself.  I don't think sheriffs can ever do it for you successfully.
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.
(Assignee)

Comment 25

2 years ago
(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.
Flags: needinfo?(nfroyd)

Comment 26

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0654ecff2a
(Assignee)

Comment 27

2 years ago
(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.
(Assignee)

Comment 28

2 years ago
Created attachment 8672722 [details] [diff] [review]
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.
Attachment #8672722 - Flags: review?(botond)
(Assignee)

Comment 29

2 years ago
Created attachment 8672781 [details] [diff] [review]
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.
Attachment #8672781 - Flags: review?(botond)
(Assignee)

Updated

2 years ago
Attachment #8672722 - Attachment is obsolete: true
Attachment #8672722 - Flags: review?(botond)
https://hg.mozilla.org/mozilla-central/rev/0c0654ecff2a

Comment 31

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c93b4b5ef5f
https://hg.mozilla.org/mozilla-central/rev/1c93b4b5ef5f

Updated

2 years ago
Attachment #8672781 - Flags: review?(botond) → review+

Comment 33

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3aaa4ba24cb
https://hg.mozilla.org/mozilla-central/rev/c3aaa4ba24cb

Comment 35

2 years ago
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
(Assignee)

Comment 36

2 years ago
(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/
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/ca86c21a96b4
https://hg.mozilla.org/mozilla-central/rev/12d944fcb09d
https://hg.mozilla.org/mozilla-central/rev/e8c7dfe727cd
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Updated

2 years ago
Depends on: 1216177
Depends on: 1217947

Updated

2 years ago
Depends on: 1218160
See Also: → bug 925371

Updated

11 months ago
Blocks: 1320785
You need to log in before you can comment on or make changes to this bug.