745296 - Enable FAIL_ON_WARNINGS in more of netwerk/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

There you go

Comment 1

[Jason Duell]

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 2

[Jason Duell]

Comment on attachment 614877 [details] [diff] [review]
Patch v1

+r with nits addressed.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(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

Comment 4

[Jason Duell]

Attachment: 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

Comment 5

[Jason Duell]

Attachment: 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

Comment 8

[Jason Duell]

filed bug 750616 to deal with android-only warning.

Comment 9

[Jason Duell]

Attachment: part3, v2

updated to fix some OSX-only warnings.

Comment 10

[Jason Duell]

Attachment: 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

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 12

[Honza Bambas (:mayhemer)]

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 ?

Comment 13

[Jason Duell]

> #ifdef ANDROID ?

I don't understand--you've added these pragmas for e10s, and Android isn't running e10s.

Comment 14

[Honza Bambas (:mayhemer)]

(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..

Comment 15

[Josh Matthews [:jdm]]

The old XUL version of Fennec used e10s. The newer native version does not.

Comment 16

[Honza Bambas (:mayhemer)]

Josh, so this is INVALID on the native build?

Comment 17

[Josh Matthews [:jdm]]

I don't know what "this" is, unfortunately.

Comment 18

[Honza Bambas (:mayhemer)]

this == this bug

Comment 19

[Honza Bambas (:mayhemer)]

Oh.. not this bug, but bug 621446.

Comment 20

[Josh Matthews [:jdm]]

B2G is actively working on moving to e10s.

Comment 21

[Honza Bambas (:mayhemer)]

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.

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 23

[Honza Bambas (:mayhemer)]

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.

Comment 24

[Jason Duell]

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!

Comment 26

[Valentin Gosu [:valentin] (he/him)]

Attachment: 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.

Comment 27

[Jason Duell]

Attachment: 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!

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: 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

Comment 29

[Valentin Gosu [:valentin] (he/him)]

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

Comment 30

[Jason Duell]

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).

Comment 31

[Ed Morley [:emorley]]

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