Open
Bug 896648
Opened 11 years ago
Updated 2 years ago
Make conversion operator of ref pointers safer
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
NEW
People
(Reporter: jcranmer, Unassigned)
References
Details
Attachments
(1 file)
2.34 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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•11 years ago
|
||
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Either sounds good to me, with a slight preference towards the first solution.
Comment 6•11 years ago
|
||
> 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•11 years ago
|
||
Because unboxing smart pointers is bad.
Comment 8•11 years ago
|
||
(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.
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•