Closed Bug 1207245 Opened 9 years ago Closed 9 years ago

get rid of nsRefPtr / RefPtr split

Categories

(Core :: MFBT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

References

Details

Attachments

(8 files, 1 obsolete file)

Seems as good a time as any to do this.
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)
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)
# 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)
part 4 - remove RefPtr.h

It has been superseded by the availability of nsRefPtr.h.
Attachment #8664439 - Flags: review?(ehsan)
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)
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)
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.
Attachment #8664439 - Attachment description: .# Attachment to Bug 1207245 - get rid of nsRefPtr / RefPtr split → part 4 - remove RefPtr.h
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+
Attachment #8664437 - Flags: review?(ehsan) → review+
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+
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 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+
(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*.
Attachment #8664439 - Flags: review?(ehsan) → review+
Attachment #8664440 - Flags: review?(ehsan) → review+
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+
(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!
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: nobody → nfroyd
Keywords: leave-open
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
Blocks: 1212462
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.
(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)
(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.
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)
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)
Attachment #8672722 - Attachment is obsolete: true
Attachment #8672722 - Flags: review?(botond)
Attachment #8672781 - Flags: review?(botond) → review+
Depends on: 1216177
Depends on: 1217947
Depends on: 1218160
You need to log in before you can comment on or make changes to this bug.