get rid of nsAutoPtr's copy constructor

REOPENED
Unassigned

Status

()

defect
REOPENED
5 years ago
2 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

It secretly acts like a move constructor.  Explicitly saying it is going to be moved is better than implicitly relying on it and making it look like the code calling the copy constructor is buggy.
This isn't all of the usage (there's at least the webrtc runnable_utils thing left), but its a lot
Attachment #8419021 - Flags: review?(nfroyd)
Comment on attachment 8419021 [details] [diff] [review]
part 1 - Remove a bunch of usage of nsAutoPtr's copy ctor

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

Couple of comments:

- There's a distinct lack of #include "mozilla/Move.h", which should be fixed.
- You want to define nsAutoPtr& operator=(const nsAutoPtr&&).  (And presumably, in your tree, MOZ_DELETE the normal operator=)
- Following on to that, if you are changing functions to use nsAutoPtr<T>&& arguments and you don't have corresponding Move() changes in the function bodies, then your changes are not doing what you think they are doing.  (If I understand correctly, you're still calling the copy constructor, normal operator=, as appropriate; you're not calling the move versions of those.)
- You should be using Move() instead of Forward() everywhere, if I understand things correctly.  Forward() should only be used for templated code.

::: dom/xslt/xslt/txStylesheet.cpp
@@ +14,5 @@
>  #include "txLog.h"
>  #include "txKey.h"
>  #include "txXPathTreeWalker.h"
>  
> +using namespace mozilla;

How about just |using mozilla::Move;| instead?  Also, #include "mozilla/Move.h" somewhere.

::: dom/xslt/xslt/txStylesheetCompiler.cpp
@@ +722,5 @@
>      mNextInstrPtr = 0;
>  }
>  
>  nsresult
> +txStylesheetCompilerState::addInstruction(nsAutoPtr<txInstruction>&& aInstruction)

This looks to be unnecessary, given the explicit forget() in the body.
Attachment #8419021 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #2)
> Comment on attachment 8419021 [details] [diff] [review]
> part 1 - Remove a bunch of usage of nsAutoPtr's copy ctor
> 
> Review of attachment 8419021 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Couple of comments:
> 
> - There's a distinct lack of #include "mozilla/Move.h", which should be
> fixed.

spraying includes like that around feels a little silly, but I guess is really the right thing to do.

> - You want to define nsAutoPtr& operator=(const nsAutoPtr&&).  (And

I assume you didn't mean the const part, I don't think it makes much sense right?

the existance of the move ctor made me assume there was a move assignment operator too.

> presumably, in your tree, MOZ_DELETE the normal operator=)
> - Following on to that, if you are changing functions to use nsAutoPtr<T>&&
> arguments and you don't have corresponding Move() changes in the function
> bodies, then your changes are not doing what you think they are doing.  (If
> I understand correctly, you're still calling the copy constructor, normal
> operator=, as appropriate; you're not calling the move versions of those.)

Well, given the way those operate on nsAutoPtr that might actually work out ok, but I might as well fix it up now.

> - You should be using Move() instead of Forward() everywhere, if I
> understand things correctly.  Forward() should only be used for templated
> code.

ah, I didn't catch that somehow.
> 
> ::: dom/xslt/xslt/txStylesheet.cpp
> @@ +14,5 @@
> >  #include "txLog.h"
> >  #include "txKey.h"
> >  #include "txXPathTreeWalker.h"
> >  
> > +using namespace mozilla;
> 
> How about just |using mozilla::Move;| instead?  Also, #include

I expect someone will want something else in mozilla:: sooner or later, but I guess it doesn't really make that harder.

> ::: dom/xslt/xslt/txStylesheetCompiler.cpp
> @@ +722,5 @@
> >      mNextInstrPtr = 0;
> >  }
> >  
> >  nsresult
> > +txStylesheetCompilerState::addInstruction(nsAutoPtr<txInstruction>&& aInstruction)
> 
> This looks to be unnecessary, given the explicit forget() in the body.

I'll try and figure out why I needed it.
> > ::: dom/xslt/xslt/txStylesheetCompiler.cpp
> > @@ +722,5 @@
> > >      mNextInstrPtr = 0;
> > >  }
> > >  
> > >  nsresult
> > > +txStylesheetCompilerState::addInstruction(nsAutoPtr<txInstruction>&& aInstruction)
> > 
> > This looks to be unnecessary, given the explicit forget() in the body.
> 
> I'll try and figure out why I needed it.


though actually I'd assert the sane way to write that method is using nsAutoPtr<T>&& so mind if I just change the impl to use Move()?
Comment on attachment 8419663 [details] [diff] [review]
part 1 - Remove a bunch of usage of nsAutoPtr's copy ctor

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

r=me with the changes below.

::: dom/xslt/xslt/txInstructions.cpp
@@ +91,2 @@
>                           txNamespaceMap* aMappings)
> +    : mName(Move(aName)), mNamespace(Move(aNamespace)), mMappings(aMappings)

Nit: in deference to whoever wrote this, I think I'd leave the formatting the same.

@@ +182,2 @@
>                                       txInstruction* aTarget)
> +    : mCondition(Move(aCondition)), mTarget(aTarget)

Here too.

@@ +545,5 @@
> +                   nsAutoPtr<Expr>&& aGroupingSeparator,
> +                   nsAutoPtr<Expr>&& aGroupingSize)
> +    : mLevel(aLevel), mCount(Move(aCount)),
> +    mFrom(Move(aFrom)),
> +    mValue(Move(aValue)),

Nit: indentation looks off here.

::: dom/xslt/xslt/txRtfHandler.cpp
@@ +8,2 @@
>  
> +using mozilla::Move;

I'd spell out mozilla::Move everywhere here, and in nsXPathExpression.cpp.  (I think if you're not already |using namespace mozilla|, spelling things out is a little cleaner for unified builds?  I guess this is up to you.)

::: dom/xslt/xslt/txStylesheetCompiler.cpp
@@ +484,5 @@
>      nsresult rv = pushObject(mElementContext);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      mElementContext.forget();
> +    mElementContext = Move(context);

This looks really weird with the forget, but I assume the pushObject three lines up takes care of that...

::: dom/xslt/xslt/txToplevelItems.cpp
@@ +41,4 @@
>                                 const txExpandedName& aName,
>                                 const txExpandedName& aMode, double aPrio)
> +    : mMatch(Forward<nsAutoPtr<txPattern>>(aMatch)), mName(aName),
> +    mMode(aMode), mPrio(aPrio)

Nit: two more spaces here.

@@ +52,3 @@
>                                 bool aIsParam)
> +    : mName(aName), mValue(Forward<nsAutoPtr<Expr>>(aValue)),
> +    mIsParam(aIsParam)

Nit: two more spaces here.

::: layout/style/nsCSSParser.cpp
@@ +3575,5 @@
>    if (!declaration) {
>      return false;
>    }
>  
>    // Takes ownership of declaration.

I don't know if this comment is useful or not now.

@@ +3598,5 @@
>    if (!declaration) {
>      return nullptr;
>    }
>  
>    // Takes ownership of declaration, and steals contents of selectorList.

Same with this bit about |declaration|.  The selectorList bit is non-obvious, to be sure.

::: xpcom/base/nsAutoPtr.h
@@ +117,5 @@
>            return *this;
>          }
>  
> +      nsAutoPtr<T>&
> +        operator=(nsAutoPtr<T>& aRhs)

Please make the additional method the move operator, rather than altering the existing method, to preserve blame information

Also, this file is about to get reformatted with proper Mozilla style.  If you land after that patch, please fix up your code.
Attachment #8419663 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/60590620a8da
Assignee: nobody → trev.saunders
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Nathan Froyd from comment #2)
> (From update of attachment 8419021 [details] [diff] [review])
> >  nsresult
> > +txStylesheetCompilerState::addInstruction(nsAutoPtr<txInstruction>&& aInstruction)
> 
> This looks to be unnecessary, given the explicit forget() in the body.

* When the callee always wants to transfer the pointer, pass by value. You can pass any suitable constructor expression, such as a raw owned pointer or a move of a variable.
* When the callee may or may not want to transfer the pointer, pass by lvalue reference. The lvalue is needed to hold the pointer for when the callee doesn't want it. You can't pass a temporary expression in this case, so you can't accidentally try to Move the pointer to force it to transfer.
* Passing by rvalue reference gives you the worst of both worlds. You have to Move the lvalue, but it doesn't actually guarantee the transfer of the pointer.

Speaking of forget(), I was disappointed to see so many calls in the code. One example is this:
>      nsresult rv = mGlobalVariables.add(aVariable->mName, var);
>      NS_ENSURE_SUCCESS(rv, rv);
>      
>      var.forget();
Maybe that method needs to take an nsAutoPtr<nsGlobalVariable> parameter so that you can use Move(var) instead.
Other cases could be covered with a move constructor that accepts an nsAutoPtr of a derived type.
Comment on attachment 8426546 [details] [diff] [review]
finish removing nsAutoPtrs copy ctor

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

Did you compile this with --enable-webrtc?  Pretty sure applying the MOZ_DELETE treatment to the copy constructor will point up some more instances there.  I have plans for solving this in another bug.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +6178,5 @@
>  
>    // Note that FindPreviousSibling is passed the iterator by value, so that
>    // the later usage of the iterator starts from the same place.
>    uint8_t childDisplay = UNSET_DISPLAY;
> +  nsIFrame* prevSibling = FindPreviousSibling(Move(iter), childDisplay);

This doesn't seem right, given the FindNextSibling call below, which expects iter to be in a proper state.

::: xpcom/base/nsAutoPtr.h
@@ -92,5 @@
> -  nsAutoPtr(nsAutoPtr<T>& aSmartPtr)
> -    : mRawPtr(aSmartPtr.forget())
> -    // Construct by transferring ownership from another smart pointer.
> -  {
> -  }

You probably want to make this private (and the const nsAutoPtr<T>&-taking version of this) and MOZ_DELETE them both.

::: xpcom/glue/nsThreadUtils.h
@@ +427,5 @@
>  NS_NewRunnableMethodWithArg(PtrType ptr, Method method, typename dependent_type<Arg>::type arg)
>  {
> +  return new nsRunnableMethodImpl<Method, Arg, true>(mozilla::Move(ptr),
> +                                                     method,
> +                                                     mozilla::Move(arg));

I think this is potentially surprising for non-nsAutoPtr usages, though perhaps mitigated by the need to explicitly instantiate with template arguments.  Are you able to point to places where this happens?
Attachment #8426546 - Flags: review?(nfroyd) → feedback+
> Did you compile this with --enable-webrtc?  Pretty sure applying the
> MOZ_DELETE treatment to the copy constructor will point up some more
> instances there.  I have plans for solving this in another bug.

its quiet possible I didn't (I hadn't run this by try yet) as I care more about incrementally removing some usage than the final removal of the operators.

> ::: layout/base/nsCSSFrameConstructor.cpp
> @@ +6178,5 @@
> >  
> >    // Note that FindPreviousSibling is passed the iterator by value, so that
> >    // the later usage of the iterator starts from the same place.
> >    uint8_t childDisplay = UNSET_DISPLAY;
> > +  nsIFrame* prevSibling = FindPreviousSibling(Move(iter), childDisplay);
> 
> This doesn't seem right, given the FindNextSibling call below, which expects
> iter to be in a proper state.

I didn't catch that, but that certainly seems funny.  On the other hand I think its equivalent to the previous code unless I'm missing something.

> ::: xpcom/base/nsAutoPtr.h
> @@ -92,5 @@
> > -  nsAutoPtr(nsAutoPtr<T>& aSmartPtr)
> > -    : mRawPtr(aSmartPtr.forget())
> > -    // Construct by transferring ownership from another smart pointer.
> > -  {
> > -  }
> 
> You probably want to make this private (and the const nsAutoPtr<T>&-taking
> version of this) and MOZ_DELETE them both.

can't hurt to make it clear though gcc atleast claims they're already implicitly deleted by the existance of the move operators.

> ::: xpcom/glue/nsThreadUtils.h
> @@ +427,5 @@
> >  NS_NewRunnableMethodWithArg(PtrType ptr, Method method, typename dependent_type<Arg>::type arg)
> >  {
> > +  return new nsRunnableMethodImpl<Method, Arg, true>(mozilla::Move(ptr),
> > +                                                     method,
> > +                                                     mozilla::Move(arg));
> 
> I think this is potentially surprising for non-nsAutoPtr usages, though
> perhaps mitigated by the need to explicitly instantiate with template
> arguments.  Are you able to point to places where this happens?

yes, content/media/fmp4/ffmpeg/FFmpegAACDecoder.cpp:114   I agree its kind of weird, but I think its actually always ok? if the argument has a normal copy ctor then your just moving from the local that was just coppied to to the function your calling, and nobody else knows about it.  If the copy ctor is actually a move ctor then it just got moved by the copy ctor and so its fine.  If you make the arg type a rval ref then you had to move into it, so the arg being moved again should be expected.  There may be some really strange things, but I think those are all the important cases?
(In reply to Trevor Saunders (:tbsaunde) from comment #12)
> > Did you compile this with --enable-webrtc?  Pretty sure applying the
> > MOZ_DELETE treatment to the copy constructor will point up some more
> > instances there.  I have plans for solving this in another bug.
> 
> its quiet possible I didn't (I hadn't run this by try yet) as I care more
> about incrementally removing some usage than the final removal of the
> operators.
> 
> > ::: layout/base/nsCSSFrameConstructor.cpp
> > @@ +6178,5 @@
> > >  
> > >    // Note that FindPreviousSibling is passed the iterator by value, so that
> > >    // the later usage of the iterator starts from the same place.
> > >    uint8_t childDisplay = UNSET_DISPLAY;
> > > +  nsIFrame* prevSibling = FindPreviousSibling(Move(iter), childDisplay);
> > 
> > This doesn't seem right, given the FindNextSibling call below, which expects
> > iter to be in a proper state.
> 
> I didn't catch that, but that certainly seems funny.  On the other hand I
> think its equivalent to the previous code unless I'm missing something.

so looking more closely the thing requiring the Move() is nsAutoPtr<ExplicitChildIterator> mShadowIterator in ExplicitChildIterator which appears to be only used for web components and maybe xbl? So I'm kind of prepared to believe this code has been busted all along and this patch just makes it more explicit.  I can certainly file a bug about this code though if you like.
Flags: needinfo?(nfroyd)
(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> so looking more closely the thing requiring the Move() is
> nsAutoPtr<ExplicitChildIterator> mShadowIterator in ExplicitChildIterator
> which appears to be only used for web components and maybe xbl? So I'm kind
> of prepared to believe this code has been busted all along and this patch
> just makes it more explicit.  I can certainly file a bug about this code
> though if you like.

I think it'd be worth filing a bug just to see what's really going on, yes.  Please CC me?
Flags: needinfo?(nfroyd)
Assignee: tbsaunde+mozbugs → nobody
You need to log in before you can comment on or make changes to this bug.