Closed
Bug 825570
Opened 12 years ago
Closed 12 years ago
Calling setLocalDescription on a PeerConnection object with a valid SDP - localDescription does not change
Categories
(Core :: WebRTC, defect, P3)
Core
WebRTC
Tracking
()
VERIFIED
FIXED
mozilla21
People
(Reporter: jsmith, Assigned: abr)
Details
(Whiteboard: [WebRTC][blocking-webrtc+])
Attachments
(2 files, 3 obsolete files)
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.
Reporter | ||
Comment 1•12 years ago
|
||
I'm going to guess right up front this one is definitely a blocker.
Whiteboard: [WebRTC][blocking-webrtc+]
Reporter | ||
Updated•12 years ago
|
Flags: in-testsuite?
Updated•12 years ago
|
Assignee: nobody → adam
Priority: -- → P3
Reporter | ||
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
(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.
Flags: needinfo?(jsmith)
Reporter | ||
Comment 4•12 years ago
|
||
Comment on attachment 696689 [details]
Test Case
Opps...an attached old copy. Let me fix that then...
Attachment #696689 -
Attachment is obsolete: true
Flags: needinfo?(jsmith)
Reporter | ||
Comment 6•12 years ago
|
||
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
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Comment 8•12 years ago
|
||
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).
Attachment #697478 -
Flags: review?(rjesup)
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 9•12 years ago
|
||
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.
Assignee | ||
Comment 10•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #697478 -
Attachment is obsolete: true
Attachment #697478 -
Flags: review?(rjesup)
Assignee | ||
Comment 11•12 years ago
|
||
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.
Attachment #701981 -
Flags: review?(rjesup)
Assignee | ||
Comment 12•12 years ago
|
||
Hold off the review on this for a while -- I think I turned up a bug in the new patch.
Assignee | ||
Comment 13•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #701981 -
Attachment is obsolete: true
Attachment #701981 -
Flags: review?(rjesup)
Assignee | ||
Comment 14•12 years ago
|
||
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.
Attachment #702866 -
Flags: review?(rjesup)
Comment 15•12 years ago
|
||
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
Attachment #702866 -
Flags: review?(rjesup)
Attachment #702866 -
Flags: review?(bugs)
Attachment #702866 -
Flags: review+
Comment 16•12 years ago
|
||
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++. )
Attachment #702866 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 17•12 years ago
|
||
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•12 years ago
|
||
(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.
Assignee | ||
Updated•12 years ago
|
Attachment #702866 -
Flags: checkin?(rjesup)
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/edcfa1ea5dda
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Attachment #702866 -
Flags: checkin?(rjesup) → checkin+
Comment 20•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/edcfa1ea5dda
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 21•12 years ago
|
||
Verified on today's build. Adam added a test in a separate patch that actually tests for this, so marking in-testsuite+.
Status: RESOLVED → VERIFIED
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•