1098134 - Fix or suppress warnings in gmp-api, gmp-clearkey, and gmp-plugin and mark as FAIL_ON_WARNINGS

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: fix-gmp-warnings.patch

Fix -Wdelete-non-virtual-dtor warnings by adding virtual destructors to gmp-api abstract base classes:
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:47:3 [-Wdelete-non-virtual-dtor] delete called on 'FakeDecryptor' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:95:5 [-Wdelete-non-virtual-dtor] delete called on 'TestEmptyContinuation' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:107:5 [-Wdelete-non-virtual-dtor] delete called on 'TruncateContinuation' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:122:5 [-Wdelete-non-virtual-dtor] delete called on 'VerifyAndFinishContinuation' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:139:5 [-Wdelete-non-virtual-dtor] delete called on 'VerifyAndOverwriteContinuation' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:166:5 [-Wdelete-non-virtual-dtor] delete called on 'OpenedSecondTimeContinuation' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:183:5 [-Wdelete-non-virtual-dtor] delete called on 'OpenedFirstTimeContinuation' that has virtual functions but non-virtual destructor
> dom/media/gmp-plugin/gmp-test-decryptor.cpp:266:5 [-Wdelete-non-virtual-dtor] delete called on 'ReportReadStatusContinuation' that has virtual functions but non-virtual destructor

Suppress warnings in third-party openaes library:
> media/gmp-clearkey/0.1/openaes/oaes_lib.c:100:8 [-Wmissing-braces] suggest braces around initialization of subobject
> media/gmp-clearkey/0.1/openaes/oaes_lib.c:783:15 [-Wsign-compare] comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int'

Instead of suppressing these -Wimplicit-function-declaration warnings in openaes, we can force the preprocesser to include appropriate header files to get the function prototypes:
> media/gmp-clearkey/0.1/openaes/oaes_lib.c:474:3 [-Wimplicit-function-declaration] implicit declaration of function 'sprintf'
> media/gmp-clearkey/0.1/openaes/oaes_lib.c:514:4 [-Wimplicit-function-declaration] implicit declaration of function 'getpid'

Here is a green Try build with FAIL_ON_WARNINGS:

https://tbpl.mozilla.org/?tree=Try&rev=951ff732a95a

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'm getting build errors after a clobber when building locally:

 0:20.85 Unified_c_gmp-clearkey_0.10.obj
 0:20.86 module.res
 0:20.87 Creating Resource file: module.res
 0:20.90 Microsoft (R) Windows (R) Resource Compiler Version 6.3.9600.16384
 0:20.90
 0:20.90 Copyright (C) Microsoft Corporation.  All rights reserved.
 0:20.90
 0:20.90
 0:20.90 Unified_cpp_unit_tlsserver_lib0.obj
 0:20.91 http_upload.obj
 0:20.92 guid_string.obj
 0:20.94 string_utils.obj
 0:20.95 nsRDFResource.obj
 0:20.95 TestWebappRT.obj
 0:20.95 TestCodeGenBinding.obj
 0:20.95 TestDictionaryBinding.obj
 0:21.07 FI
 0:21.07 c1 : fatal error C1083: Cannot open source file: 'C:/mozilla-build/msys/FI': No such file or directory
 0:21.07 stdio.h
 0:21.07 c1 : fatal error C1083: Cannot open source file: 'stdio.h': No such file or directory
 0:21.07 Unified_c_gmp-clearkey_0.10.c
 0:21.07 Warning: C4013 in c:\Users\cpearce\src\mozilla\inbound\media\gmp-clearkey\0.1\openaes\oaes_lib.c: 'sprintf' undefined; assuming extern returning int
 0:21.07 c:\Users\cpearce\src\mozilla\inbound\media\gmp-clearkey\0.1\openaes/oaes_lib.c(474) : warning C4013: 'sprintf' undefined; assuming extern returning int
 0:21.07 Generating Code...
 0:21.08
 0:21.09 In the directory  /c/Users/cpearce/src/mozilla/inbound/objdir/media/gmp-clearkey/0.1
 0:21.09 The following command failed to execute properly:
 0:21.09 c:/Users/cpearce/src/mozilla/inbound/objdir/_virtualenv/Scripts/python.exe -m mozbuild.action.cl cl -FoUnified_c_gmp-clearkey_0.10.obj -c -DMOZ_NO_MOZALLOC -DAB_CD=en-US -DNO_NSPR_10_SUPPORT
-Ic:/Users/cpearce/src/mozilla/inbound/media/gmp-clearkey/0.1 -I. -Ic:/Users/cpearce/src/mozilla/inbound/dom/media/gmp -I../../../dist/include -Ic:/Users/cpearce/src/mozilla/inbound/objdir/dist/includ
e/nspr -Ic:/Users/cpearce/src/mozilla/inbound/objdir/dist/include/nss -MT -FI ../../../dist/include/mozilla-config.h -DMOZILLA_CLIENT -TC -nologo -D_HAS_EXCEPTIONS=0 -W3 -Gy -arch:IA32 -FS -wd4244 -wd
4267 -wd4819 -we4553 -DDEBUG -DTRACING -Z7 -Oy- /FI stdio.h -wd4090 c:/Users/cpearce/src/mozilla/inbound/objdir/media/gmp-clearkey/0.1/Unified_c_gmp-clearkey_0.10.c
 0:21.11 c:/Users/cpearce/src/mozilla/inbound/config/rules.mk:902: recipe for target 'Unified_c_gmp-clearkey_0.10.obj' failed
 0:21.11 mozmake.EXE[5]: *** [Unified_c_gmp-clearkey_0.10.obj] Error 1
 0:21.11 mozmake.EXE[5]: *** Deleting file 'Unified_c_gmp-clearkey_0.10.obj'
 0:21.12 c:/Users/cpearce/src/mozilla/inbound/config/recurse.mk:74: recipe for target 'media/gmp-clearkey/0.1/target' failed
 0:21.12 mozmake.EXE[4]: *** [media/gmp-clearkey/0.1/target] Error 2
 0:21.12 mozmake.EXE[4]: *** Waiting for unfinished jobs....
 0:21.12 client_info.obj
 0:21.17 TestWebappRT.cpp
 0:21.18 Note: rebuild with "c:/mozilla-build/mozmake/mozmake.EXE VERBOSE=1 all-local" to show all compiler parameters.
 0:21.19 TestWebappRT.exe
 0:21.42 guid_string.cc

I'm building on Visual Studio 2013, Win8.1.

Comment 2

[Chris Peterson [:cpeterson]]

I just backed this out. It was green on Try, but I think Try only performs incremental builds.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ea818fdbd81c

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Looks like this actually built on Try, tomcat said it was the push before that was bad?

Sorry for the false alarm.

Comment 4

[Chris Peterson [:cpeterson]]

I suspect the build problem was that I used the command line syntax "/FI stdio.h" instead of "-FI stdio.h", but Try's incremental build didn't catch this because I made no .cpp changes and just changing moz.build didn't trigger a rebuild.

I'm testing a new patch with "-FI stdio.h" here:

https://tbpl.mozilla.org/?tree=Try&rev=f613142083ff

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8521993 [details] [diff] [review]
fix-gmp-warnings.patch

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

::: dom/media/gmp/gmp-api/gmp-decryption.h
@@ +20,5 @@
>  #include "gmp-platform.h"
>  
>  class GMPEncryptedBufferMetadata {
>  public:
> +  virtual ~GMPEncryptedBufferMetadata() {}

Also before you land again, please put all of these destructors at the *end* of the class definition, so that you don't change the vtable layout and break existing GMPs that are compiled with an older version of these headers.

Comment 6

[Chris Peterson [:cpeterson]]

Take 2:
* Fixed -FI flag. I confirmed this works now with a clobbered Try build.
* Moved new virtual destructors to the end of the class definition.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f85d8942e3e

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3f85d8942e3e