bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Remove dead remotedesc/localdesc messages and events from sipcc

RESOLVED FIXED in mozilla21

Status

()

Core
WebRTC: Signaling
--
trivial
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: abr, Assigned: achronop)

Tracking

unspecified
mozilla21
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Whiteboard: [WebRTC] [blocking-webrtc-]
(Assignee)

Comment 1

6 years ago
Hi can I work on this? Thanks
(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!
(Reporter)

Comment 3

6 years ago
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!
(Assignee)

Comment 4

6 years ago
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.
(Reporter)

Updated

6 years ago
Assignee: nobody → achronop
(Assignee)

Comment 5

6 years ago
Created attachment 699396 [details] [diff] [review]
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?
Attachment #699396 - Flags: review?(adam)
(Reporter)

Comment 6

6 years ago
(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.
(Reporter)

Comment 7

6 years ago
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.
Attachment #699396 - Flags: review?(adam) → review+
(Assignee)

Comment 8

6 years ago
Can you please help me with try server and check in? I do not have the required permission. Thanks
(Reporter)

Comment 9

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

Comment 10

6 years ago
I see some build errors on mobile platforms. Can you please advice how to approach them. Thanks
(Reporter)

Comment 11

6 years ago
(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.
(Reporter)

Comment 12

6 years ago
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.
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3399392cc837
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21

Updated

6 years ago
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
You need to log in before you can comment on or make changes to this bug.