Closed
Bug 840508
Opened 11 years ago
Closed 11 years ago
Add thread asserts to nr_timer.cpp
Categories
(Core :: WebRTC: Networking, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: ekr, Assigned: ekr)
Details
(Whiteboard: [WebRTC] [blocking-webrtc+] [qa-])
Attachments
(1 file, 2 obsolete files)
2.32 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
nr_timer.cpp should only be used in the STS thread. Add asserts to make sure that is true.
Updated•11 years ago
|
Assignee: nobody → ekr
Whiteboard: [WebRTC] [blocking-webrtc+]
Updated•11 years ago
|
Priority: -- → P3
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #715444 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #715446 -
Flags: review?(adam)
Assignee | ||
Comment 3•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #715446 -
Attachment is obsolete: true
Attachment #715446 -
Flags: review?(adam)
Assignee | ||
Updated•11 years ago
|
Attachment #715539 -
Flags: review?(adam)
Comment 4•11 years ago
|
||
Comment on attachment 715539 [details] [diff] [review] nr_timer asserts Review of attachment 715539 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. Some nits inline. Not inline, but also a nit: there are three places where this patch introduces new trailing white space. ::: media/mtransport/nr_timer.cpp @@ +105,5 @@ > } // close namespace > > > using namespace mozilla; > I'd think a comment block here (indicating that timers must be used from the STS thread, and that this function is used to enforce the contract) would be useful. @@ +146,5 @@ > return 0; > } > > int NR_async_schedule(NR_async_cb cb, void *arg, char *func, int l) { > + CheckSTSThread(); Given that this is just sugar around NR_async_timer_set (which already checks the thread), checking the thread here seems a bit excessive. If do_GetService is very cheap, I guess this isn't a big issue; but I'm worried that we're doing double work here to no effect.
Attachment #715539 -
Flags: review?(adam) → review+
Assignee | ||
Comment 5•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aac7d571ba0
Comment 6•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0aac7d571ba0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Updated•11 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+] [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•