970271 - [Network] Fix some GCC warnings in Necko

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

GCC prints lots of warnings when bulding Necko code for Gonk.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 970271: Fix whitespace errors

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 970271: Fix GCC warnings about uninitialized variables

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 970271: Fix GCC warnings about ignored return value

Comment 5

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [05] Bug 970271: Fix casting pointer to integer of different size

Comment 6

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [06] Bug 970271: Fix GCC warnings about unused variables

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [07] Bug 970271: Fix inaccessible base class nsIStreamListener

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8373285 [details] [diff] [review]
[01] Bug 970271: Fix whitespace errors

Going to pass these off to someone actively involved in necko...

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 8373285 [details] [diff] [review]
> [01] Bug 970271: Fix whitespace errors
> 
> Going to pass these off to someone actively involved in necko...

Oops, sorry. You are listed as a module peer for Necko.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Oops, sorry. You are listed as a module peer for Necko.

Yeah, that's sort of a relic of times past.  I actually asked to be removed recently, but it hadn't happened yet...  Fixed now.

Comment 11

[Jason Duell]

Comment on attachment 8373285 [details] [diff] [review]
[01] Bug 970271: Fix whitespace errors

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

Wow--epic amount of whitespace fixing! :)

So the general policy is this:  we're happy to take whitespace fixes that don't clutter the hg blame output.  So all the fixes here that fix space on otherwise empty lines (or lines that only have '}', etc) are fine.  

Lines that have source code we should just let sit, so if someone wants to use blame to track down why the code is so, they don't have to hop around version history to find it.   So strip those out of the patch, please.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8373285 [details] [diff] [review]
[01] Bug 970271: Fix whitespace errors

Let's make this obsolete. These changes were done automatically by my text editor. I don't feel like going through all the files and sort out the empty-line fixes by hand. :D

Comment 13

[Patrick McManus [:mcmanus]]

I don't actually mind formatting cleanups that address more than empty lines. I'd rather wait for the big clang formatting patch being discussed on dev-mumble though

a] so this only happens once
b] so there is a tool that can enforce the style as part of checkin

also, and this is more important imo, rtsp and sctp changes should be looked at by :sworkman and :jesup.. they may involve things that are really just imported from upstream and need to be managed differently.

Comment 14

[Jason Duell]

Comment on attachment 8373289 [details] [diff] [review]
[04] Bug 970271: Fix GCC warnings about ignored return value

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

Just a minor nit.

(Patrick: the Rtsp Parent/Child code is all Mozilla and not upstream.  I'll make sure to request additional review for rtsp/sctp stuff that might be upstream.)

::: netwerk/protocol/rtsp/controller/RtspControllerParent.cpp
@@ +22,5 @@
>  #define SEND_DISCONNECT_IF_ERROR(rv)                         \
>    if (NS_FAILED(rv) && mIPCOpen && mTotalTracks > 0ul) {     \
>      for (uint32_t i = 0; i < mTotalTracks; i++) {            \
> +      bool success = SendOnDisconnected(i, rv);              \
> +      NS_WARN_IF_FALSE(success, "SendOnDisconnected failed");\

Do it this way, just for consistency with the rest of our e10s code:

  #include "mozilla/unused.h"

  unused << SendOnDisconnected(i, rv);

see for instance:

   http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelParent.cpp#608

Comment 15

[Jason Duell]

Comment on attachment 8373290 [details] [diff] [review]
[05] Bug 970271: Fix casting pointer to integer of different size

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

Looks fine but checking with Jesup to see if there's any upstream issue here.

Comment 16

[Jason Duell]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

ditto

Comment 17

[Jason Duell]

Comment on attachment 8373292 [details] [diff] [review]
[07] Bug 970271: Fix inaccessible base class nsIStreamListener

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

I don't understand this patch.  Can you clarify on what line of code GCC issues the error?

Does this code link once you change nsBaseChannel to inherit from nsIStreamListener?  You haven't implemented the nsIStreamListener methods.  And you don't want to :)

If there's any way we can fix this in RtspChannel that would be better. (Why can't GCC access the base nsIStreamListener class?  We're inheriting publicly from it).

Comment 18

[Jason Duell]

Comment on attachment 8373287 [details] [diff] [review]
[03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

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

+r on the RtspControllerChild/Parent parts.   Forwarding the sctp bit to jesup

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8373287 [details] [diff] [review]
[03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

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

r+, but note that the sctp source files are all from upstream, so you need to request a change in the SCTP upstream from Michael Tuexen (if they're not already fixed)

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Michael, can you check if these are fixed upstream?  (I'll be importing an update soon.)  Thanks

Comment 21

[Michael Tüxen]

(In reply to Randell Jesup [:jesup] from comment #20)
> Michael, can you check if these are fixed upstream?  (I'll be importing an
> update soon.)  Thanks

Will do tomorrow and let you know.

Best regards
Michael

Comment 22

[Michael Tüxen]

Randell,

most of them were already addressed, mostly in a different way. The ones which
were not addressed have been addressed in
https://code.google.com/p/sctp-refimpl/source/detail?r=8809
https://code.google.com/p/sctp-refimpl/source/detail?r=8810

So all issues should be resolved by updating the usrsctp code.

Best regards
Michael

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8373290 [details] [diff] [review]
[05] Bug 970271: Fix casting pointer to integer of different size

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

Ok, since these are resolved upstream, I'm going to cancel review of them for the SCTP code and it will be resolved when I import a new SCTP library shortly.

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8373287 [details] [diff] [review]
[03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values

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

please don't land the sctp parts per above

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

SCTP update is bug 916427

Comment 26

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 970271: Fix GCC warnings about uninitialized variables (v2)

Changes to v1:

  - removed SCTP fixes

Comment 27

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values (v2)

Changes to v1:

  - removed SCTP fixes

Comment 28

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 970271: Fix GCC warnings about ignored return value (v2)

Changes to v1:

  - shift by return value, as shown in comment 14

Comment 29

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8373290 [details] [diff] [review]
[05] Bug 970271: Fix casting pointer to integer of different size

Obsoleting [05] as it only contains SCTP fixes.

Comment 30

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Jason Duell (:jduell) from comment #17)
> Comment on attachment 8373292 [details] [diff] [review]
> [07] Bug 970271: Fix inaccessible base class nsIStreamListener
> 
> Review of attachment 8373292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand this patch.  Can you clarify on what line of code GCC
> issues the error?

I see this on the command line:

> In file included from /home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspChannel.cpp:8:
/home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspChannel.h:26: warning: direct base 'nsIStreamListener' inaccessible in 'mozilla::net::RtspChannel' due to ambiguity
> In file included from /home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspHandler.cpp:8:
/home/mozilla/Projects/mozilla/src/mozilla-central/netwerk/protocol/rtsp/RtspChannel.h:26: warning: direct base 'nsIStreamListener' inaccessible in 'mozilla::net::RtspChannel' due to ambiguity

> Does this code link once you change nsBaseChannel to inherit from
> nsIStreamListener?  You haven't implemented the nsIStreamListener methods. 
> And you don't want to :)

Yes it links fine.

RtspChannel inherits from nsBaseChannel. The ambiguity comes from RtspChannel and nsBaseChannel both inheriting from nsIStreamListener. I removed the inheritance in RtspChannel, and made the inheritance of nsBaseChannel protected, so that RtspChannel can access the implementation.

> If there's any way we can fix this in RtspChannel that would be better. (Why
> can't GCC access the base nsIStreamListener class?  We're inheriting
> publicly from it)

Comment 31

[Jason Duell]

Comment on attachment 8373292 [details] [diff] [review]
[07] Bug 970271: Fix inaccessible base class nsIStreamListener

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

Ah, now I understand.  Nice fix--thanks!

Comment 32

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Ping for review of [06].

Comment 33

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

I'm not involved in RTSP.....  Check the logs for those files.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

jduell and sworkman reviewed previous checkins to these files

Comment 35

[Jason Duell]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

Benjamin, can you review these RTSP bits? (you're listed in hg log as the person who added the files).  The main issue is whether the code comes from upstream (and if so whether we should submit the fixes upstream instead of in our tree).

Comment 36

[Benjamin Chen [:bechen]]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

Redirect to :vchang

Comment 37

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

Thanks for your help on this.

Comment 38

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 970271: Fix GCC warnings about comparison of signed and unsigned values (v3)

Changes to v2:

  - updated for removal of whitespace fix [01]

Comment 39

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks :)

https://hg.mozilla.org/integration/b2g-inbound/rev/32ebe4dcd0ba
https://hg.mozilla.org/integration/b2g-inbound/rev/26d23e6ded3a
https://hg.mozilla.org/integration/b2g-inbound/rev/f936af2c52ad
https://hg.mozilla.org/integration/b2g-inbound/rev/b5d404be0b1e
https://hg.mozilla.org/integration/b2g-inbound/rev/c95e7ab2b48b

https://tbpl.mozilla.org/?tree=B2g-Inbound&rev=c95e7ab2b48b

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/32ebe4dcd0ba
https://hg.mozilla.org/mozilla-central/rev/26d23e6ded3a
https://hg.mozilla.org/mozilla-central/rev/f936af2c52ad
https://hg.mozilla.org/mozilla-central/rev/b5d404be0b1e
https://hg.mozilla.org/mozilla-central/rev/c95e7ab2b48b

Comment 41

[Ethan Tseng [:ethan]]

Comment on attachment 8373291 [details] [diff] [review]
[06] Bug 970271: Fix GCC warnings about unused variables

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

::: netwerk/protocol/rtsp/rtsp/AMPEG4AudioAssembler.cpp
@@ +106,5 @@
>  }
>  
>  static status_t parseGASpecificConfig(
>          ABitReader *bits,
>          unsigned audioObjectType, unsigned channelConfiguration) {

Hi Thomas,
Thank you for cleaning those annoying GCC warnings. Appreciated. :)

Removal of this line caused a regression in bug 979761.
ABitReader::getBits() actually shifts its internal members recording the bit position parsed.
I think we should follow the style in other places in this file, that is:
/*unsigned frameLengthFlag = */bits->getBits(1);

This is fixed in the patch in bug 979761.

Comment 42

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Oh, sorry about this and thanks a lot for fixing the problem. I'll be more careful in the future.

Comment 43

[Ethan Tseng [:ethan]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #42)
> Oh, sorry about this and thanks a lot for fixing the problem. I'll be more
> careful in the future.
Don't be sorry. I am so happy to have these fixes to eliminate warnings.
It just reminds me that we should hurry up to work out a test framework of the RTSP feature, so that we can prevent such regression bug.