821066 - Remove dead remotedesc/localdesc messages and events from sipcc

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Adam Roach [:abr]]

The infrastructure for asynchronous fetching of localDescription and remoteDescription is dead code. It should be removed. See Bug 797534, comment 1 for background.

In media/webrtc/signaling/src/sipcc/core/includes/ccapi.h, the messages CC_MSG_REMOTEDESC and CC_MSG_LOCALDESC need to be removed. In media/webrtc/signaling/sipcc/core/gsm/fsmdef.c, the corresponding functions fsmdef_ev_remotedesc and fsmdef_ev_localdesc need to be removed.

Note that the addition and removal of events from the SIPCC stack is not a trivial task. Performing the cleanup described above will effectively require reversal of the steps described at https://wiki.mozilla.org/Media/WebRTC/SIPCCMessaging

Comment 1

[Alex Chronopoulos [:achronop]]

Hi can I work on this? Thanks

Comment 2

[Maire Reavy [:mreavy]]

(In reply to Alexandros Chronopoulos [:achronop] from comment #1)
> Hi can I work on this? Thanks

Just let us know that you'd like to fix this bug and start coding up a fix.

Then, if you have questions or code that you'd like someone to look at, you can post your questions/requests here, and we'll help you out.

Thanks for wanting to help out!

Comment 3

[Adam Roach [:abr]]

Alexandros: If you have any questions about any part of this task, feel free to find me (abr) in #media on irc.mozilla.org. Both ehugg and ekr know their way around this part of the code pretty well also, in case I'm not around.

Thanks!

Comment 4

[Alex Chronopoulos [:achronop]]

I will start working it and I'll let you know my progress/problems ... Thanks both for the support! Can you please assign it to me.

Comment 5

[Alex Chronopoulos [:achronop]]

Attachment: Fix for bug 821066

This is an initial patch for this bug. I have removed the MSGs and FEATUREs for REMOTEDESC and LOCALDESC including the relevant code. Please provide an initial review and let me know if I am missing anything.

Can you please advice how this can be tested properly. I try to execute the signalling unit test but I get an error about the the libnss3.so (outdated). I decided to ask first before try anything else because I am not sure if this is the correct testing.

Another thing I would like to ask is if I can update the wiki page provided in the description. I went though it thoroughly and I saw some differences with the current status of the code. If yes should I open a bug for it?

Comment 6

[Adam Roach [:abr]]

(In reply to Alexandros Chronopoulos [:achronop] from comment #5)
> This is an initial patch for this bug. I have removed the MSGs and FEATUREs
> for REMOTEDESC and LOCALDESC including the relevant code. Please provide an
> initial review and let me know if I am missing anything.

Excellent. It will probably be tomorrow before I can look at this.

> Can you please advice how this can be tested properly. I try to execute the
> signalling unit test but I get an error about the the libnss3.so (outdated).
> I decided to ask first before try anything else because I am not sure if
> this is the correct testing.

The signaling unit tests are what I would use for a first test, followed by the mochi tests. You might need to update your LD_LIBRARY_PATH to get the test to run. See the instructions here for running the tests:

https://wiki.mozilla.org/Webrtc/checkin_policy

> Another thing I would like to ask is if I can update the wiki page provided
> in the description. I went though it thoroughly and I saw some differences
> with the current status of the code. If yes should I open a bug for it?

Feel free to update the wiki to match the code. You don't need to open a bug to update the wiki.

Comment 7

[Adam Roach [:abr]]

Comment on attachment 699396 [details] [diff] [review]
Fix for bug 821066

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

Looks good to me. Apparently, the implementation of these messages didn't get plumbed as far up the stack as I'd expected. Thanks!

BTW, I've verified these changes with signaling_unittests and the media mochi tests (I was mostly wanting to make sure we caught everywhere the change in the FSM array definition length changed). Both run clean.

Comment 8

[Alex Chronopoulos [:achronop]]

Can you please help me with try server and check in? I do not have the required permission. Thanks

Comment 9

[Adam Roach [:abr]]

I pushed this to try off Alder for mochi tests and crashtests: https://tbpl.mozilla.org/?tree=Try&rev=d4e24e90410d

Comment 10

[Alex Chronopoulos [:achronop]]

I see some build errors on mobile platforms. Can you please advice how to approach them. Thanks

Comment 11

[Adam Roach [:abr]]

(In reply to Alexandros Chronopoulos [:achronop] from comment #10)
> I see some build errors on mobile platforms. Can you please advice how to
> approach them. Thanks

Those don't look like they're related to this patch. It's far more likely that there was some problem with the build system when that try went through. I've retriggered the failed builds just to be on the safe side.

Comment 12

[Adam Roach [:abr]]

Alexandros: The rebuild attempt worked. There are a small handful of mochi test failures, but these stem from a known problem with the mochi tests that has since been fixed. In my opinion, you're clear to ask for check in. I would suggest asking :jesup or :ehugg.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3399392cc837

Thanks for the patch, Alexandros! In the future, please make sure your patches follow the guidelines below so that all the necessary commit information is present in them. It makes life easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3399392cc837