896648 - Make conversion operator of ref pointers safer

Status: NEW

(Core :: XPCOM, defect)

Description

[Joshua Cranmer [:jcranmer]]

As discussed in the mailing list, right now, nsRefPtr/nsCOMPtr/etc. all allow implicit conversion operators to T*. This is basically required by XPIDL semantics, so we can't change it easily, but it means we can't distinguish between a reference count that is about to deleted and one whose lifetime will extend past a function call.

N2439 lets us constrain functions by whether this is an lvalue or an rvalue, but it is nowhere near universally available, so we could do something like this:

class nsRefPtr {
...
#if defined(__has_feature) && __has_feature(cxx_reference_qualified_functions)
  operator T*() const && MOZ_DELETE;
  operator T*() const &
#else
  operator T*() const
#endif
  {
    return get();
  }
};

Comment 1

[(no longer active)]

This code fails to compile with a patch similar to the above:

http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp#716

Comment 2

[(no longer active)]

Attachment: Patch

Comment 3

[(no longer active)]

 5:00.38 ../../../../../media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp:716:17: error: no matching function for call to 'RUN_ON_THREAD'
 5:00.38   nsresult rv = RUN_ON_THREAD(pc.impl()->media()->ice_ctx()->thread(),
 5:00.38                 ^~~~~~~~~~~~~
 5:00.38 ../../../../../media/mtransport/runnable_utils.h:62:24: note: candidate function not viable: no known conversion from 'nsCOMPtr<nsIEventTarget>' to 'nsIEventTarget *' for 1st argument
 5:00.38 static inline nsresult RUN_ON_THREAD(nsIEventTarget *thread, runnable_args_base *runnable, uint32_t flags) {
 5:00.38                        ^
 5:00.38 ../../../../../media/mtransport/runnable_utils.h:42:24: note: candidate function not viable: no known conversion from 'nsCOMPtr<nsIEventTarget>' to 'nsIEventTarget *' for 1st argument
 5:00.38 static inline nsresult RUN_ON_THREAD(nsIEventTarget *thread, nsIRunnable *runnable, uint32_t flags) {
 5:00.38                        ^
 5:00.38 1 error generated.
 5:00.38 
 5:00.38 In the directory  /Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/media/webrtc/signaling/signaling_ecc
 5:00.38 The following command failed to execute properly:
 5:00.39 /opt/bin/ccache /usr/local/bin/clang++ -o src/media/VcmSIPCCBinding.o -c -fvisibility=hidden -DNO_NSPR_10_SUPPORT -DOS_POSIX=1 -DOS_MACOSX=1 -DCHROMIUM_BUILD -DUSE_LIBJPEG_TURBO=1 -DENABLE_ONE_CLICK_SIGNIN -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DENABLE_CONFIGURATION_POLICY -DENABLE_INPUT_SPEECH -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DENABLE_GPU=1 -DUSE_OPENSSL=1 -DENABLE_EGLIMAGE=1 -DUSE_SKIA=1 -DENABLE_TASK_MANAGER=1 -DENABLE_WEB_INTENTS=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGIN_INSTALLATION=1 -DENABLE_PROTECTOR_SERVICE=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_BACKGROUND=1 -DENABLE_AUTOMATION=1 -DENABLE_PRINTING=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DLOG4CXX_STATIC -D_NO_LOG4CXX -DUSE_SSLEAY -D_CPR_USE_EXTERNAL_LOGGER -DWEBRTC_RELATIVE_PATH -DHAVE_WEBRTC_VIDEO -DHAVE_WEBRTC_VOICE -DHAVE_STDLIB_H=1 -DINTEGER_TYPES_H="mozilla/StandardInteger.h" -DHAVE_UINT8_T=1 -DHAVE_UINT16_T=1 -DHAVE_UINT32_T=1 -DHAVE_UINT64_T=1 -DMOZILLA_INTERNAL_API -DSIP_OS_OSX -DOSX -D_FORTIFY_SOURCE=2 -D__STDC_FORMAT_MACROS -DDYNAMIC_ANNOTATIONS_ENABLED=1 -DWTF_USE_DYNAMIC_ANNOTATIONS=1 -I. -I/Users/ehsanakhgari/moz/mozilla-central/ipc/chromium/src -I/Users/ehsanakhgari/moz/mozilla-central/ipc/glue -I../../../../ipc/ipdl/_ipdlheaders -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/.. -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/callcontrol -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/common -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/common/browser_logging -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/media -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/media-conduit -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/mediapipeline -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/softphonewrapper -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/peerconnection -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/sipcc/include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/./src/sipcc/cpr/include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../ipc/chromium/src -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../ipc/chromium/src/base/third_party/nspr -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../xpcom/base -I../../../../dist/include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../dom/base -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../content/media -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../media/mtransport -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../trunk -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../trunk/webrtc -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../trunk/webrtc/video_engine/include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../trunk/webrtc/voice_engine/include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../trunk/webrtc/modules/interface -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../trunk/webrtc/peerconnection -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../netwerk/srtp/src/include -I/Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/../../../netwerk/srtp/src/crypto/include -fPIC -Qunused-arguments -Qunused-arguments -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wno-c++0x-extensions -Wno-extended-offsetof -Wno-unknown-warning-option -Wno-return-type-c-linkage -Wno-mismatched-tags -fcolor-diagnostics -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -std=gnu++0x -pthread -DNO_X11 -pipe -DDEBUG -D_DEBUG -DTRACING -g -fno-omit-frame-pointer -I/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/include/nspr -I/Users/ehsanakhgari/moz/mozilla-central/obj-ff-dbg/dist/include/nss -Qunused-arguments -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -MD -MP -MF .deps/VcmSIPCCBinding.o.pp /Users/ehsanakhgari/moz/mozilla-central/media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
 5:00.39 make[7]: *** [src/media/VcmSIPCCBinding.o] Error 1
 5:00.39 make[7]: *** Waiting for unfinished jobs....

Comment 4

[Eric Rescorla (:ekr)]

Ehsan,

If I am reading this correctly, the problem is that we are relying on implicit conversion from
nsCOMPtr<nsIEventTarget> to nsIEventTarget *.


What I suggest is that we add an overload for RUN_ON_THREAD() that is:

static inline result RUN_ON_THREAD(const nsCOMPtr<nsIEventTarget>& thread, ...)

That should fix the compile problem.

The other choice, of course, is to add a .get() at the end here, but that seems like a less-general solution.

Comment 5

[(no longer active)]

Either sounds good to me, with a slight preference towards the first solution.

Comment 6

[Trevor Saunders (:tbsaunde)]

> If I am reading this correctly, the problem is that we are relying on
> implicit conversion from
> nsCOMPtr<nsIEventTarget> to nsIEventTarget *.

yes, but other than  "to be slow" why  do you have a function returning a refptr in the first place?

Comment 7

[Eric Rescorla (:ekr)]

Because unboxing smart pointers is bad.

Comment 8

[[e-mail]]

(In reply to Eric Rescorla from comment #7)
> Because unboxing smart pointers is bad.

Surely that's a bit of an oversimplification. With C++ standard smart pointers you only need to use them when you want to allow or force the transfer of ownership (you may actually want to prevent that for various reasons) while for objects that maintain their own lifetime, as is the case here, you can safely pass a raw pointer if you know the object will outlive your call, yet the recipient of the pointer can still acquire a share of ownership in the object.