834153 - Queue CreateAnswer in PC.js

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

Currently, PeerConnection.js examines remoteDescription which transitively
calls PCImpl::remotedescription

  createAnswer: function(onSuccess, onError, constraints, provisional) {
    if (!this.remoteDescription) {
      throw new Error("setRemoteDescription not called");
    }

    if (this.remoteDescription.type != "offer") {
      throw new Error("No outstanding offer");
    }

    if (!constraints) {
      constraints = {};
    }

  ...

    // TODO: Implement provisional answer.
    this._queueOrRun({
      func: this._pc.createAnswer,
      args: [constraints],
      wait: true
    });


There is a race condition here because SetRemoteDescription is queued
pending SIPCC and ICE readiness but the check of this.remoteDescription
is not queued. If you lose the aces, then either:

(a) the ICE state check in GetRemoteDescription fails
(b) An empty string is returned at which point this exception is
thrown.

Either one causes problems.

Comment 1

[Adam Roach [:abr]]

Attachment: Queue CreateAnswer State Checks

Comment 2

[Adam Roach [:abr]]

Attachment: Queue CreateAnswer State Checks

Comment 3

[Eric Rescorla (:ekr)]

Comment on attachment 705894 [details] [diff] [review]
Queue CreateAnswer State Checks

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

lgtm but hold off on committing till I hjave tested it in the case that fails.

Comment 4

[Adam Roach [:abr]]

Attachment: Queue CreateAnswer State Checks

Comment 5

[Adam Roach [:abr]]

Comment on attachment 705919 [details] [diff] [review]
Queue CreateAnswer State Checks

Rebasing on m-i tip. Carrying forward r+ from ekr

Comment 6

[Adam Roach [:abr]]

Attachment: Mochi test

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 705950 [details] [diff] [review]
Mochi test

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

lgtm but with proposed nits.

::: dom/media/tests/mochitest/test_peerConnection_bug834153.html
@@ +16,5 @@
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=834153">Queue CreateAnswer in PeerConnection.js</a>
> +<p id="display"></p>
> +<pre id="test">
> +<script class="testbody" type="application/javascript">
> +function croak(msg) {

Could this be moved into pc.js or some other common code.

@@ +29,5 @@
> +  ok(pc1, "pc1 is non-null");
> +
> +  pc1.createOffer(
> +    function(d) {
> +      pc2 = new mozRTCPeerConnection();

Shouldn't there be a var somewhere? this is being hoisted into global scope.

Comment 8

[Eric Rescorla (:ekr)]

Attachment: Queue CreateAnswer State Checks

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 706172 [details] [diff] [review]
Queue CreateAnswer State Checks

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

The problem with Adam's initial patch was that we were cranking executeNext out of non-callback events (e.g.. pc.onaddstream).

This means that if you, for instance, call setRemoteDescription() and createAnswer(), then createAnswer() can get invoked on PCImpl out of pc.onaddstream(), which you don't want.

Comment 10

[Adam Roach [:abr]]

Attachment: Queue CreateAnswer State Checks, remove extraneous _executeNext calls

Comment 11

[Adam Roach [:abr]]

Comment on attachment 706240 [details] [diff] [review]
Queue CreateAnswer State Checks, remove extraneous _executeNext calls

This patch removes some additional extraneous calls to _executeNext, on top of the ones EKR already identified as problematic. The changes pass mochi locally, although I haven't formulated any specific test cases to test the kinds of sequencing that would make this problem show up.

Given the relative urgency of landing this patch, I'd be inclined to push rigorous sequencing testing off onto another bug.

Comment 12

[Eric Rescorla (:ekr)]

Comment on attachment 706240 [details] [diff] [review]
Queue CreateAnswer State Checks, remove extraneous _executeNext calls

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

lgtm

Comment 13

[Adam Roach [:abr]]

Checked in:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e4e3590aa1da
http://hg.mozilla.org/integration/mozilla-inbound/rev/5a2c4f450e07

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4e3590aa1da
https://hg.mozilla.org/mozilla-central/rev/5a2c4f450e07