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

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

(Blocks 1 bug)

unspecified
mozilla38
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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. :)
Attachment #8547394 - Flags: review?(tlee)
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.
(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 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.
Attachment #8547394 - Flags: review?(tlee) → review+
https://hg.mozilla.org/mozilla-central/rev/ccbb7771093d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1126376
Depends on: 1138295
You need to log in before you can comment on or make changes to this bug.