Closed Bug 864109 Opened 11 years ago Closed 11 years ago

Can't create an offer if there are no mediastreams attached and createDataChannel() hasn't been called (fails on 0 m-lines)

Categories

(Core :: WebRTC: Networking, defect)

Product:
Core
Component:
WebRTC: Networking
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24

People

(Reporter: jsmith, Assigned: ctangirala)

Details

(Whiteboard: [webrtc][blocking-webrtc-][spec-issue?][qa-])

Attachments

(2 files, 3 obsolete files)

Test Case
2.12 KB, text/html
Details
Fixed indentation issue with lines inside if block.
2.39 KB, patch
ehugg
: review+
Details | Diff | Splinter Review
Attached file Test Case — 
STR:

1. Load the attached test case
2. Analyze the web console

Expected:

The handshake between two peers should be successful.

Actual:

createOffer fails with an INTERNAL_ERROR:

"local pc createOffer failed with: Could not create local SDP for offer; cause = ERR"
Whiteboard: [WebRTC][blocking-webrtc-][turn]
Why is this marked as TURN? Does it succeed without TURN?
(In reply to Eric Rescorla (:ekr) from comment #1)
> Why is this marked as TURN? Does it succeed without TURN?

Reproduces without TURN.
Summary: Trying to create an offer using a TURN server with a media constraint provided fails with INTERNAL_ERROR → Trying to create an offer with a media constraint provided fails with INTERNAL_ERROR
Whiteboard: [WebRTC][blocking-webrtc-][turn] → [WebRTC][blocking-webrtc?]
The testcase doesn't add any streams; CreateOffer() with no streams fails to create SDP (in theory it should, as it's valid SDP per RFC 4566, but that would also mean no media active, which means no DTLS sessions and no identity verification (though it's unclear if that matters if there's no media).  The spec isn't entirely clear on this yet.

This was discussed (perhaps in IRC, perhaps email) when we landed the datachannel SDP changes and stopped creating an m=application line on all offer SDP's, and it broke several tests that assumed you didn't need at least the offer to receive a stream.  I can't find a bug on it, so repurposing this one.
Summary: Trying to create an offer with a media constraint provided fails with INTERNAL_ERROR → Can't create an offer if there are no mediastreams attached and createDataChannel() hasn't been called (fails on 0 m-lines)
Whiteboard: [WebRTC][blocking-webrtc?] → [webrtc][blocking-webrtc-][spec-issue?]
Oh wait - I see the mistake I made. I didn't include mandatory with the media constraint, so it failed to go through. So we just need to fix the error code here then.
I see at least two potential action items here:

1) Have createOffer recognize this no-op case and throw nice error. Something like:
   "Error: createOffer has nothing to do. Add a stream, data channel or constraint."

2) Emit a "Warning: Malformed constraint?" if neither "mandatory:" or "optional:"
   are found AND other keys are found at the top level.
May want to hold off until bug 823512 lands, for practical reasons.
Assignee: nobody → ctangira
Discussed the first action item with jib and I plan to trap the no-op case in the Javascript layer and throw an exception with a relevant error message.
Why an exception and not an error message?
If the problem is that first-encounter users of the API balk at the rather low-level error message they get for the naive mistake they made of calling createOffer before addStream or createDataChannel, then adding an exception for this logic error seems reasonable, points them at the correct line in their source, and is simple to implement at the API layer, without worrying about whether addStream or createDataChannel succeeds (those calls produce errors independently I hope).
Given that you are required to provide error callbacks, I don't really see the important value here.
To help narrow down where work should be done: fsmdef_ev_createoffer (in fsmdef.c) needs to check whether any media has been added, and send back an evCreateOfferError with a sensible error message if it has not. Something like:

>        ui_create_offer(evCreateOfferError, fcb->state, line, call_id,
>            dcb->caller_id.call_instance_id, strlib_empty(),
>            PC_INVALID_STATE, "Cannot create SDP without any streams");

At this level (for right now), you check whether any streams are present by traversing the dcb_p->media_cap_tbl for CC_AUDIO_1, CC_VIDEO_1, and CC_DATACHANNEL_1; if ->enabled is false for all three of them, then return this error.
Attachment #756000 - Flags: review?(adam)
Comment on attachment 756000 [details] [diff] [review]
Check media_cap_tbl and see if any streams are added. If none are true, then send an error to the UI.

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

Thanks for the patch! I hope that this review doesn't come off as overly critical. We have a number of coding conventions for C/C++ code that we're trying to maintain. For legacy, imported code like this, the general rule of thumb is that any new or changed code should conform to the Mozilla coding standards (which I cite in a few places below).

I'm marking this as r- for now, mostly for the risk of null dereferences.

General comment: use spaces rather than tabs for indenting (see https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Whitespace). You may want to set up your editor to assist you with not adding tabs to files. If you use vim, you can add "set expandtab" to your ~/.vimrc file. For emacs, you can add "(setq-default indent-tabs-mode nil)" to your ~/.emacs file. Other editors should have similar settings.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3130,4 @@
>  
>      FSM_DEBUG_SM(DEB_F_PREFIX"Entered.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
>  
> +    if(dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled == TRUE ||

You need to check whether dcb->media_cap_tbl is non-null before attempting to dereference it -- I think there might be code paths that can get us here before the media_cap_tbl has been initialized.

There should be a space after "if".

@@ +3131,5 @@
>      FSM_DEBUG_SM(DEB_F_PREFIX"Entered.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
>  
> +    if(dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled == TRUE ||
> +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){

Remove "== TRUE" from all three comparisons, please. You may want to take a quick glance at https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.2B.2B_practices

There should be a space before the "{" character.

@@ +3134,5 @@
> +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> +    	has_stream = TRUE;
> +    }
> +    if(!has_stream){

Space after "if", and another one before "{"

@@ +3135,5 @@
> +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> +    	has_stream = TRUE;
> +    }
> +    if(!has_stream){
> +    	FSM_DEBUG_SM(DEB_F_PREFIX"Cannot create SDP without any streams.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));

Wrap this line to fit within 80 columns (see https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Line_Length)

@@ +3138,5 @@
> +    if(!has_stream){
> +    	FSM_DEBUG_SM(DEB_F_PREFIX"Cannot create SDP without any streams.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +    	ui_create_offer(evCreateOfferError, line, call_id,
> +			dcb->caller_id.call_instance_id, strlib_empty(),
> +			PC_INVALID_STATE, "Cannot create SDP without any streams.");

It's hard to tell with the tabs in here, but I think this line also requires wrapping to fit in 80 columns.

@@ +3139,5 @@
> +    	FSM_DEBUG_SM(DEB_F_PREFIX"Cannot create SDP without any streams.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> +    	ui_create_offer(evCreateOfferError, line, call_id,
> +			dcb->caller_id.call_instance_id, strlib_empty(),
> +			PC_INVALID_STATE, "Cannot create SDP without any streams.");
> +    	return SM_RC_CLEANUP;

This kind of failure shouldn't be fatal to the fcb (e.g., if the app adds streams and tries again, it should work) -- use SM_RC_END instead.

@@ +3140,5 @@
> +    	ui_create_offer(evCreateOfferError, line, call_id,
> +			dcb->caller_id.call_instance_id, strlib_empty(),
> +			PC_INVALID_STATE, "Cannot create SDP without any streams.");
> +    	return SM_RC_CLEANUP;
> +    }

Please move all of the new code down below the "if (dcb == NULL)" check, below -- we can't dereference dcb until we know it's valid.
Attachment #756000 - Flags: review?(adam) → review-
Thanks Adam. I will make the suggested changes and upload another patch.

(In reply to Adam Roach [:abr] from comment #13)
> Comment on attachment 756000 [details] [diff] [review]
> Check media_cap_tbl and see if any streams are added. If none are true, then
> send an error to the UI.
> 
> Review of attachment 756000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! I hope that this review doesn't come off as overly
> critical. We have a number of coding conventions for C/C++ code that we're
> trying to maintain. For legacy, imported code like this, the general rule of
> thumb is that any new or changed code should conform to the Mozilla coding
> standards (which I cite in a few places below).
> 
> I'm marking this as r- for now, mostly for the risk of null dereferences.
> 
> General comment: use spaces rather than tabs for indenting (see
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Whitespace). You may want to set up your editor to assist you
> with not adding tabs to files. If you use vim, you can add "set expandtab"
> to your ~/.vimrc file. For emacs, you can add "(setq-default
> indent-tabs-mode nil)" to your ~/.emacs file. Other editors should have
> similar settings.
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +3130,4 @@
> >  
> >      FSM_DEBUG_SM(DEB_F_PREFIX"Entered.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> >  
> > +    if(dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled == TRUE ||
> 
> You need to check whether dcb->media_cap_tbl is non-null before attempting
> to dereference it -- I think there might be code paths that can get us here
> before the media_cap_tbl has been initialized.
> 
> There should be a space after "if".
> 
> @@ +3131,5 @@
> >      FSM_DEBUG_SM(DEB_F_PREFIX"Entered.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> >  
> > +    if(dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled == TRUE ||
> > +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> > +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> 
> Remove "== TRUE" from all three comparisons, please. You may want to take a
> quick glance at
> https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#C.2FC.
> 2B.2B_practices
> 
> There should be a space before the "{" character.
> 
> @@ +3134,5 @@
> > +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> > +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> > +    	has_stream = TRUE;
> > +    }
> > +    if(!has_stream){
> 
> Space after "if", and another one before "{"
> 
> @@ +3135,5 @@
> > +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> > +    	has_stream = TRUE;
> > +    }
> > +    if(!has_stream){
> > +    	FSM_DEBUG_SM(DEB_F_PREFIX"Cannot create SDP without any streams.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> 
> Wrap this line to fit within 80 columns (see
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Line_Length)
> 
> @@ +3138,5 @@
> > +    if(!has_stream){
> > +    	FSM_DEBUG_SM(DEB_F_PREFIX"Cannot create SDP without any streams.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> > +    	ui_create_offer(evCreateOfferError, line, call_id,
> > +			dcb->caller_id.call_instance_id, strlib_empty(),
> > +			PC_INVALID_STATE, "Cannot create SDP without any streams.");
> 
> It's hard to tell with the tabs in here, but I think this line also requires
> wrapping to fit in 80 columns.
> 
> @@ +3139,5 @@
> > +    	FSM_DEBUG_SM(DEB_F_PREFIX"Cannot create SDP without any streams.", DEB_F_PREFIX_ARGS(FSM, __FUNCTION__));
> > +    	ui_create_offer(evCreateOfferError, line, call_id,
> > +			dcb->caller_id.call_instance_id, strlib_empty(),
> > +			PC_INVALID_STATE, "Cannot create SDP without any streams.");
> > +    	return SM_RC_CLEANUP;
> 
> This kind of failure shouldn't be fatal to the fcb (e.g., if the app adds
> streams and tries again, it should work) -- use SM_RC_END instead.
> 
> @@ +3140,5 @@
> > +    	ui_create_offer(evCreateOfferError, line, call_id,
> > +			dcb->caller_id.call_instance_id, strlib_empty(),
> > +			PC_INVALID_STATE, "Cannot create SDP without any streams.");
> > +    	return SM_RC_CLEANUP;
> > +    }
> 
> Please move all of the new code down below the "if (dcb == NULL)" check,
> below -- we can't dereference dcb until we know it's valid.
Comment on attachment 756000 [details] [diff] [review]
Check media_cap_tbl and see if any streams are added. If none are true, then send an error to the UI.

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

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3134,5 @@
> +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> +    	has_stream = TRUE;
> +    }
> +    if(!has_stream){

It seems to me that you could remove the has_stream variable and have this if be:
if (!dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled && 
!dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled && 
!dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled) {

unless I missed some other usage here of has_stream
(In reply to Ethan Hugg [:ehugg] from comment #15)
> Comment on attachment 756000 [details] [diff] [review]
> Check media_cap_tbl and see if any streams are added. If none are true, then
> send an error to the UI.
> 
> Review of attachment 756000 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> @@ +3134,5 @@
> > +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> > +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> > +    	has_stream = TRUE;
> > +    }
> > +    if(!has_stream){
> 
> It seems to me that you could remove the has_stream variable and have this
> if be:
> if (!dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled && 
> !dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled && 
> !dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled) {
> 
> unless I missed some other usage here of has_stream

That occurred to me as well, but I actually like this structure a little better. The reason is that, in the future, we're going to have a very different kind of structure for keeping track of how *many* streams of each kind we have, and depending on how that looks, we might have to do a literal iteration over an undetermined number of entries. In that kind of scenario, having a flag that starts as false and gets updated when a stream is found seems to be preferable.

But I really don't feel strongly one way or another about it.
(In reply to Adam Roach [:abr] from comment #13)

> Thanks for the patch! I hope that this review doesn't come off as overly
> critical. We have a number of coding conventions for C/C++ code that we're
> trying to maintain. For legacy, imported code like this, the general rule of
> thumb is that any new or changed code should conform to the Mozilla coding
> standards (which I cite in a few places below).

I'd phrase it slightly differently: when coding, you should generally follow the style of the file, especially for imported code, doubly-so for code with an active upstream.

That said, there is a mozilla coding style (see abr's reference).  New files should follow that.  Major new code/functions in existing files should follow that (this is fuzzy).  I would not apply it to within-function changes at odds with the style in the function, doubly-so for imported code.  In some cases restyling a file is ok, but generally frowned on due to the disruption of version-control 'blame'.

An example of this style issue also occurs in media/mtransport, which generally follows google style, and media/webrtc/signaling/src/mediapipeline and media-conduit.  They're an exception to the "new code" issue due to convenience and that ekr was more used to that style.  As module owner, I have discretion here.

> General comment: use spaces rather than tabs for indenting (see
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Whitespace). You may want to set up your editor to assist you
> with not adding tabs to files. If you use vim, you can add "set expandtab"
> to your ~/.vimrc file. For emacs, you can add "(setq-default
> indent-tabs-mode nil)" to your ~/.emacs file. Other editors should have
> similar settings.

yes, please!  Also do not introduce trailing spaces, and it's ok to do things like "delete-trailing-whitespace" on files in emacs.


I do like the 'has_stream' approach as we know we'll need to support multiple streams - but it's not a big thing at all.
>yes, please!  Also do not introduce trailing spaces, and it's ok to do things like >"delete-trailing-whitespace" on files in emacs.

Bug 800611 was done earlier just so you can turn on delete-trailing-whitespace in your editor and not worry about making extra whitespace changes.
My reasoning was similar for having a hcs_stream variable. For future need if there is ever a need to quickly check and see if any stream was ever added. I don't have any preference either. Let me know which way to go.

(In reply to Adam Roach [:abr] from comment #16)
> (In reply to Ethan Hugg [:ehugg] from comment #15)
> > Comment on attachment 756000 [details] [diff] [review]
> > Check media_cap_tbl and see if any streams are added. If none are true, then
> > send an error to the UI.
> > 
> > Review of attachment 756000 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
> > @@ +3134,5 @@
> > > +    		dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled == TRUE ||
> > > +    		dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled == TRUE){
> > > +    	has_stream = TRUE;
> > > +    }
> > > +    if(!has_stream){
> > 
> > It seems to me that you could remove the has_stream variable and have this
> > if be:
> > if (!dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled && 
> > !dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled && 
> > !dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled) {
> > 
> > unless I missed some other usage here of has_stream
> 
> That occurred to me as well, but I actually like this structure a little
> better. The reason is that, in the future, we're going to have a very
> different kind of structure for keeping track of how *many* streams of each
> kind we have, and depending on how that looks, we might have to do a literal
> iteration over an undetermined number of entries. In that kind of scenario,
> having a flag that starts as false and gets updated when a stream is found
> seems to be preferable.
> 
> But I really don't feel strongly one way or another about it.
>My reasoning was similar for having a hcs_stream variable. For future need if there >is ever a need to quickly check and see if any stream was ever added. I don't have >any preference either. Let me know which way to go.

You should leave in has_stream.  ABR is your reviewer - I'm just throwing rocks.
Your patch breaks mochitests: https://tbpl.mozilla.org/?tree=Try&rev=7ccf1d0eb355

Run ./mach mochitest-plain dom/media/tests/mochitest
Hmm, not sure why tinderbox fails to load that link. Worked earlier. Here's a log from that try from a window I had open: https://tbpl.mozilla.org/php/getParsedLog.php?id=23604973&tree=Try#error0
(In reply to Jan-Ivar Bruaroey [:jib] from comment #21)
> Your patch breaks mochitests

Good catch. The cases that fail all share the property that they're not adding any streams, but *are* calling createOffer with constraints that turn streams on for receiving audio and/or video. The problem is that the new code this patch adds attempts to check for active streams before the constraint processing has taken place, so it doesn't take the constraints into consideration.

This points to a need to put the new code down even further in the function, *after* gsmsdp_process_cap_constraints() has a chance to run.
Attachment #756000 - Attachment is obsolete: true
Attachment #756780 - Attachment is obsolete: true
Comment on attachment 756799 [details] [diff] [review]
Check media_cap_tbl and see if any streams are added. If none are true, then send an error to the UI.


Assuming this should go back to abr for review.
Attachment #756799 - Flags: review?(adam)
jib also kicked off a try last week. Following is the link:

https://tbpl.mozilla.org/?tree=Try&rev=0db335af29e2
Comment on attachment 756799 [details] [diff] [review]
Check media_cap_tbl and see if any streams are added. If none are true, then send an error to the UI.

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

Try runs look clean. r+, assuming you fix the one formatting nit.

::: media/webrtc/signaling/src/sipcc/core/gsm/fsmdef.c
@@ +3173,5 @@
>      }
>  
> +    if (dcb->media_cap_tbl->cap[CC_VIDEO_1].enabled ||
> +      dcb->media_cap_tbl->cap[CC_AUDIO_1].enabled ||
> +      dcb->media_cap_tbl->cap[CC_DATACHANNEL_1].enabled) {

Indent these two lines to match the opening paren ("dcb" should line up on all three lines).
Attachment #756799 - Flags: review?(adam) → review+
Attachment #756799 - Attachment is obsolete: true
Attachment #757657 - Flags: review?(adam)
Comment on attachment 757657 [details] [diff] [review]
Fixed indentation issue with lines inside if block.


Bringing forward r+ from abr.

Charith, you already got your r+ from abr.  I will check the whitespace and check it in for you.
Attachment #757657 - Flags: review?(adam) → review+
(In reply to Ethan Hugg [:ehugg] from comment #31)
> I will check the whitespace and check it in for you.

FWIW, it's still not quite in line with the style of the rest of the file. The two lines in question still need one more space each.
Oh, and "has_stream = TRUE;" needs to move back to where it was. We're just changing the clause of the "if ()", not its body.
I got those nits.  One more thing for next time.  The patch should have a message that describes what the whole patch does and not just the latest mods.  I changed the patch message to

Return proper error message if createOffer called with no streams r=abr

I will push to M-I this evening.
Thanks for the clarification, I see what I missed. Jib directed me towards Eclipse CDT for Firefox page (https://developer.mozilla.org/en-US/docs/Eclipse_CDT). It has a section that covers coding styles. I will go through the the process and hopefully that will take care of coding style issues for me :)

(In reply to Ethan Hugg [:ehugg] from comment #31)
> Comment on attachment 757657 [details] [diff] [review]
> Fixed indentation issue with lines inside if block.
> 
> 
> Bringing forward r+ from abr.
> 
> Charith, you already got your r+ from abr.  I will check the whitespace and
> check it in for you.
https://hg.mozilla.org/mozilla-central/rev/c588029a06fa
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [webrtc][blocking-webrtc-][spec-issue?] → [webrtc][blocking-webrtc-][spec-issue?][qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: