1026336 - Fix warnings in content/media/webrtc and mark FAIL_ON_WARNINGS

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: fix-webrtc-warnings.patch

* Fixed clang and gcc warnings:

> content/media/webrtc/LoadManager.cpp:44:5 [-Wreorder] field 'mLoadSumMeasurements' will be initialized after field 'mOveruseActive'
> content/media/webrtc/LoadManager.cpp:49:5 [-Wreorder] field 'mLowLoadThreshold' will be initialized after field 'mCurrentState'
> content/media/webrtc/LoadManager.cpp:50:5 [-Wreorder] field 'mCurrentState' will be initialized after field 'mLock'
> content/media/webrtc/MediaEngineWebRTC.h:347:7 [-Wunused-private-field] private field 'mSamples' is not used

The warning about MediaEngineWebRTC's mSamples member variable being unused is only true for non-PR_LOGGING builds. However, since PR_LOGGING is a per-.cpp file #define, I could not add #ifdef PR_LOGGING to the mSamples member variable's declaration in MediaEngineWebRTC.h. Instead, I changed its visibility from private to protected. This is a hack, but it suppresses the warning because the clang compiler doesn't know if any derived classes might use mSamples. :)

> content/media/webrtc/PeerIdentity.cpp:81:14: error: ignoring return value of function declared with const attribute [-Werror,-Wunused-value]

NS_WARN_IF returns a bool value, which PeerIdentity::GetNormalizedHost() does not check. As a workaround, replace NS_WARN_IF with NS_WARN_IF_FALSE, which doesn't return a value.

> xpcom/base/nsAutoPtr.h:73:5 [-Wdelete-incomplete] deleting pointer to incomplete type 'mozilla::LoadManager' may cause undefined behavior
> 
> Warning: -Wdelete-incomplete in xpcom/base/nsAutoPtr.h: deleting pointer to incomplete type 'mozilla::LoadManager' may cause undefined behavior
> media/webrtc/signaling/../../../xpcom/base/nsAutoPtr.h:73:5: warning: deleting pointer to incomplete type 'mozilla::LoadManager' may cause undefined beavhior
>     delete mRawPtr;
>     ^      ~~~~~~~
> media/webrtc/signaling/./src/media-conduit/VideoConduit.h:223:3: note: in instantiation of member function 'nsAutoPtr<mozilla::LoadManager>::~n
>   WebrtcVideoConduit():
>   ^
> ../../../../dist/include/LoadManagerFactory.h:11:7: note: forward declaration of 'mozilla::LoadManager'
> class LoadManager;

Here, clang is warning that WebrtcVideoConduit's ctor's initialization of the nsAutoPtr<LoadManager> mLoadManager (somehow) could call delete on a LoadManager pointer, which is only forward declared in VideoConduit.h. Moving WebrtcVideoConduit's ctor out of line to VideoConduit.cpp fixes the warning because the VideoConduit.cpp #includes LoadManager's class definition (through LoadManagerFactory.h).

* Fixed MSVC warnings:
> content\media\webrtc\LoadManagerFactory.cpp(32) : warning C4305: 'argument' : truncation from 'double' to 'float'
> content\media\webrtc\LoadManagerFactory.cpp(34) : warning C4305: 'argument' : truncation from 'double' to 'float'

* Suppressed MSVC warnings:
> c:\PROGRA~2\MICROS~2.0\vc\include\typeinfo(157) : warning C4275: non dll-interface class 'stdext::exception' used as base for dll-interface class 'std::bad_cast'
>         c:\PROGRA~2\MICROS~2.0\vc\include\exception(218) : see declaration of 'stdext::exception'
        > c:\PROGRA~2\MICROS~2.0\vc\include\typeinfo(156) : see declaration of 'std::bad_cast'


Green Try build:
https://tbpl.mozilla.org/?tree=Try&rev=72870633532d

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8441117 [details] [diff] [review]
fix-webrtc-warnings.patch

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

::: content/media/webrtc/MediaEngineWebRTC.h
@@ +319,5 @@
>  
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
> +protected:
> +  int mSamples; // int to avoid conversions when comparing/etc to samplingFreq & length

Add a comment about why it's not private

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8441117 [details] [diff] [review]
fix-webrtc-warnings.patch

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

forgot to mark r+ when I reviewed

Comment 3

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/855ac2eeb5fc

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/855ac2eeb5fc