825570 - Calling setLocalDescription on a PeerConnection object with a valid SDP - localDescription does not change

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P3)

Description

[Jason Smith [:jsmith]]

Attachment: Test Case

Steps:

1. Load the attached test case
2. Create a peer connection object
3. Create an offer
4. Set the local description with the SDP from the offer just made
5. Check the localDescription attribute on the peer connection

Expected:

The localDescription attribute should be the SDP given in step #4.

Actual:

The localDescription ends up being null. Something tells me we've forgot to set the local description attribute that's publicly exposed.

Comment 1

[Jason Smith [:jsmith]]

I'm going to guess right up front this one is definitely a blocker.

Comment 2

[Jason Smith [:jsmith]]

Hmm...I'm wondering now if I'm understanding the spec correctly here. So after I call setRemoteDescription, then I call setLocalDescription, the localDescription object is set. If I just call setRemoteDescription, the remoteDescription does get set. But calling setLocalDescription without setRemoteDescription does not change the localDescription attribute.

Comment 3

[Adam Roach [:abr]]

(In reply to Jason Smith [:jsmith] from comment #2)
> Hmm...I'm wondering now if I'm understanding the spec correctly here. So
> after I call setRemoteDescription, then I call setLocalDescription, the
> localDescription object is set. If I just call setRemoteDescription, the
> remoteDescription does get set. But calling setLocalDescription without
> setRemoteDescription does not change the localDescription attribute.

IIRC, the spec doesn't spell out what happens if you do a setLocalDescription before first doing a createOffer or createAnswer. I can't quite determine, from your description, whether you're doing either of these actions first.

Also, I'm having a hard time following the reproduction steps. I'm interpreting step 2 as "Push the button marked 'Create Peer Connection 1'" and step 3 as "Push the button marked 'PC1 Offer'".

I have no clue what action step 4 is supposed to evoke.

I presume step 5 is to mean something like "Press the button marked 'Get PC 1 Attributes' and examine the 'localDescription' field."

Reading through the javascript in the attached testcase, I don't see any attempts to set the local description. Perhaps you inadvertently attached an early, unfinished version of the test case rather than the one you're actually using?

I'm asking for feedback on two fronts: (1) please be a bit more explicit about which buttons are to be pressed in the reproduction steps, using the actual text rendered in the browser, and (2) please double-check that the attached test case is the one you intended to attach.

Comment 4

[Jason Smith [:jsmith]]

Comment on attachment 696689 [details]
Test Case

Opps...an attached old copy. Let me fix that then...

Comment 5

[Jason Smith [:jsmith]]

Attachment: Test Case

Here's the updated copy

Comment 6

[Jason Smith [:jsmith]]

Steps:

1. Select Create Peer Connection 1
2. Select PC 1 Offer
3. Select PC 1 Set Local Description
4. Select Get PC 1 Attributes

Expected - localDescription should be set to the sdp set in step #3

Actual - localDescription is null

Comment 7

[Adam Roach [:abr]]

Attachment: Finish plumbing attribute getters for localDescription and remoteDescription

Comment 8

[Adam Roach [:abr]]

Comment on attachment 697478 [details] [diff] [review]
Finish plumbing attribute getters for localDescription and remoteDescription

This patch is a fix for the earlier plumbing attempt, which confused the pre-existing wallpaper with the wall. We've changed localDescription and remoteDescription to use JavaScript getters which call into the PeerConnectionImpl. This allows the dynamically updated session description (i.e., updated via ice/trickle ice) to be correctly exposed to the application.

For the type (offer/answer/etc), we keep a local queue of that tracks the setLocalDescription and setRemoteDescription operations in the queue of pending operations, and assign them to _localType and _remoteType (as appropriate) upon success (or discard them on failure). While we could plumb these all the way down to the SIPCC layer, nothing would be gained by doing so (and the rather labyrinthine gauntlet required to get information up and down the stack makes such plumbing quite time consuming).

Comment 9

[Adam Roach [:abr]]

After a discussion with jesup, I'm going to revise this patch so that it uses a temporary scalar instead of a temporary queue. This will be accomplished by piggybacking the types on the actions queue.

Comment 10

[Adam Roach [:abr]]

Attachment: Finish plumbing attribute getters for localDescription and remoteDescription

Comment 11

[Adam Roach [:abr]]

Comment on attachment 701981 [details] [diff] [review]
Finish plumbing attribute getters for localDescription and remoteDescription

This patch makes the changes we discussed earlier -- using a scalar instead of a shadow queue.

Comment 12

[Adam Roach [:abr]]

Hold off the review on this for a while -- I think I turned up a bug in the new patch.

Comment 13

[Adam Roach [:abr]]

Attachment: Finish plumbing attribute getters for localDescription and remoteDescription

Comment 14

[Adam Roach [:abr]]

Comment on attachment 702866 [details] [diff] [review]
Finish plumbing attribute getters for localDescription and remoteDescription

Fixing an issue with the patch in which the description types didn't get updated for queued operations.

Comment 15

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

Comment on attachment 702866 [details] [diff] [review]
Finish plumbing attribute getters for localDescription and remoteDescription

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

Much better, thanks

smaug: Is this something I should be bothering you/DOM-team for additional review on?  I'm tending towards "ask for review" as a default for stuff in this area even if the impact seems contained, but don't want to bug you if it's not needed.  Thanks

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 702866 [details] [diff] [review]
Finish plumbing attribute getters for localDescription and remoteDescription

Could you add comments why you explicitly check for undefined.

(I think it is good to have 2 pairs of eyes to check this code, so asking
someone from DOM team to look at this too is ok.
As you know, personally I'd feel a lot more comfortable if the code was in C++. )

Comment 17

[Adam Roach [:abr]]

Olli -- like you, I don't have an abundance of familiarity with JavaScript. The reason I'm doing explicit comparisons with undefined here is because that's the way the rest of the code in this file is written. When I showed up, there were already eight explicit comparisons against undefined.

The decision to compare explicitly against undefined, then, is something of a "when in Rome" convention: people who probably know more than I do established a pattern of doing so, and I don't have enough experience with the language to second-guess them.

That's not the sort of thing that I'd put in a code comment, though. Do we need to track down a JavaScript style expert to explain which (if any) of the many explicit comparisons against undefined in this file are okay, and which should just use the variable as a boolean value?

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Adam Roach [:abr] from comment #17)
> Olli -- like you, I don't have an abundance of familiarity with JavaScript.
> The reason I'm doing explicit comparisons with undefined here is because
> that's the way the rest of the code in this file is written. When I showed
> up, there were already eight explicit comparisons against undefined.
Ok, thanks. This is good enough explanation.
No need to add anything to the code.

Comment 19

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/edcfa1ea5dda

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Jason Smith [:jsmith]]

Verified on today's build. Adam added a test in a separate patch that actually tests for this, so marking in-testsuite+.