Closed
Bug 1037729
Opened 10 years ago
Closed 10 years ago
Allow the stack to unroll for callbacks in PeerConnection.js
Categories
(Core :: WebRTC, defect)
Core
WebRTC
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: mt, Assigned: mt)
Details
Attachments
(1 file)
1.41 KB,
patch
|
jib
:
review+
|
Details | Diff | Splinter Review |
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.
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
And an erroneous call to createAnswer already has this problem.
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
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+
Assignee | ||
Comment 7•10 years ago
|
||
Whoops, looked like I jumped the gun on this one; thanks :jib.
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/31c23d75e539
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/31c23d75e539
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•