Last Comment Bug 853590 - Remove cprGet/ReleaseMutex from Signaling Code
: Remove cprGet/ReleaseMutex from Signaling Code
Status: RESOLVED FIXED
[WebRTC][blocking-webrtc-][qa-]
:
Product: Core
Classification: Components
Component: WebRTC: Signaling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla23
Assigned To: Charith Tangirala
: Jason Smith [:jsmith]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-21 12:27 PDT by Ethan Hugg [:ehugg]
Modified: 2013-04-02 20:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove unused mutex impl from CPR (17.94 KB, patch)
2013-03-29 08:57 PDT, Charith Tangirala
no flags Details | Diff | Splinter Review
Remove unused mutex impl from CPR (17.94 KB, patch)
2013-03-29 09:09 PDT, Charith Tangirala
no flags Details | Diff | Splinter Review
Remove unused mutex impl from CPR (21.04 KB, patch)
2013-03-29 11:45 PDT, Charith Tangirala
no flags Details | Diff | Splinter Review
Remove unused mutex impl from CPR (38.94 KB, patch)
2013-04-01 13:13 PDT, Charith Tangirala
no flags Details | Diff | Splinter Review
Remove unused mutex impl from CPR (41.87 KB, patch)
2013-04-01 15:35 PDT, Charith Tangirala
rjesup: review+
Details | Diff | Splinter Review

Description Ethan Hugg [:ehugg] 2013-03-21 12:27:35 PDT
Signaling code still has instances of cprGet/ReleaseMutex in kpmlmap.c and is defined in cpr for all OS targets.  These functions should be removed entirely.  The instances in kpml can be either replaces with mozilla/Mutex or the kpml functions can be stubbed out since we don't currently support kpml.
Comment 1 Ethan Hugg [:ehugg] 2013-03-21 12:29:22 PDT
Assigning to our new Cisco volunteer Charith Tangirala
Comment 2 Jason Smith [:jsmith] 2013-03-27 19:10:29 PDT
This reads off as code cleanup, which would mean this doesn't block.
Comment 3 Ethan Hugg [:ehugg] 2013-03-27 19:36:08 PDT
Yes, this is code cleanup. Things should run exactly the same after this patch.
Comment 4 Charith Tangirala 2013-03-29 08:57:07 PDT
Created attachment 731188 [details] [diff] [review]
Remove unused mutex impl from CPR
Comment 5 Charith Tangirala 2013-03-29 09:09:48 PDT
Created attachment 731192 [details] [diff] [review]
Remove unused mutex impl from CPR
Comment 6 Ethan Hugg [:ehugg] 2013-03-29 09:26:28 PDT
Comment on attachment 731192 [details] [diff] [review]
Remove unused mutex impl from CPR

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

Unless there's a reason to keep cprCreateMutex and cprDestroyMutex you should remove those as well.  Update with this patch applied, then do an 'hg qref' and another bzexport.  If it looks OK, then I'll run a Try for you.

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_locks.c
@@ +72,3 @@
>      cprMutexPtr = (cpr_mutex_t *) cpr_malloc(sizeof(cpr_mutex_t));
>      if (cprMutexPtr != NULL) {
> +         Assign name if one was passed in

These changes look suspicious, did you mean to remove the comment chars?  I'm guessing you meant to remove these altogether.
Comment 7 Charith Tangirala 2013-03-29 11:45:29 PDT
Created attachment 731242 [details] [diff] [review]
Remove unused mutex impl from CPR
Comment 8 Ethan Hugg [:ehugg] 2013-03-29 14:56:05 PDT
Pushed to try - https://tbpl.mozilla.org/?tree=Try&rev=25410da1cccf

If this builds green, I'll ask for review probably from Jesup.
Comment 9 Ethan Hugg [:ehugg] 2013-03-29 16:07:19 PDT
Looks like from the try build that we missed a reference to cprCreateMutex in the windows builds.
Comment 10 Ethan Hugg [:ehugg] 2013-03-29 17:34:09 PDT
A quick grep -r of the signaling tree shows cprGet/Release/DeleteMutex are gone but there are still many instances of cprCreateMutex
Comment 11 Charith Tangirala 2013-04-01 13:13:37 PDT
Created attachment 732011 [details] [diff] [review]
Remove unused mutex impl from CPR
Comment 12 Ethan Hugg [:ehugg] 2013-04-01 13:32:55 PDT
Got this on my Linux box with this patch:

/home/ehugg/mozilla/mozilla-central/media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_init.c:116:29: fatal error: cpr_linux_locks.h: No such file or directory
compilation terminated.

Please 'grep -r cpr_linux_locks.h' the signaling directory for each file you removed to find any more.
Comment 13 Charith Tangirala 2013-04-01 15:35:38 PDT
Created attachment 732097 [details] [diff] [review]
Remove unused mutex impl from CPR
Comment 14 Ethan Hugg [:ehugg] 2013-04-01 16:29:34 PDT
Try run with new patch - https://tbpl.mozilla.org/?tree=Try&rev=6fbc4904027c
Comment 15 Ethan Hugg [:ehugg] 2013-04-01 19:57:30 PDT
Comment on attachment 732097 [details] [diff] [review]
Remove unused mutex impl from CPR


Try run is green.  Requesting review from Randell Jesup
Comment 17 Ryan VanderMeulen [:RyanVM] 2013-04-02 19:35:12 PDT
https://hg.mozilla.org/mozilla-central/rev/5ec9994badfc

Note You need to log in before you can comment on or make changes to this bug.