1492743 - Abuses of already_Addrefed

Status: RESOLVED INCOMPLETE

(Core :: General, enhancement, P3)

Description

[Mike Hommey [:glandium]]

Bug 1418854 added a misuse of already_Addrefed. That should have been caught by static analysis, but for some reason it wasn't. It's only caught with clang 7 after fixing bug 1479232.

already_Addrefed is meant to be used for temporaries, but bug 1418854 added code that doesn't use it that way.

Comment 1

[Mike Hommey [:glandium]]

There are, actually, much more than just in Autoclose.h. A stupid regexp search finds plenty.
https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%22already_AddRefed%3C%5B%5E%3E%5D*%3E+%5Ba-zA-Z%5D*+%3D%22&redirect=false

Comment 2

[Mike Hommey [:glandium]]

It seems at least part of the reason for those things to happen is that numerous methods take an already_AddRefed<T>& as argument. And I'm not sure what that is supposed to achieve.

It's getting too late for me too look at this further. Nika, Nathan, any opinion on this mess? This is preventing a compiler upgrade on non-Windows platforms. The short summary is that essentially, the annotation from bug 1443265 didn't really do its job.

Comment 3

[Nathan Froyd [:froydnj]]

It looks like the analysis doesn't check for a pattern (as seen in a lot of DOM code) like:

TempClass t = f();

or at least there are no tests verifying that pattern fails.

Comment 4

[Nathan Froyd [:froydnj]]

Presumably we also shouldn't permit parameters of `TempClass&` and variants?  Or maybe `const TempClass&` would be OK?

Comment 5

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #3)
> It looks like the analysis doesn't check for a pattern (as seen in a lot of
> DOM code) like:
> 
> TempClass t = f();

As per comment 0, the check didn't work for some reason... until clang 7. And this bug was meant to address those patterns, which you now moved to bug 1492894.

> or at least there are no tests verifying that pattern fails.

Let's recycle this bug for that, then.

Comment 6

[Mike Hommey [:glandium]]

Actually, let's recycle this bug for *other* abuses of already_AddRefed, because there are :(

Comment 7

[Mike Hommey [:glandium]]

Unfortunately, because of bug 1493051, the error messages are not as useful as they could be, but here's a complete list of what I got after applying the patches from bug 1492894, and turning the errors into warnings so that a full build would go through:

 In file included from /builds/worker/workspace/build/src/obj-firefox/media/webrtc/signaling/src/peerconnection/Unified_cpp_src_peerconnection0.cpp:2:
 In file included from /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:7:
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/media/MediaUtils.h:219:31: warning: variable of type 'LambdaType' (aka 'LambdaRunnable<std::_Bind<(lambda at /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:30:11) (already_AddRefed<mozilla::PeerConnectionImpl>)> >') is only valid as a temporary
   RefPtr<LambdaType> lambda = new LambdaType(std::forward<OnRunType>(aOnRun));
                               ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/media/MediaUtils.h:219:31: note: value incorrectly allocated on the heap
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/media/MediaUtils.h:211:13: note: 'LambdaType' (aka 'LambdaRunnable<std::_Bind<(lambda at /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:30:11) (already_AddRefed<mozilla::PeerConnectionImpl>)> >') is a temporary type because member 'mOnRun' is a temporary type 'std::_Bind<(lambda at /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:30:11) (already_AddRefed<mozilla::PeerConnectionImpl>)>'
   OnRunType mOnRun;
             ^
 /builds/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/functional:1256:29: note: 'std::_Bind<(lambda at /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PacketDumper.cpp:30:11) (already_AddRefed<mozilla::PeerConnectionImpl>)>' is a temporary type because member '_M_bound_args' is a temporary type 'tuple<already_AddRefed<mozilla::PeerConnectionImpl> >'
       tuple<_Bound_args...> _M_bound_args;
                             ^
 /builds/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/tuple:390:11: note: 'tuple<already_AddRefed<mozilla::PeerConnectionImpl> >' is a temporary type because it inherits from a temporary type '_Tuple_impl<0, already_AddRefed<mozilla::PeerConnectionImpl> >'
     class tuple : public _Tuple_impl<0, _Elements...>
           ^
 /builds/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/tuple:231:12: note: '_Tuple_impl<0, already_AddRefed<mozilla::PeerConnectionImpl> >' is a temporary type because it inherits from a temporary type '_Head_base<0UL, already_AddRefed<mozilla::PeerConnectionImpl>, __empty_not_final<already_AddRefed<PeerConnectionImpl> >::value>'
     struct _Tuple_impl<_Idx, _Head, _Tail...>
            ^
 /builds/worker/workspace/build/src/clang/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.9.4/../../../../include/c++/4.9.4/tuple:174:13: note: '_Head_base<0UL, already_AddRefed<mozilla::PeerConnectionImpl>, __empty_not_final<already_AddRefed<PeerConnectionImpl> >::value>' is a temporary type because member '_M_head_impl' is a temporary type 'already_AddRefed<mozilla::PeerConnectionImpl>'
       _Head _M_head_impl;
             ^
 In file included from /builds/worker/workspace/build/src/obj-firefox/media/webrtc/signaling/src/peerconnection/Unified_cpp_src_peerconnection0.cpp:11:
 In file included from /builds/worker/workspace/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp:9:
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:116:10: warning: variable of type 'runnable_args_func<void (*)(const RefPtr<nsDOMDataChannel> &, const RefPtr<mozilla::dom::PeerConnectionObserver> &), typename mozilla::Decay<already_AddRefed<nsDOMDataChannel> >::Type, typename mozilla::Decay<RefPtr<PeerConnectionObserver> &>::Type>' (aka 'runnable_args_func<void (*)(const RefPtr<nsDOMDataChannel> &, const RefPtr<mozilla::dom::PeerConnectionObserver> &), already_AddRefed<nsDOMDataChannel>, RefPtr<mozilla::dom::PeerConnectionObserver> >') is only valid as a temporary
   return new runnable_args_func<FunType, typename mozilla::Decay<Args>::Type...>(f, std::forward<Args>(args)...);
          ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:116:10: note: value incorrectly allocated on the heap
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mtransport/runnable_utils.h:109:18: note: 'runnable_args_func<void (*)(const RefPtr<nsDOMDataChannel> &, const RefPtr<mozilla::dom::PeerConnectionObserver> &), typename mozilla::Decay<already_AddRefed<nsDOMDataChannel> >::Type, typename mozilla::Decay<RefPtr<PeerConnectionObserver> &>::Type>' (aka 'runnable_args_func<void (*)(const RefPtr<nsDOMDataChannel> &, const RefPtr<mozilla::dom::PeerConnectionObserver> &), already_AddRefed<nsDOMDataChannel>, RefPtr<mozilla::dom::PeerConnectionObserver> >') is a temporary type because member 'mArgs' is a temporary type 'Tuple<already_AddRefed<nsDOMDataChannel>, RefPtr<mozilla::dom::PeerConnectionObserver> >'
   Tuple<Args...> mArgs;
                  ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:293:7: note: 'Tuple<already_AddRefed<nsDOMDataChannel>, RefPtr<mozilla::dom::PeerConnectionObserver> >' is a temporary type because it inherits from a temporary type 'detail::TupleImpl<0, already_AddRefed<nsDOMDataChannel>, RefPtr<PeerConnectionObserver> >'
 class Tuple<A, B> : public detail::TupleImpl<0, A, B>
       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:219:9: note: 'detail::TupleImpl<0, already_AddRefed<nsDOMDataChannel>, RefPtr<PeerConnectionObserver> >' is a temporary type because member 'mHead' is a temporary type 'already_AddRefed<nsDOMDataChannel>'
   HeadT mHead;  // The element stored at this index in the tuple.
         ^

 In file included from /builds/worker/workspace/build/src/obj-firefox/gfx/layers/Unified_cpp_gfx_layers2.cpp:2:
 In file included from /builds/worker/workspace/build/src/gfx/layers/TextureSourceProvider.cpp:8:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/TextureHost.h:34:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h:14:
 /builds/worker/workspace/build/src/ipc/chromium/src/base/task.h:377:5: warning: variable of type 'RunnableFunction<void (*)(already_AddRefed<mozilla::Runnable>), ArgsTuple>' (aka 'RunnableFunction<void (*)(already_AddRefed<mozilla::Runnable>), Tuple<already_AddRefed<mozilla::Runnable> > >') is only valid as a temporary
     new RunnableFunction<Function, ArgsTuple>(name, function,
     ^
 /builds/worker/workspace/build/src/ipc/chromium/src/base/task.h:377:5: note: value incorrectly allocated on the heap
 /builds/worker/workspace/build/src/ipc/chromium/src/base/task.h:359:10: note: 'RunnableFunction<void (*)(already_AddRefed<mozilla::Runnable>), ArgsTuple>' (aka 'RunnableFunction<void (*)(already_AddRefed<mozilla::Runnable>), Tuple<already_AddRefed<mozilla::Runnable> > >') is a temporary type because member 'params_' is a temporary type 'mozilla::Tuple<already_AddRefed<mozilla::Runnable> >'
   Params params_;
          ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:233:7: note: 'mozilla::Tuple<already_AddRefed<mozilla::Runnable> >' is a temporary type because it inherits from a temporary type 'detail::TupleImpl<0, already_AddRefed<Runnable> >'
 class Tuple : public detail::TupleImpl<0, Elements...>
       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:219:9: note: 'detail::TupleImpl<0, already_AddRefed<Runnable> >' is a temporary type because member 'mHead' is a temporary type 'already_AddRefed<mozilla::Runnable>'
   HeadT mHead;  // The element stored at this index in the tuple.
         ^

 In file included from /builds/worker/workspace/build/src/obj-firefox/dom/media/Unified_cpp_dom_media3.cpp:20:
 /builds/worker/workspace/build/src/dom/media/ChannelMediaDecoder.cpp:254:3: warning: variable of type 'mozilla::MediaFormatReaderInit' is only valid as a temporary
   MediaFormatReaderInit init;
   ^
 /builds/worker/workspace/build/src/dom/media/ChannelMediaDecoder.cpp:254:3: note: value incorrectly allocated in an automatic variable
 /builds/worker/workspace/build/src/dom/media/MediaFormatReader.h:84:45: note: 'mozilla::MediaFormatReaderInit' is a temporary type because member 'mKnowsCompositor' is a temporary type 'already_AddRefed<layers::KnowsCompositor>'
   already_AddRefed<layers::KnowsCompositor> mKnowsCompositor;
                                             ^

 In file included from /builds/worker/workspace/build/src/obj-firefox/dom/media/Unified_cpp_dom_media5.cpp:2:
 In file included from /builds/worker/workspace/build/src/dom/media/MediaCache.cpp:7:
 In file included from /builds/worker/workspace/build/src/dom/media/MediaCache.h:10:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/DecoderDoctorLogger.h:15:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:21:
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1513:5: warning: variable of type 'detail::OwningRunnableMethodImpl<RefPtr<MediaFormatReader> &, void (MediaFormatReader::*)(already_AddRefed<KnowsCompositor>), already_AddRefed<KnowsCompositor> &&>' (aka 'mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::MediaFormatReader>, void (mozilla::MediaFormatReader::*)(already_AddRefed<mozilla::layers::KnowsCompositor>), true, mozilla::RunnableKind::Standard, already_AddRefed<mozilla::layers::KnowsCompositor> &&>') is only valid as a temporary
     new detail::OwningRunnableMethodImpl<PtrType, Method, Storages...>(
     ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1513:5: note: value incorrectly allocated on the heap
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1199:40: note: 'detail::OwningRunnableMethodImpl<RefPtr<MediaFormatReader> &, void (MediaFormatReader::*)(already_AddRefed<KnowsCompositor>), already_AddRefed<KnowsCompositor> &&>' (aka 'mozilla::detail::RunnableMethodImpl<RefPtr<mozilla::MediaFormatReader>, void (mozilla::MediaFormatReader::*)(already_AddRefed<mozilla::layers::KnowsCompositor>), true, mozilla::RunnableKind::Standard, already_AddRefed<mozilla::layers::KnowsCompositor> &&>') is a temporary type because member 'mArgs' is a temporary type 'RunnableMethodArguments<already_AddRefed<mozilla::layers::KnowsCompositor> &&>'
   RunnableMethodArguments<Storages...> mArgs;
                                        ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1168:59: note: 'RunnableMethodArguments<already_AddRefed<mozilla::layers::KnowsCompositor> &&>' is a temporary type because member 'mArguments' is a temporary type 'Tuple<typename ::detail::ParameterStorage<already_AddRefed<KnowsCompositor> &&>::Type>' (aka 'Tuple<StoreCopyPassByRRef<already_AddRefed<mozilla::layers::KnowsCompositor> > >')
   Tuple<typename ::detail::ParameterStorage<Ts>::Type...> mArguments;
                                                           ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:233:7: note: 'Tuple<typename ::detail::ParameterStorage<already_AddRefed<KnowsCompositor> &&>::Type>' (aka 'Tuple<StoreCopyPassByRRef<already_AddRefed<mozilla::layers::KnowsCompositor> > >') is a temporary type because it inherits from a temporary type 'detail::TupleImpl<0, StoreCopyPassByRRef<already_AddRefed<KnowsCompositor> > >'
 class Tuple : public detail::TupleImpl<0, Elements...>
       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:219:9: note: 'detail::TupleImpl<0, StoreCopyPassByRRef<already_AddRefed<KnowsCompositor> > >' is a temporary type because member 'mHead' is a temporary type 'StoreCopyPassByRRef<already_AddRefed<mozilla::layers::KnowsCompositor> >'
   HeadT mHead;  // The element stored at this index in the tuple.
         ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:925:15: note: 'StoreCopyPassByRRef<already_AddRefed<mozilla::layers::KnowsCompositor> >' is a temporary type because member 'm' is a temporary type 'StoreCopyPassByRRef<already_AddRefed<mozilla::layers::KnowsCompositor> >::stored_type' (aka 'already_AddRefed<mozilla::layers::KnowsCompositor>')
   stored_type m;
               ^

 In file included from /builds/worker/workspace/build/src/obj-firefox/dom/media/mediasource/Unified_cpp_media_mediasource0.cpp:2:
 In file included from /builds/worker/workspace/build/src/dom/media/mediasource/ContainerParser.cpp:7:
 In file included from /builds/worker/workspace/build/src/dom/media/mediasource/ContainerParser.h:11:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/MediaContainerType.h:10:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/MediaMIMETypes.h:10:
 In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/VideoUtils.h:18:
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:1433:5: warning: variable of type 'MethodCallType' (aka 'MethodCall<mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>, RefPtr<mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true> > (mozilla::TrackBuffersManager::*)(already_AddRefed<mozilla::MediaByteBuffer>, const mozilla::SourceBufferAttributes &), mozilla::TrackBuffersManager, StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >, StoreCopyPassByRRef<mozilla::SourceBufferAttributes> >') is only valid as a temporary
     new MethodCallType(aMethod, aThisVal, std::forward<ActualArgTypes>(aArgs)...);
     ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:1433:5: note: value incorrectly allocated on the heap
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/MozPromise.h:1382:40: note: 'MethodCallType' (aka 'MethodCall<mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true>, RefPtr<mozilla::MozPromise<mozilla::Pair<bool, mozilla::SourceBufferAttributes>, mozilla::MediaResult, true> > (mozilla::TrackBuffersManager::*)(already_AddRefed<mozilla::MediaByteBuffer>, const mozilla::SourceBufferAttributes &), mozilla::TrackBuffersManager, StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >, StoreCopyPassByRRef<mozilla::SourceBufferAttributes> >') is a temporary type because member 'mArgs' is a temporary type 'RunnableMethodArguments<StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >, StoreCopyPassByRRef<mozilla::SourceBufferAttributes> >'
   RunnableMethodArguments<Storages...> mArgs;
                                        ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1168:59: note: 'RunnableMethodArguments<StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >, StoreCopyPassByRRef<mozilla::SourceBufferAttributes> >' is a temporary type because member 'mArguments' is a temporary type 'Tuple<typename ::detail::ParameterStorage<StoreCopyPassByRRef<already_AddRefed<MediaByteBuffer> > >::Type, typename ::detail::ParameterStorage<StoreCopyPassByRRef<SourceBufferAttributes> >::Type>' (aka 'Tuple<StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >, StoreCopyPassByRRef<mozilla::SourceBufferAttributes> >')
   Tuple<typename ::detail::ParameterStorage<Ts>::Type...> mArguments;
                                                           ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:293:7: note: 'Tuple<typename ::detail::ParameterStorage<StoreCopyPassByRRef<already_AddRefed<MediaByteBuffer> > >::Type, typename ::detail::ParameterStorage<StoreCopyPassByRRef<SourceBufferAttributes> >::Type>' (aka 'Tuple<StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >, StoreCopyPassByRRef<mozilla::SourceBufferAttributes> >') is a temporary type because it inherits from a temporary type 'detail::TupleImpl<0, StoreCopyPassByRRef<already_AddRefed<MediaByteBuffer> >, StoreCopyPassByRRef<SourceBufferAttributes> >'
 class Tuple<A, B> : public detail::TupleImpl<0, A, B>
       ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Tuple.h:219:9: note: 'detail::TupleImpl<0, StoreCopyPassByRRef<already_AddRefed<MediaByteBuffer> >, StoreCopyPassByRRef<SourceBufferAttributes> >' is a temporary type because member 'mHead' is a temporary type 'StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >'
   HeadT mHead;  // The element stored at this index in the tuple.
         ^
 /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:925:15: note: 'StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >' is a temporary type because member 'm' is a temporary type 'StoreCopyPassByRRef<already_AddRefed<mozilla::MediaByteBuffer> >::stored_type' (aka 'already_AddRefed<mozilla::MediaByteBuffer>')
   stored_type m;
               ^
 In file included from /builds/worker/workspace/build/src/obj-firefox/dom/media/mediasource/Unified_cpp_media_mediasource0.cpp:20:
 /builds/worker/workspace/build/src/dom/media/mediasource/MediaSourceDecoder.cpp:51:3: warning: variable of type 'mozilla::MediaFormatReaderInit' is only valid as a temporary
   MediaFormatReaderInit init;
   ^
 /builds/worker/workspace/build/src/dom/media/mediasource/MediaSourceDecoder.cpp:51:3: note: value incorrectly allocated in an automatic variable
 /builds/worker/workspace/build/src/obj-firefox/dist/include/MediaFormatReader.h:84:45: note: 'mozilla::MediaFormatReaderInit' is a temporary type because member 'mKnowsCompositor' is a temporary type 'already_AddRefed<layers::KnowsCompositor>'
   already_AddRefed<layers::KnowsCompositor> mKnowsCompositor;
                                             ^

 In file included from /builds/worker/workspace/build/src/obj-firefox/dom/media/webaudio/Unified_cpp_dom_media_webaudio1.cpp:119:
 /builds/worker/workspace/build/src/dom/media/webaudio/MediaBufferDecoder.cpp:185:3: warning: variable of type 'mozilla::MediaFormatReaderInit' is only valid as a temporary
   MediaFormatReaderInit init;
   ^
 /builds/worker/workspace/build/src/dom/media/webaudio/MediaBufferDecoder.cpp:185:3: note: value incorrectly allocated in an automatic variable
 /builds/worker/workspace/build/src/dom/media/MediaFormatReader.h:84:45: note: 'mozilla::MediaFormatReaderInit' is a temporary type because member 'mKnowsCompositor' is a temporary type 'already_AddRefed<layers::KnowsCompositor>'
   already_AddRefed<layers::KnowsCompositor> mKnowsCompositor;
                                             ^

Comment 8

[Mike Hommey [:glandium]]

Interestingly, dom/media/MediaFormatReader.h contains:
struct MOZ_STACK_CLASS MediaFormatReaderInit
{
  MediaResource* mResource = nullptr;
  VideoFrameContainer* mVideoFrameContainer = nullptr;
  FrameStatistics* mFrameStats = nullptr;
  already_AddRefed<layers::KnowsCompositor> mKnowsCompositor;
  already_AddRefed<GMPCrashHelper> mCrashHelper;
  // Used in bug 1393399 for temporary telemetry.
  MediaDecoderOwnerID mMediaDecoderOwnerID = nullptr;
};

There are, actually, *two* already_AddRefed members, but static analysis only complains about one.

Comment 9

[Mike Hommey [:glandium]]

Nathan, if you want to take a look at comment 7... all those uses of already_AddRefed with Runnables make my head hurt. Kind of related: bug 1268314, which would probably make things even worse than they currently are.

Comment 10

[Nathan Froyd [:froydnj]]

We have several bugs open on this stuff already:

- merging runnable_utils and NS_NewRunnableMethod et al: bug 1174274 and bug 1107585
- already_AddRefed members in runnable_utils: bug 968800
- already_AddRefed members in classes generally: bug 968799 (in which smaug points out that already_AddRefed is somewhat easier to deal with vs. nsCOMPtr when you're dealing with threads)

I don't know how to do the first one (which would, by extension, handle the second one) without one of the runnable implementations giving up something:

- runnable_utils giving up implicit instantiation of templates (which, IMHO, leads to some peculiar code and non-obvious behavior), which at least ekr is strongly against; or
- NS_NewRunnableMethod giving up explicit instantiation of templates.

bsmedberg was strongly in favor of explicit instantiation.  I'm not sure that it's as important given that we have lambdas and NS_NewRunnableFunction and that *seems* to be working out OK.  But I think being explicit about passing data across threads is still a good idea.

If people are using NS_NewRunnableMethod with already_AddRefed arguments, that just seems wrong, and straightforward to fix.

Dealing with the explicit already_AddRefed members seems a little easier, though perhaps we have to think through some of the behavior with threads a little carefully when writing the patches.

Comment 11

[Mike Hommey [:glandium]]

(In reply to Nathan Froyd [:froydnj] from comment #10)
> We have several bugs open on this stuff already:
> 
> - merging runnable_utils and NS_NewRunnableMethod et al: bug 1174274 and bug
> 1107585
> - already_AddRefed members in runnable_utils: bug 968800
> - already_AddRefed members in classes generally: bug 968799 (in which smaug
> points out that already_AddRefed is somewhat easier to deal with vs.
> nsCOMPtr when you're dealing with threads)

So, what you're saying, essentially, is that bug 1443265 was premature, and it turns out it only "worked" because static analysis missed plenty of errors. Would it be reasonable to backout bug 1443265 to unblock bug 1492663?

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Mike Hommey [:glandium] from comment #11)
> (In reply to Nathan Froyd [:froydnj] from comment #10)
> > We have several bugs open on this stuff already:
> > 
> > - merging runnable_utils and NS_NewRunnableMethod et al: bug 1174274 and bug
> > 1107585
> > - already_AddRefed members in runnable_utils: bug 968800
> > - already_AddRefed members in classes generally: bug 968799 (in which smaug
> > points out that already_AddRefed is somewhat easier to deal with vs.
> > nsCOMPtr when you're dealing with threads)
> 
> So, what you're saying, essentially, is that bug 1443265 was premature, and
> it turns out it only "worked" because static analysis missed plenty of
> errors. Would it be reasonable to backout bug 1443265 to unblock bug 1492663?

r=me on the backout.  Unfortunate, but if it's basically useless atm, no reason to block other useful work.

Comment 13

[Mike Hommey [:glandium]]

I found what was going on before clang 7: only the last annotation was seen by the plugin (it was eaten by clang, afaict). In the case of already_AddRefed, it means it was only MOZ_NON_AUTOABLE, and neither MOZ_TEMPORARY_CLASS nor MOZ_MUST_USE_TYPE had any effect. So, for clang < 7 builds, backing out bug 1443265 is actually a no-op.

Considering the above, it's incredible that there aren't more errors from more annotations being enabled.

Comment 14

[(Away)]

(In reply to Mike Hommey [:glandium] from comment #13)
> I found what was going on before clang 7: only the last annotation was seen
> by the plugin (it was eaten by clang, afaict). In the case of
> already_AddRefed, it means it was only MOZ_NON_AUTOABLE, and neither
> MOZ_TEMPORARY_CLASS nor MOZ_MUST_USE_TYPE had any effect. So, for clang < 7
> builds, backing out bug 1443265 is actually a no-op.

Nice find! Do you happen to know which revision fixed it?

> Considering the above, it's incredible that there aren't more errors from
> more annotations being enabled.

Bug 1438446 is another, but it's Windows-only.

Comment 15

[Mike Hommey [:glandium]]

(In reply to David Major [:dmajor] from comment #14)
> (In reply to Mike Hommey [:glandium] from comment #13)
> > I found what was going on before clang 7: only the last annotation was seen
> > by the plugin (it was eaten by clang, afaict). In the case of
> > already_AddRefed, it means it was only MOZ_NON_AUTOABLE, and neither
> > MOZ_TEMPORARY_CLASS nor MOZ_MUST_USE_TYPE had any effect. So, for clang < 7
> > builds, backing out bug 1443265 is actually a no-op.
> 
> Nice find! Do you happen to know which revision fixed it?

I haven't checked the changes between clang 6 and 7 that might have fixed this. I just checked what can be seen by scanning the AST through the API with both versions. Interestingly, using the AST viewer on godbolt doesn't show the same problem.

Comment 16

[Selena Deckelmann :selenamarie :selena]

:glandium can you take ownership of this bug, or designate a good owner? Thank you!

Comment 17

[Mike Hommey [:glandium]]

With bug 1443265 being reopened and other bugs already existing for the various issues that the plugin finds, I think there's nothing left for this bug. Not sure which bug status is the most appropriate... picking incomplete.