1120312 - Fix -Wunused-variable warning in toolkit/xre and mark directory as FAIL_ON_WARNINGS

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: xre-warnings.patch

Variable `rv` in function XRE_ProcLoaderPreload() is reported as unused in release builds. XRE_ProcLoaderPreload returns void, so there is no way to return early if NS_NewNativeLocalFile() or omnijarFile->AppendNative() were to return an error.

This patch uses MOZ_RELEASE_ASSERT to crash the process if either of these functions fail. Or do you prefer that I change XRE_ProcLoaderPreload() to return bool or nsresult so the caller, LoadStaticData() in B2GLoader.cpp, can return?

This -Wunused-variable warning was the last warning in the toolkit/xre directory, so this patch also marks toolkit/xre and toolkit/xre/test/win/moz.build as FAIL_ON_WARNINGS. :)

Comment 1

[Chris Peterson [:cpeterson]]

Green Try build with FAIL_ON_WARNINGS:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=41bf04f8b09d

Comment 2

[Thinker Li [:sinker]]

Comment on attachment 8547394 [details] [diff] [review]
xre-warnings.patch

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

Hi Chris,  how do you think to use DebugOnly instead of replacing MOZ_ASSERT() with MOZ_RELEASE_ASSERT()?

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +875,4 @@
>  {
>      void PreloadXPT(nsIFile *);
>  
>      nsresult rv;

DebugOnly<nsresult> rv;

@@ +879,5 @@
>      nsCOMPtr<nsIFile> omnijarFile;
>      rv = NS_NewNativeLocalFile(nsCString(aProgramDir),
>  			       true,
>  			       getter_AddRefs(omnijarFile));
> +    MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));

keep this using MOZ_ASSERT()

@@ +885,2 @@
>      rv = omnijarFile->AppendNative(NS_LITERAL_CSTRING(NS_STRINGIFY(OMNIJAR_NAME)));
> +    MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));

And this.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Thinker Li [:sinker] from comment #2)
> Hi Chris,  how do you think to use DebugOnly instead of replacing
> MOZ_ASSERT() with MOZ_RELEASE_ASSERT()?

I considered using DebugOnly<nsresult> but if XRE_ProcLoaderPreload() can't successfully NS_NewNativeLocalFile() or omnijarFile->AppendNative(), that seems like a critical "can't happen" failure. If we used only a debug MOZ_ASSERT, then a hypothetical XRE_ProcLoaderPreload() failure would be harder to debug.

Comment 4

[Thinker Li [:sinker]]

Comment on attachment 8547394 [details] [diff] [review]
xre-warnings.patch

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

Ok! Since this is not a performance critical path, I don't insist my previous opinion.  Even I think debug build is good enough to detect these errors.

Comment 5

[Chris Peterson [:cpeterson]]

Thanks, Thinker!

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

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ccbb7771093d