Closed Bug 840508 Opened 11 years ago Closed 11 years ago

Add thread asserts to nr_timer.cpp

Categories

(Core :: WebRTC: Networking, defect, P3)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: ekr, Assigned: ekr)

Details

(Whiteboard: [WebRTC] [blocking-webrtc+] [qa-])

Attachments

(1 file, 2 obsolete files)

nr_timer.cpp should only be used in the STS thread. Add asserts to make sure that is true.
Assignee: nobody → ekr
Whiteboard: [WebRTC] [blocking-webrtc+]
Priority: -- → P3
Attached patch nr_timer asserts (obsolete) — Splinter Review
Attached patch nr_timer asserts (obsolete) — Splinter Review
Attachment #715444 - Attachment is obsolete: true
Attachment #715446 - Flags: review?(adam)
Attached patch nr_timer assertsSplinter Review
Attachment #715446 - Attachment is obsolete: true
Attachment #715446 - Flags: review?(adam)
Attachment #715539 - Flags: review?(adam)
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+
https://hg.mozilla.org/mozilla-central/rev/0aac7d571ba0
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [WebRTC] [blocking-webrtc+] → [WebRTC] [blocking-webrtc+] [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: