Enable FAIL_ON_WARNINGS in more of netwerk/

RESOLVED FIXED in mozilla18

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Ms2ger, Assigned: valentin)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 614877 [details] [diff] [review]
Patch v1

There you go
Attachment #614877 - Flags: review?(jduell.mcbugs)
(Reporter)

Updated

6 years ago
Blocks: 187528
Comment on attachment 614877 [details] [diff] [review]
Patch v1

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

A couple nits.

Have you run through try?

::: netwerk/base/src/nsFileStreams.h
@@ +182,5 @@
>  {
>  public:
> +    using nsFileInputStream::Init;
> +    using nsFileInputStream::Read;
> +

You needed to add these to get rid of a warning?

::: netwerk/socket/nsSOCKSIOLayer.cpp
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

Bonus points if you add a vim modeline here too.
Comment on attachment 614877 [details] [diff] [review]
Patch v1

+r with nits addressed.
Attachment #614877 - Flags: review?(jduell.mcbugs) → review+
(Reporter)

Comment 3

6 years ago
(In reply to Jason Duell (:jduell) from comment #1)
> Comment on attachment 614877 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 614877 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A couple nits.
> 
> Have you run through try?
> 
> ::: netwerk/base/src/nsFileStreams.h
> @@ +182,5 @@
> >  {
> >  public:
> > +    using nsFileInputStream::Init;
> > +    using nsFileInputStream::Read;
> > +
> 
> You needed to add these to get rid of a warning?

Yeah... C++ hiding stuff
Created attachment 619770 [details] [diff] [review]
v1, part 2

Together with part 1 and my patch for 750445, this gets rid of all necko warnings for me, at least on my Linux box. Delegating review to ms2ger since he's been working on this stuff.

Pushed to try:

   https://tbpl.mozilla.org/?tree=Try&rev=be0f811a6c4d
Attachment #619770 - Flags: review?(Ms2ger)
Created attachment 619803 [details] [diff] [review]
v1, part 3

I should have said, "gets rid of all necko warnings for me in directories with FAIL_ON_WARNINGS".  This gets rid of one of the last remaining warnings in the other directories, and turns on FAIL_ON_WARNINGS for all directories that contain C++ code except wifi/ and tests/

re: ParseFTPList.  C programmers do silly things sometimes.  Like use a pointer to a function as a boolean "isInitialized" flag when they could just use a contructor.

The last remaining necko warning (on linux at least) is in wifi/: it's harder to fix because we're using dlsym() to get a function pointer, but dlsym() itself returns void *, so there's no easy way to get rid of the warning about converting void * to a function ptr AFACIT (bug 703121 comment 8 suggests we may be able to do it at some point with a #pragma).  I'm not going to worry about it for now. 

This also fixes the OSX-only gcc warning about empty while statements that failed on my last try attempt.  Pushing again:

   https://tbpl.mozilla.org/?tree=Try&rev=f54d408a48d2
Assignee: Ms2ger → jduell.mcbugs
Attachment #619803 - Flags: review?
Attachment #619803 - Flags: review? → review?(Ms2ger)
Duplicate of this bug: 666617
Duplicate of this bug: 491190
filed bug 750616 to deal with android-only warning.
No longer blocks: 750616
Depends on: 750616
Created attachment 619846 [details] [diff] [review]
part3, v2

updated to fix some OSX-only warnings.
Attachment #619803 - Attachment is obsolete: true
Attachment #619803 - Flags: review?(Ms2ger)
Attachment #619846 - Flags: review?(Ms2ger)
Created attachment 619847 [details] [diff] [review]
part 4: avoid pragma warning on OSX

Honza:  This patch just disables your diagnostic check from bug 621446 on OSX.  Since we're only seeing the error on Linux, I assume that's OK.

With the four patches in this bug, plus those for bug 750445 and bug 750616, I'm now seeing all-green on try with FAIL_ON_WARNINGS turned all for all of necko except system/mac (I filed bug 750584 for that) and the dlsym issue in wifi/

   https://tbpl.mozilla.org/?tree=Try&rev=9d59ea146fb7
Attachment #619847 - Flags: review?(honzab.moz)
(Reporter)

Comment 11

6 years ago
Comment on attachment 619770 [details] [diff] [review]
v1, part 2

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

::: netwerk/base/src/nsURLParsers.cpp
@@ +503,5 @@
>  
>      // search backwards for @
>      const char *p = auth + authLen - 1;
> +    for (; (*p != '@') && (p > auth); --p) {
> +      ;

Nit: I prefer 'continue;'

::: netwerk/cache/nsDiskCacheBinding.cpp
@@ +339,3 @@
>  
>      hashEntry = (HashTableEntry*) PL_DHashTableOperate(&table,
> +                                                       (void*)(PRUptrdiff) key,

key already is a void*, so just remove the casts here.
Attachment #619770 - Flags: review?(Ms2ger) → review+
Comment on attachment 619847 [details] [diff] [review]
part 4: avoid pragma warning on OSX

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

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +379,5 @@
>    return true;
>  }
>  
>  // Bug 621446 investigation
> +#if !defined(XP_MACOSX)

#ifdef ANDROID ?
> #ifdef ANDROID ?

I don't understand--you've added these pragmas for e10s, and Android isn't running e10s.
(In reply to Jason Duell (:jduell) from comment #13)
> > #ifdef ANDROID ?
> 
> I don't understand--you've added these pragmas for e10s, and Android isn't
> running e10s.

??? The crash is reported exceptionally on Fennec.  There is ANDROID defined, isn't it?

Was ever e10s used on Android?  Has that been changed recently?

If that is so, then I'm totally confused now..
The old XUL version of Fennec used e10s. The newer native version does not.
Josh, so this is INVALID on the native build?
I don't know what "this" is, unfortunately.
this == this bug
Oh.. not this bug, but bug 621446.
B2G is actively working on moving to e10s.
Josh, problem is that I now can't get any crash-debug data.  So the investigation patch is actually useless if we want bug 621446 get fixed before publish of b2g.

(In reply to Jason Duell (:jduell) from comment #13)
> > #ifdef ANDROID ?
> 
> I don't understand--you've added these pragmas for e10s, and Android isn't
> running e10s.

I want to be just symmetric.  We change the prefs when ANDROID is defined, so we should reset them again only when ANDROID is defined.

According the native fennec situation I think I'll back the investigation patch out and fix the crash by added null check+assertion+log.
(Reporter)

Comment 22

6 years ago
Comment on attachment 619846 [details] [diff] [review]
part3, v2

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

LGTM

::: netwerk/cookie/nsCookieService.cpp
@@ +2852,5 @@
>    // remove trailing <LWS>; first check we're not at the beginning
>    lastSpace = aIter;
>    if (lastSpace != start) {
> +    while (--lastSpace != start && iswhitespace(*lastSpace))
> +      ;

continue here too
Attachment #619846 - Flags: review?(Ms2ger) → review+
Comment on attachment 619847 [details] [diff] [review]
part 4: avoid pragma warning on OSX

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

I'm completely removing this code in bug 621446.
Attachment #619847 - Flags: review?(honzab.moz)
Attachment #619847 - Attachment is obsolete: true
Assignee: jduell.mcbugs → valentin.gosu
Valentin:  I think this should be able to land, once you compile again to check that we haven't added any new warnings since the patches were posted.

IIRC these patches cover the rest of necko--check to make sure I didn't miss any.

It'll be nice to have this!
(Assignee)

Updated

6 years ago
Duplicate of this bug: 750584
(Assignee)

Comment 26

6 years ago
Created attachment 654554 [details] [diff] [review]
Final fix

This patch fixes a remaining warning an enables FAIL_ON_WARNINGS = 1 for most of the subfolders. Skipped netwerk/locales and netwerk/protocol/app because they don't contain any C++ files.
Also enabled in netwerk/test, although I don't know if that's necessary.

Fixes warning:
In file included from /Users/vgosu/ff_trunk_debug/netwerk/base/src/nsFileStreams.cpp:21:
/Users/vgosu/ff_trunk_debug/netwerk/base/src/nsFileStreams.h:135:9: error: field 'mIOFlags' will be initialized after field 'mLineBuffer' [-Werror,-Wreorder]
      : mIOFlags(0), mLineBuffer(nullptr), mPerm(0), mCachedPosition(0)
        ^
1 error generated.
Attachment #654554 - Flags: review?(jduell.mcbugs)
Created attachment 655121 [details] [diff] [review]
rolled all patches into one and rebased: still broken

Valentin:   the patches here duplicated a bunch of stuff, and some makefile infrastructure (and PR types changes) bitrotted them.  I fixed all that, but it still doesn't compile w/o warnings.   Grab this patch and try to compile, and fix the remaining warnings when you have time--thanks!
Attachment #614877 - Attachment is obsolete: true
Attachment #619770 - Attachment is obsolete: true
Attachment #619846 - Attachment is obsolete: true
Attachment #654554 - Attachment is obsolete: true
Attachment #654554 - Flags: review?(jduell.mcbugs)
Attachment #655121 - Flags: review-
(Reporter)

Comment 28

6 years ago
Created attachment 655336 [details] [diff] [review]
Patch

Passes try: https://tbpl.mozilla.org/?tree=Try&rev=7246d906a5e2

This patch adds FAIL_ON_WARNINGS to all folders that contain C++, except for netwerk/wifi and netwerk/streamconv/src on Mac, for the following failures:

https://tbpl.mozilla.org/php/getParsedLog.php?id=14697410&tree=Try
https://tbpl.mozilla.org/php/getParsedLog.php?id=14696913&tree=Try
Attachment #655121 - Attachment is obsolete: true
Attachment #655336 - Flags: review?(jduell.mcbugs)
Attachment #655336 - Flags: feedback?(valentin.gosu)
(Assignee)

Comment 29

6 years ago
Comment on attachment 655336 [details] [diff] [review]
Patch

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

Looks pretty good.

::: netwerk/Makefile.in
@@ +6,5 @@
>  DEPTH		= @DEPTH@
>  topsrcdir	= @top_srcdir@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
> +FAIL_ON_WARNINGS := 1

This line actually does nothing
Attachment #655336 - Flags: feedback+
Comment on attachment 655336 [details] [diff] [review]
Patch

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

\o/  No more unnoticed warnings...
 
  https://hg.mozilla.org/integration/mozilla-inbound/rev/a3c81ae4cabb
  
Landed w/o FAIL_ON_WARNINGS for /netwerk/cache due to new warning that landed there: filed bug 786154 for it.

::: netwerk/Makefile.in
@@ +6,5 @@
>  DEPTH		= @DEPTH@
>  topsrcdir	= @top_srcdir@
>  srcdir		= @srcdir@
>  VPATH		= @srcdir@
> +FAIL_ON_WARNINGS := 1

True.  I removed it.

::: netwerk/mime/nsMIMEHeaderParamImpl.cpp
@@ -795,5 @@
>        delimiters++;
> -    } else if (tc >= 128) {
> -      // fail early, not ASCII
> -      NS_WARNING("non-US-ASCII character in RFC5987-encoded param");
> -      return NS_ERROR_INVALID_ARG;

We want to keep the check for not-ASCII here. C++ allows compilers to choose whether 'char' is unsigned or not by default), so I changed the 'if' test to (((unsigned char)tc) >= 128).
Attachment #655336 - Flags: review?(jduell.mcbugs)
Attachment #655336 - Flags: review+
Attachment #655336 - Flags: feedback?(valentin.gosu)
https://hg.mozilla.org/mozilla-central/rev/a3c81ae4cabb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.