w3c / webrtc-pc

WebRTC 1.0 API
https://w3c.github.io/webrtc-pc/
Other
439 stars 115 forks source link

statechange fired synchronously before state on RTCSctpTransport; no event on closed #2622

Open jan-ivar opened 3 years ago

jan-ivar commented 3 years ago

"Once an SCTP transport is connected" we fail to queue a task when we "... Fire an event named statechange at transport."

This violates years of precedence to not alter observable state while JS is running:

pc.sctp.addEventListener("statechange", A);
for (let i = 0; i < N; i++) ; // busy-loop
pc.sctp.addEventListener("statechange", B); // A and B should receive same # of events for all values of N 

An oversight, as it seems uncontroversial this algorithm was meant to be in a queued task like all the others.

But the algorithm also fails to set [[SctpTransportState]] before firing, breaking obvious behavior:

pc.sctp.onstatechange = () => console.log(pc.sctp.state); // should be new state, not previous state!

When I click on [[SctpTransportState]] in the spec, it shows only two references: the state getter and the pc close algorithm, which at first made me think we never update any state but "closed" from pc.close().

But prose to queue a task to update state is hiding in the enum descriptions, E.g. "When the negotiation of an association is completed, a task is queued to update the [[SctpTransportState]] slot to "connected". " Which is good I suppose, as it ensures:

const A = pc.sctp.state;
for (let i = 0; i < N; i++) ; // busy-loop
const B = pc.sctp.state; // A and B should be identical for all values of N

But this is after the statechange event has fired in the "connected" case, and fires no event at all in the "closed" case, which seems wrong since there are cases like remote close that isn't covered by the pc close algorithm.

PS: This may be causing a Firefox bug found on StackOverflow, and fixing it should help cement the already noted intent to have data channel openfire after all this.

jan-ivar commented 3 years ago

To clarify, I think the intended behavior here is clear and discernible in spite of these bugs: always queue a task to set state and fire relevant change events immediately after, and queue a second task to open any data channels.

nztyrone36 commented 3 years ago

On Sun, 7 Feb 2021 at 4:54 AM, Jan-Ivar Bruaroey notifications@github.com wrote:

To clarify, I think the intended behavior here is clear and discernible in spite of these bugs: always queue a task to set state and fire relevant change events immediately after, and queue a second task to open any data channels.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/w3c/webrtc-pc/issues/2622#issuecomment-774497903, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIR6PTWLSULD5HBMFA3UXETS5VQ3LANCNFSM4XGMMUMA .

What’s this all about

alvestrand commented 3 years ago

@dontcallmedom what's the procedure for incorporating an errata like this into the REC?

dontcallmedom commented 3 years ago

the first step is to document it in our errata page.

We can also fix the editors draft maintained in this repo without further ado.

Assuming we don't think the change is editorial, we can bring that change to the TR published version following the updated Recommendation process with the candidate correction. While this isn't a very onerous process, we may want to collect a few of these corrections (e.g. https://github.com/w3c/webrtc-extensions/issues/69) (and even, possibly new features) before starting it.