864286 - Remove a lot of implicit conversions from raw pointer to already_AddRefed

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Comment 1

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

These should all or almost all be straightforward and not need module owner review.  If there are any specific changes you don't want to review, I can drop them from this patch and put them in other patches.  Also, if you don't want the patch, feel free to throw it at someone else.

(I only just now noticed that you can upload an attachment while filing a bug.  Hmm, was that always true?)

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to :Aryeh Gregor from comment #1)
> (I only just now noticed that you can upload an attachment while filing a
> bug.  Hmm, was that always true?)

AFAIK, yes.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 740244 [details] [diff] [review]
Patch

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

I got to see a lot of code I didn't really want to see... r=me with the leak and the crash fixed, at least.

::: accessible/src/generic/Accessible.h
@@ +150,5 @@
>     */
>    inline already_AddRefed<nsIDOMNode> DOMNode() const
>    {
> +    nsCOMPtr<nsIDOMNode> DOMNode = do_QueryInterface(GetNode());
> +    return DOMNode.forget();

Please file a bug to remove this function

::: content/base/src/Attr.cpp
@@ +141,5 @@
>      mNodeInfo->GetName(name);
>      nsAutoString lowercaseName;
>      nsContentUtils::ASCIIToLower(name, lowercaseName);
>      nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(lowercaseName);
> +    return nameAtom.forget();

This could be |return do_GetAtom(lowercaseName);|

::: content/base/src/nsAttrAndChildArray.cpp
@@ +208,5 @@
>  
>    memmove(pos, pos + 1, (childCount - aPos - 1) * sizeof(nsIContent*));
>    SetChildCount(childCount - 1);
>  
> +  return child.forget();

Just return dont_AddRef(child) here instead of adding the nsCOMPtr

::: content/base/src/nsNodeInfoManager.cpp
@@ +241,5 @@
>  
>    void *node = PL_HashTableLookup(mNodeInfoHash, &tmpKey);
>  
>    if (node) {
> +    nsCOMPtr<nsINodeInfo> nodeInfo = static_cast<nsINodeInfo *>(node);

No space before the *, please

::: content/media/webspeech/synth/nsSynthVoiceRegistry.cpp
@@ +520,4 @@
>    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>      task = new SpeechTaskChild(&aUtterance);
>      SpeechSynthesisRequestChild* actor =
> +      new SpeechSynthesisRequestChild(static_cast<SpeechTaskChild*>(task.get()));

:/

::: content/xul/document/src/XULDocument.cpp
@@ +1253,1 @@
>                                              MatchAttribute,

Indentation

@@ +1296,1 @@
>                                              MatchAttribute,

Ditto

::: gfx/layers/opengl/ImageLayerOGL.cpp
@@ +97,5 @@
>        mContext->MakeCurrent();
>        mContext->fDeleteTextures(1, &mTexture);
>      } else {
>        nsCOMPtr<nsIRunnable> runnable =
> +        new TextureDeleter(mContext.forget(), mTexture);

This is wrong. You'll crash on the next line. bz suggests:

{
  already_AddRefed<GLContext> context = mContext.forget();
  nsCOMPtr<nsIRunnable> runnable = new TextureDeleter(context, mTexture);
  context.get()->DispatchToOwningThread(runnable);
}

::: gfx/thebes/gfxContext.cpp
@@ +148,5 @@
>      if (s == mSurface->CairoSurface()) {
>          if (dx && dy)
>              cairo_surface_get_device_offset(s, dx, dy);
> +        nsRefPtr<gfxASurface> ret = mSurface;
> +        return ret.forget();

Indentation is wacky here, but don't worry about it.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +4142,5 @@
>    aNewFrame = gfxScrollFrame;
>  
>    // we used the style that was passed in. So resolve another one.
>    nsStyleSet *styleSet = mPresShell->StyleSet();
> +  nsRefPtr<nsStyleContext> aScrolledChildStyle =

Not going to object if you make this 'scrolledChildStyle' while you're here...

@@ +4147,1 @@
>      styleSet->ResolveAnonymousBoxStyle(aScrolledPseudo, contentStyle).get();

Leak! Drop the .get() here.

::: layout/base/nsPresContext.cpp
@@ +1459,5 @@
>  
>  already_AddRefed<nsISupports>
>  nsPresContext::GetContainerInternal() const
>  {
> +  nsCOMPtr<nsISupports> result = do_QueryReferent(mContainer.get());

Do you need the .get()?

::: layout/base/nsPresShell.cpp
@@ +4773,5 @@
>    // restore the old selection display state
>    frameSelection->SetDisplaySelection(oldDisplaySelection);
>  
> +  nsRefPtr<gfxASurface> ret = surface;
> +  return ret.forget();

Please make the surface variable a strong pointer and remove the 'delete surface' in the surface->CairoStatus() block.

::: media/webrtc/signaling/test/FakeMediaStreams.h
@@ +197,5 @@
>    static already_AddRefed<Fake_DOMMediaStream>
>    CreateSourceStream(nsIDOMWindow* aWindow, uint32_t aHintContents) {
>      Fake_SourceMediaStream *source = new Fake_SourceMediaStream();
>  
> +    nsCOMPtr<Fake_DOMMediaStream> ds = new Fake_DOMMediaStream(source);

nsRefPtr

::: netwerk/base/public/nsNetUtil.h
@@ +1665,3 @@
>   */
>  inline already_AddRefed<nsIURI>
>  NS_GetInnermostURI(nsIURI *uri)

Please call the argument aURI and make uri a local nsCOMPtr.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +1863,5 @@
>    // AddRef for the local reference
>    NS_ADDREF(sTelemetry);
>    // AddRef for the caller
>    NS_ADDREF(sTelemetry);
> +  return already_AddRefed<nsITelemetry>(sTelemetry);

An nsCOMPtr for the latter NS_ADDREF, please.

::: toolkit/components/typeaheadfind/nsTypeAheadFind.cpp
@@ +1223,4 @@
>    if (shell) {
>      nsPresContext *pc = shell->GetPresContext();
>      if (!pc || !nsCOMPtr<nsISupports>(pc->GetContainer())) {
> +      shell = nullptr;

Just 'return nullptr;', I'd say.

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/447cf900cd4f

Green try: https://tbpl.mozilla.org/?tree=Try&rev=0a84b658935c

Comment 5

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

This was actually fixed, just the bug wasn't closed because the bug number was wrong:

https://hg.mozilla.org/mozilla-central/rev/447cf900cd4f

This is in 23, right?