Open Bug 1169386 Opened 9 years ago Updated 2 years ago

Avoid queuing a task ahead of peerConnection operations in the non-corner case.

Categories

(Core :: WebRTC, defect, P4)

Product:
Core
Component:
WebRTC
Version:
37 Branch
Type:
defect

Tracking

()

Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jib, Assigned: jib)

References

(
URL
)

Details

(Whiteboard: [spec compliance])

Attachments

(2 files)

MozReview Request: Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.
39 bytes, text/x-review-board-request
mt
: review+
Details
MozReview Request: Bug 1169386 - test addTrack(X); createOffer(); addTrack(Y);
40 bytes, text/x-review-board-request
mt
: review+
Details 
For spec-compliance, we should not queue a task before executing createOffer/setLocalDescription/setRemoteDescription/AddIceCandidate when there are no pending operations.

Spec behavior is: Running sync on empty is the common case, whereas chaining is a corner-case.
This means that we want:

  pc.addTrack(X); pc.createOffer(); pc.addTrack(Y);

where Y is never part of the offer. The exception is in:

  pc.setRemoteDescription(desc); // not checking for failure!
  pc.addTrack(X); pc.createOffer(); pc.addTrack(Y);

where Y IS part of the offer, by nature of the createOffer being deferred.
This is considered a natural outcome of a deprecated pattern. Use promises.

This regressed in 37 (Bug 1106675) when we started always chaining operations (Y is always part of the offer), a well-intended move, but nevertheless non-compliant.
Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.
Attachment #8612474 - Flags: review?(martin.thomson)
Comment on attachment 8612474 [details]
MozReview Request: Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.

Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.
Realizing from trip-up above that we don't have a test for this. Will add one shortly.
https://reviewboard.mozilla.org/r/9583/#review8365

I think that we probably want to test this somehow.  Maybe add a clause to dom/media/tests/mochitest/test_peerConnection_syncSetDescription.html

::: dom/media/PeerConnection.js:443
(Diff revision 2)
> +    this._operations = p.then(() => this._operationsLength--,
> +                              () => this._operationsLength--);

Why not `.catch(_ => {}).then(_ => --this._operationsLength)`

::: dom/media/PeerConnection.js:430
(Diff revision 2)
> -    let p = this._operationsChain.then(() => {
> +    let p = (!this._operationsLength)? func() : this._operations.then(() => {

Are we *certain* that func() returns a promise?  It might make sense to wrap it:
    Promise.resolve(func())

Also, don't use the ?: operator here.  An if statement will do.
And I see that you have already volunteered to create a test.  We must be doing something right (and I need to review the bug before reviewing the patch...)
(In reply to Martin Thomson [:mt] from comment #5)
> ::: dom/media/PeerConnection.js:443
> (Diff revision 2)
> > +    // Note: Decrement must run before other p.then's, so don't extend fork!
> > +    this._operations = p.then(() => this._operationsLength--,
> > +                              () => this._operationsLength--);
> 
> Why not `.catch(_ => {}).then(_ => --this._operationsLength)`

Because that would extend the fork and run afoul of the comment above it ^

> ::: dom/media/PeerConnection.js:430
> (Diff revision 2)
> > -    let p = this._operationsChain.then(() => {
> > +    let p = (!this._operationsLength)? func() : this._operations.then(() => {
> 
> Are we *certain* that func() returns a promise?  It might make sense to wrap
> it:
>     Promise.resolve(func())

I'm certain. That would mean chaining a synchronous method or an async method that forgot to return its promise (a timing nightmare), so I think I would rather have a JS error in that case. 

> Also, don't use the ?: operator here.  An if statement will do.

That was my favorite part.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> (In reply to Martin Thomson [:mt] from comment #5)
> > ::: dom/media/PeerConnection.js:443
> > (Diff revision 2)
> > > +    // Note: Decrement must run before other p.then's, so don't extend fork!
> > > +    this._operations = p.then(() => this._operationsLength--,
> > > +                              () => this._operationsLength--);
> > 
> > Why not `.catch(_ => {}).then(_ => --this._operationsLength)`
> 
> Because that would extend the fork and run afoul of the comment above it ^

OK, I really don't having to rely on races.  That's what cause the last mess.  Can you put the counter decrement in differently?

if (this._operationsLength > 0) {
  p = func();
} else {
  p = this._operationsChain.then(() => func()); // add the closed check, I'm lazy.
}
++this._operationsLength;
p = p.then(r => {
  --this._operationsLength;
  return r;
}, e => {
  --this._operationsLength;
  throw e;
});
this._operationsChain = p.catch(() => {});
(In reply to Martin Thomson [:mt:] from comment #8)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> > (In reply to Martin Thomson [:mt] from comment #5)
> > > ::: dom/media/PeerConnection.js:443
> > > (Diff revision 2)
> > > > +    // Note: Decrement must run before other p.then's, so don't extend fork!
> > > > +    this._operations = p.then(() => this._operationsLength--,
> > > > +                              () => this._operationsLength--);
> > > 
> > > Why not `.catch(_ => {}).then(_ => --this._operationsLength)`
> > 
> > Because that would extend the fork and run afoul of the comment above it ^
> 
> OK, I really don't having to rely on races.  That's what cause the last
> mess.

This is actually not a "race" but well-defined. Multiple p.then()s is guaranteed FIFO.

>  Can you put the counter decrement in differently?
> 
> if (this._operationsLength > 0) {
>   p = func();
> } else {
>   p = this._operationsChain.then(() => func()); // add the closed check, I'm
> lazy.
> }
> ++this._operationsLength;
> p = p.then(r => {
>   --this._operationsLength;
>   return r;
> }, e => {
>   --this._operationsLength;
>   throw e;
> });
> this._operationsChain = p.catch(() => {});

I considered this, but it adds an extra dispatch always for no reason, upsetting the timing of the passed-in function, a worse maintenance headache imho.
I mean multiple .then()s on the same promise.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> I considered this, but it adds an extra dispatch always for no reason,
> upsetting the timing of the passed-in function, a worse maintenance headache
> imho.

I could care less about the extra dispatch, but I'm interested in what you think might be affected by this.  If func() is setLocalDescription(), how is this extra dispatch any different from that function just taking one extra cycle to do its work?

Ultimately, this is just wallpapering over the real problem, which is that the operations queue is fundamentally broken.  How about we fix that in the spec instead (and until then, this will have to suffice).
Why do you think the operations queue is broken?
It's "special" and that means we have long discussions like this.

Other APIs don't try to add all affordances for programmers that don't know how to use them properly, they throw if methods are invoked from the wrong state.  We should too.
I don't think that the issue is that that things are being invoked
in the wrong state, but rather that without the queue you are
requiring things to be done in lockstep which is a pain.
(In reply to Martin Thomson [:mt:] from comment #11)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> > I considered this, but it adds an extra dispatch always for no reason,
> > upsetting the timing of the passed-in function, a worse maintenance headache
> > imho.
> 
> I could care less about the extra dispatch, but I'm interested in what you
> think might be affected by this.  If func() is setLocalDescription(), how is
> this extra dispatch any different from that function just taking one extra
> cycle to do its work?

We never landed the patch in Bug 1157042, so this would likely do actual damage to the time-sensitive setLocalDescription (which is implemented across c++ and JS) by delaying the SetLD success callback until after the first onicecandidate fires from c++ (through PeerConnection.js to the user).
backlog: --- → webRTC+
Rank: 30
Priority: -- → P3
Whiteboard: [spec compliance]
Comment on attachment 8612474 [details]
MozReview Request: Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.

Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.
Attachment #8612474 - Flags: review?(martin.thomson)
Bug 1169386 - test addTrack(X); createOffer(); addTrack(Y);
Attachment #8615468 - Flags: review?(martin.thomson)
Depends on: 1171634
Attachment #8612474 - Flags: review?(martin.thomson)
Comment on attachment 8612474 [details]
MozReview Request: Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.

Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.
Comment on attachment 8615468 [details]
MozReview Request: Bug 1169386 - test addTrack(X); createOffer(); addTrack(Y);

Bug 1169386 - test addTrack(X); createOffer(); addTrack(Y);
Note that the tests will fail because of Bug 1171634.
https://reviewboard.mozilla.org/r/9583/#review9227

Our increasingly brittle timing still bothers me a great deal.

::: dom/media/tests/mochitest/test_peerConnection_operations.html:26
(Diff revision 4)
> +                    .reduce((a,l) => (l.startsWith("m=") && a.push(l), a), [])

This is unnecessarily cryptic.  I find your aversion to braces a little too aggressive.

is(offer.sdp.split('\nm='), 2)
Comment on attachment 8615468 [details]
MozReview Request: Bug 1169386 - test addTrack(X); createOffer(); addTrack(Y);

Gah, you get used to reviewing the whole block, and now you have to individually r+ each piece.
Attachment #8615468 - Flags: review?(martin.thomson) → review+
Comment on attachment 8612474 [details]
MozReview Request: Bug 1169386 - avoid queueing a task ahead of pc operations in the non-corner case.

I still don't like the brittle timing here.
Attachment #8612474 - Flags: review?(martin.thomson) → review+
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
Severity: minor → S4
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: