1241518 - remove nsAutoPtr usage inside xpcom/

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nathan Froyd [:froydnj]]

We'd like to remove nsAutoPtr entirely from the tree, as mozilla::UniquePtr (a polyfill for std::unique_ptr) has better, safer semantics and is identical with what the standard library implements.  We can start with some of the uses in xpcom:

https://dxr.mozilla.org/mozilla-central/search?q=ref%3AnsAutoPtr+path%3Axpcom%2F&redirect=false&case=false

Changing nsClassHashtable would require a substantial amount of refactoring that is out of scope for this bug.  We'd want to fix all the other files listed in that query (besides nsAutoPtr.h and TestAutoPtr.cpp itself, of course).

This change is a matter of:

1. Changing nsAutoPtr<T> to UniquePtr<T>
2. Make sure mozilla/UniquePtr.h is included if necessary

and then some combination of:

3. If we're constructing a UniquePtr like so:

UniquePtr<T> p(new T(...));

we should use MakeUnique instead:

auto p = MakeUnique<T>(...);

4. If we're constructing a UniquePtr from a function that returns a raw pointer, we should change that function to return UniquePtr instead.

5. Change .forget() calls on what were nsAutoPtr to .release() on UniquePtr...or use Move() if that seems more appropriate.

6. If we absolutely need the raw pointer for some reason, we'll need to insert .get() calls on UniquePtr, as UniquePtr<T> doesn't implicitly convert to T*.  Ideally we won't have to add many calls like that.

Everything else should just about work, I think.

Tyler, does this seem like something you're interested in doing?

Comment 1

[Tyler Steiman]

I'd love to work on this, thanks! I'm going to assign it to myself.

Is there a good way to test this when the changes are made?

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Tyler Steiman from comment #1)
> Is there a good way to test this when the changes are made?

I don't know that there's a good way to test this that will return pass/fail.  Some of this stuff is pretty basic to the browser and if you get it wrong, things are probably going to crash. :)

I'd suggest building and then running something like:

mach mochitest -f plain dom/base/
mach mochitest -f plain dom/indexedDB/

and if those don't crash (make sure you have a debug build!), then you're probably in pretty good shape.  A try run on Linux64 with ASan enabled will make sure that you don't have any use-after-free errors.

Comment 3

[Tyler Steiman]

Attachment: bug1241518_remove_nsAutoPtr.diff

This is just an initial patch to see if I'm making progress here. I (think) I've made the basic changes but it's definitely not compiling correctly.

What I'm not sure about:
I removed `nsAutoPtr.h` from the files, does that need to get put back in?

I tried to include mozilla/UniquePtr.h wherever nsAutoPtr was included, I need to make sure that's OK or not.

I wasn't sure how to deal with what you mentioned about functions that   return a raw pointer. I didn't really change anything in that area.

I was hoping an initial review would get me on the right track here.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8711161 [details] [diff] [review]
bug1241518_remove_nsAutoPtr.diff

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

::: xpcom/tests/gtest/TestCloneInputStream.cpp
@@ +144,5 @@
>    ASSERT_TRUE(NS_SUCCEEDED(rv));
>    testing::ConsumeAndValidateStream(clone, doubled);
>  
>    // Stream that has been read should fail.
> +  auto buffer = MakeUnique<char>(new char[512]);

Probably should be using MakeUnique<char[]>.

@@ +174,5 @@
>      ASSERT_TRUE(NS_SUCCEEDED(rv));
>    }
>  
>    // Fail when first stream read, but second hasn't been started.
> +  auto buffer = MakeUnique<char>(new char[1024]);

Same here.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8711161 [details] [diff] [review]
bug1241518_remove_nsAutoPtr.diff

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

Thanks for the patch and for explicitly noting that it was a work in progress!  I've pointed out some issues with the patch below that may be the source of your compiler errors.  If you're running into specific compiler errors that you don't understand, it can be helpful to include them; we've all run into weird compiler errors that we don't understand.

In addition to my comments below, Xidorn's comments are also relevant.

::: xpcom/base/nsCycleCollector.cpp
@@ +1272,5 @@
>    CycleCollectedJSRuntime* mJSRuntime;
>  
>    ccPhase mIncrementalPhase;
>    CCGraph mGraph;
> +  UniquePtr<CCGraphBuilder> mBuilder;

Assignment to this in nsCycleCollector::BeginCollection should use MakeUnique; UniquePtr doesn't permit assignment of raw pointers into it.  (You can do it by calling .reset(), but that's obviously uglier than using the assingment syntax.)

@@ +2047,5 @@
>    nsCycleCollectionParticipant* mJSZoneParticipant;
>    nsCString mNextEdgeName;
>    RefPtr<nsCycleCollectorLogger> mLogger;
>    bool mMergeZones;
> +  UniquePtr<NodePool::Enumerator> mCurrNode;

Likewise for this in CCGraphBuilder::DoneAddingRoots.

::: xpcom/build/MainThreadIOLogger.cpp
@@ +209,5 @@
>  
>  bool
>  Init()
>  {
> +  auto impl = MakeUnique<MainThreadIOLogger>(new MainThreadIOLogger);

This should be:

auto impl = MakeUnique<MainThreadIOLogger>();

::: xpcom/components/nsComponentManager.cpp
@@ +1661,5 @@
>      mContractIDs.Put(nsDependentCString(aContractID), oldf);
>      return NS_OK;
>    }
>  
> +  auto f = MakeUnique<nsFactoryEntry>(new nsFactoryEntry(aClass, aFactory));

This should be:

auto f = MakeUnique<nsFactoryEntry>(aClass, aFactory);

::: xpcom/components/nsComponentManager.h
@@ +262,5 @@
>    };
>  
>    // The KnownModule is kept alive by these members, it is
>    // referenced by pointer from the factory entries.
> +  nsTArray<UniquePtr<KnownModule>> mKnownStaticModules;

You may need to adjust how mKnownStaticModules is appended to.

::: xpcom/glue/BlockingResourceBase.cpp
@@ +279,1 @@
>      sDeadlockDetector->CheckAcquisition(

CheckAcquisition is one of those functions that should now return UniquePtr.

::: xpcom/io/nsInputStreamTee.cpp
@@ -12,5 @@
>  #include "nsIInputStreamTee.h"
>  #include "nsIInputStream.h"
>  #include "nsIOutputStream.h"
>  #include "nsCOMPtr.h"
> -#include "nsAutoPtr.h"

You will want to be careful about removing includes of nsAutoPtr.h; nsAutoPtr.h defines nsAutoPtr, but for historical reasons, it also pulls in a couple of other headers that files may implicitly depend on.  That could be a source of odd compiler errors.

@@ +52,5 @@
>    nsCOMPtr<nsIOutputStream> mSink;
>    nsCOMPtr<nsIEventTarget>  mEventTarget;
> +  nsWriteSegmentFun         mWriter;      // for implementing ReadSegments
> +  void*                     mClosure;     // for implementing ReadSegments
> +  UniquePtr<Mutex>          mLock;        // synchronize access to mSinkIsValid

Style note: in general, it's not a good idea to make changes to unrelated lines of source code--in this case, aligning the comments for the previous two members.  Reviewers will generally ask you to make changes like that if they deem it necessary.

::: xpcom/tests/TestHashtables.cpp
@@ +177,5 @@
>    return PL_DHASH_NEXT;
>  }
>  
>  PLDHashOperator
> +nsCEnum(const nsACString& aKey, UniquePtr<TestUniChar>& aData, void* userArg) {

Ah, so this shouldn't be changed because it's a callback:

http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHashtables.cpp#513

that's used for iterating over an nsClassHashtable:

http://mxr.mozilla.org/mozilla-central/source/xpcom/tests/TestHashtables.cpp#461

which uses nsAutoPtr internally:

http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsClassHashtable.h#23

so the changes to this file should be reverted.

::: xpcom/threads/MozPromise.h
@@ +921,5 @@
>    }
>  
>  private:
>    RefPtr<typename PromiseType::Private> mProxyPromise;
> +  UniquePtr<MethodCall<PromiseType, ThisType, ArgTypes...>> mMethodCall;

The constructor for this type should take a UniquePtr, which would then get Move()'d into mMethodCall.  Doing that means using UniquePtr in the call sites leading up to the constructor; we want to pass around as few raw pointers as possible.

::: xpcom/threads/nsThread.cpp
@@ +394,5 @@
>      }
>  #else
>      // Scope for MessageLoop.
> +    auto loop = MakeUnique<MessageLoop>(
> +        loop(new MessageLoop(MessageLoop::TYPE_MOZILLA_NONMAINTHREAD)));

As previously suggested, this is using MakeUnique incorrectly.

@@ +727,2 @@
>      *currentThread->mRequestedShutdownContexts.AppendElement();
>    context = new nsThreadShutdownContext();

This construction should be using MakeUnique.

@@ +1163,5 @@
>      return NS_ERROR_NULL_POINTER;
>    }
>  
>    // Don't delete or release anything while holding the lock.
> +  UniquePtr<nsChainedEventQueue> queue;

Wherever |queue| is constructed should also use MakeUnique.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Tyler Steiman from comment #3)
> This is just an initial patch to see if I'm making progress here. I (think)
> I've made the basic changes but it's definitely not compiling correctly.

Thanks.  Ideally the review comments will get you pointed in the right direction.

> What I'm not sure about:
> I removed `nsAutoPtr.h` from the files, does that need to get put back in?

Unless you're having compile errors related to nsAutoPtr or other things that get pulled in by nsAutoPtr.h (like nsCOMPtr, I believe), the removals are fine.

> I tried to include mozilla/UniquePtr.h wherever nsAutoPtr was included, I
> need to make sure that's OK or not.

Yes, that's correct; the new uses of UniquePtr should be able to see the declaration of UniquePtr and shouldn't rely on bootlegging it from other headers.

> I wasn't sure how to deal with what you mentioned about functions that  
> return a raw pointer. I didn't really change anything in that area.

I mentioned the only instance that I was aware of in the review.

Comment 7

[Tyler Steiman]

Sorry if this is taking me a bit this week I've been busy, just got a new job! But I plan on submitting another patch tomorrow.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Tyler Steiman from comment #7)
> Sorry if this is taking me a bit this week I've been busy, just got a new
> job! But I plan on submitting another patch tomorrow.

No problem!  Thanks for keeping us updated!

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Please note that I'm going to land bug 1241901 soon (later today or tomorrow for me), which would remove the necessity of your changes in TestCloneInputStream.cpp. You may want to leave that file untouched in your new patch.

Comment 10

[Nicholas Nethercote [inactive]]

I do a lot of this kind of refactoring. My approach:

- I don't do much testing locally. I just start the browser and see if nothing is obviously wrong.

- Most testing I do via try server. Build on all platforms and run all tests on one platform, usually Linux64, e.g.:

    try: -b do -p all -u all[x64] -t none[x64]

  OS X tends to have fewer intermittent oranges, which is nice, but the load on the OS X machines is often higher. (TryChooser has the load measurements if you want to see them.)

- Break the changes into a bunch of fairly small patches. This makes it easier to bisect if you introduced any test failures. It's demoralizing if you have a bunch of test failures on try server in a single large patch with 100 more-or-less independent changes.

Comment 11

[Eric Rahm [:erahm]]

Attachment: Bug 1241518 - Part 1: Remove unused nsAutoPtr.h includes in xpcom/

This removes various unused #include "nsAutoPtr.h" in xpcom/. Additionally
adds a few includes to the media stack.

Comment 12

[Eric Rahm [:erahm]]

Attachment: Bug 1241518 - Part 2: Use Maybe instead of nsAutoPtr in nsInputStreamTee

Comment 13

[Eric Rahm [:erahm]]

Attachment: Bug 1241518 - Part 3: Switch various nsAutoPtr uses to UniquePtr in xpcom/

This converts straightforward nsAutoPtr usage over to UniquePtr.
nsClassHashtable is left alone for now as that will have a larger impact.
nsThread is left alone for now as it has non-trivial ownership concepts.

Comment 14

[Eric Rahm [:erahm]]

Attachment: Bug 1241518 - Part 4: Switch nsThread to use UniquePtr

This switches over one usage of nsAutoPtr that was just used to scope an allocation, a stack variable is used instead. The shutdown contexts array is switched over to hold UniquePtrs which required adding a helper to find elements in the array as UniquePtr does not auto-convert to its pointer type.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77df306727e6
Part 1: Remove unused nsAutoPtr.h includes in xpcom/ r=kmag
https://hg.mozilla.org/integration/autoland/rev/0a5024ff6c8f
Part 2: Use Maybe instead of nsAutoPtr in nsInputStreamTee r=kmag
https://hg.mozilla.org/integration/autoland/rev/7ac9f823fae4
Part 3: Switch various nsAutoPtr uses to UniquePtr in xpcom/ r=kmag
https://hg.mozilla.org/integration/autoland/rev/0e8e08b054de
Part 4: Switch nsThread to use UniquePtr r=KrisWright,kmag

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/77df306727e6
https://hg.mozilla.org/mozilla-central/rev/0a5024ff6c8f
https://hg.mozilla.org/mozilla-central/rev/7ac9f823fae4
https://hg.mozilla.org/mozilla-central/rev/0e8e08b054de