Closed Bug 1037729 Opened 5 years ago Closed 5 years ago

Allow the stack to unroll for callbacks in PeerConnection.js

Categories

(Core :: WebRTC, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: mt, Assigned: mt)

Details

Attachments

(1 file)

Byron is talking about removing some of the unnecessary threading from sipcc to improve reliability, performance and to just generally reduce cruft.  If that happens, we'll be calling callbacks inline.  That's bad because it creates uncertainty about the order in which things execute, e.g.,

peerConnection.someFunction(options, result => a(), error => b(), ...);
c();

Currently, because we have thread dispatches, a() or b() are usually called after c().  Sometimes not for b(), because we detect some errors synchronously, but not all of them.

That sort of uncertainty is awesome as a programmer.  It also leads to spectacularly long stacks.  We should fix callCB so that it schedules the callback for the next crank of the event loop.
Can you give an example of when this would happen? I would think things would have to be synchronous all the way through on main-thread only which doesn't sound like the sipcc I know. Can we link this to where Byron's talking?

Btw, for more awesome uncertainty, look at Bug 1020024 comment 111.
So, bug 991037 is where I'm working on the sipcc threading stuff. While removing async dispatches, I found that we'd loop through createOffer/setLocal or even setRemote/createAnswer/setLocal on one call stack, which some webrtc services didn't really like.
I think that the determinism argument is pretty compelling.  Take a look at createOffer.  Assuming that there is nothing on the queue (the normal case), then if the C++ code calls back synchronously, we run the callback before any subsequent code in the calling context.  That's bad.
And an erroneous call to createAnswer already has this problem.
Turns out that we need to do this for spec reasons too.  Note the consistent use of the form: "the user agent MUST queue a task to invoke successCallback" throughout the WebRTC spec.

Runs fine on my machine, but in the interests of expediting this...

https://tbpl.mozilla.org/?tree=Try&rev=a4e9773073a8
Attachment #8456318 - Flags: review?(jib)
Keywords: checkin-needed
Comment on attachment 8456318 [details] [diff] [review]
0001-Bug-1037729-Moving-callbacks-to-run-after-reaching-s.patch

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

lgtm.
Attachment #8456318 - Flags: review?(jib) → review+
Whoops, looked like I jumped the gun on this one; thanks :jib.
https://hg.mozilla.org/mozilla-central/rev/31c23d75e539
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.