Signaling doesn't clean up message queues in /tmp (seen on linux+mac)

RESOLVED FIXED in mozilla20

Status

()

Core
WebRTC: Signaling
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jesup, Assigned: jib)

Tracking

(Blocks: 1 bug)

Trunk
mozilla20
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments, 6 obsolete attachments)

16.81 KB, patch
jib
: review+
jesup
: checkin+
Details | Diff | Splinter Review
10.66 KB, text/plain
Details
10.44 KB, patch
jesup
: review+
ehugg
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
We're leaking SIPCC message queues:

$ ls -l /tmp | grep SIP-MsgQ | wc
     78     702    6144

Note that it goes up even if I exit "cleanly" via close.

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc-]

Comment 1

5 years ago
Adding to this that EKR has requested the tmp files be attributed 700 instead of 755 per m-c landing review.
(Reporter)

Comment 2

5 years ago
Jan-Ivar - this may be a good next bug; it's in the signaling code
(Assignee)

Updated

5 years ago
Assignee: nobody → jib
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Summary: Signaling doesn't clean up message queues in /tmp on linux → Signaling doesn't clean up message queues in /tmp on linux+mac
(Assignee)

Updated

5 years ago
Summary: Signaling doesn't clean up message queues in /tmp on linux+mac → Signaling doesn't clean up message queues in /tmp (seen on linux+mac)
(Assignee)

Comment 3

5 years ago
Created attachment 689908 [details] [diff] [review]
Prelim patch fixing the SIP* messages only

I've fixed the SIP-* sockets. It's now closing and deleting those cleanly.

Moving on to:
2. the CprTmr* sockets. Same problem, but cpr_timer_de_init() is never called.
3. Fix permissions in /tmp on all four sockets. I plan to chmod 700 on a mkstemp() dir and put sockets inside so they'll inherit the right permissions.
(Assignee)

Updated

5 years ago
Attachment #689908 - Attachment is patch: true
(Assignee)

Comment 4

5 years ago
I have a shutdown question: cpr_timer_de_init() @ http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c#935 isn't called anywhere. Do we think it was meant to be called on the timer thread itself or on the main thread after the timer thread is gone? Should I pass an unload msg, or is that overkill for a timer thread?
(Assignee)

Comment 5

5 years ago
ehugg and crypt think no-one uses the cpr timers, so I'll test commenting them out.
(Assignee)

Comment 6

5 years ago
Created attachment 691395 [details] [diff] [review]
bug--fix: Closes sockets + cleans up tmp-files on shutdown + file permissions
(Assignee)

Comment 7

5 years ago
Created attachment 691401 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
(Assignee)

Updated

5 years ago
Attachment #691395 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
Created attachment 692166 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
(Assignee)

Updated

5 years ago
Attachment #691401 - Attachment is obsolete: true
(Assignee)

Comment 9

5 years ago
Created attachment 692372 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions
(Assignee)

Updated

5 years ago
Attachment #692166 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #692372 - Flags: review?(rjesup)
(Reporter)

Comment 10

5 years ago
Comment on attachment 692372 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions

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

r+ with nits fixed

::: media/webrtc/signaling/signaling.gyp
@@ +583,5 @@
>        
>        'defines' : [
> +      # CPR timers are needed by SIP, but are disabled for now
> +      # to avoid the extra timer thread and stale cleanup code
> +      #    'CPR_TIMERS_ENABLED', 

trailing whitespace

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_socket.c
@@ +773,5 @@
>  {
>      /* Bind to the local socket */
>      memset(addr, 0, sizeof(cpr_sockaddr_un_t));
>      addr->sun_family = AF_UNIX;
> +    PR_snprintf(addr->sun_path, sizeof(addr->sun_path), name, pid);

snprintf - this is mac-only code where it exists and is safe

::: media/webrtc/signaling/src/sipcc/cpr/darwin/cpr_darwin_timers_using_select.c
@@ +226,5 @@
>   */
>  static cprRC_t addTimerToList (cpr_timer_t *cprTimerPtr, uint32_t duration, void *data)
>  {
> +    static const char fname[] = "addTimerToList";
> +

remove

@@ +270,5 @@
>          API_RETURN(tmr_rsp.u.result);
>      }
> +#else
> +    cprAssert(FALSE, CPR_FAILURE);
> +    CPR_ERROR("CPR Timers are disabled! %s\n", fname);

__FUNCTION__

::: media/webrtc/signaling/src/sipcc/cpr/linux/cpr_linux_socket.c
@@ +748,5 @@
>  {
>      /* Bind to the local socket */
>      memset(addr, 0, sizeof(cpr_sockaddr_un_t));
>      addr->sun_family = AF_LOCAL;
> +    PR_snprintf(addr->sun_path, sizeof(addr->sun_path), name, pid);

snprintf - exists and is safe in linux

::: media/webrtc/signaling/src/sipcc/cpr/win32/cpr_win_socket.c
@@ +741,5 @@
>      char hostAndPort[HOST_AND_PORT_SIZE];
>  
>      cprAssert(host != NULL, CPR_FAILURE);
>      cprAssert(localPort != NULL, CPR_FAILURE);
> +    PR_snprintf(hostAndPort, HOST_AND_PORT_SIZE, "%s:%d", host, port);

snprintf - defined somewhere in signaling as safe

@@ +1083,5 @@
>    /* COPIED FROM OTHER PLATFORM AS A PLACE HOLDER */
>    /* Bind to the local socket */
>    memset(addr, 0, sizeof(cpr_sockaddr_un_t));
>    addr->sun_family = AF_INET;
> +  PR_snprintf(addr->sun_path, sizeof(addr->sun_path), name, pid);

ditto
Attachment #692372 - Flags: review?(rjesup) → review+

Updated

5 years ago
Blocks: 821812
(Assignee)

Comment 12

5 years ago
Created attachment 692423 [details] [diff] [review]
bug--fix: Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.
(Assignee)

Updated

5 years ago
Attachment #692372 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Created attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.
(Assignee)

Updated

5 years ago
Attachment #692423 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Comment on attachment 692437 [details] [diff] [review]
Disable timerthread + close sockets + cleanup tmp-files on shutdown + file permissions.

Carrying forward r+ from Randell.
Attachment #692437 - Flags: review+
Attachment #692437 - Flags: checkin?(rjesup)
(Reporter)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c073841902c
Target Milestone: --- → mozilla20
(Reporter)

Updated

5 years ago
Attachment #692437 - Flags: checkin?(rjesup) → checkin+
Created attachment 693747 [details]
bustage

I'm getting link bustage.  I bisected it to this patch.  The exact output is attached.  The problem seems to be with PR_snprintf and/or mkdir.

I'm on Ubuntu 12.04, using clang 3.2 and gold 1.11.
The problem doesn't happen unless --enable-tests is specified, so it's a problem with the unit tests.
Actually, it might be --enable-debug that's causing the problem... not sure yet.
Missing includes in media/webrtc/signaling/src/sipcc/core/sipstack/sip_platform_task.c are the reason for Nicholas' build failure.
  #include <sys/stat.h>
  #include "prprf.h"
Depends on: 822978
(Assignee)

Comment 20

5 years ago
Thanks Nick and Mike! Sorry, I'm a newbie. I'll add another patch to this bug and get it reviewed and checked in asap. Odd, inbound is still showing as green and it doesn't appear to have been backed out. Did this break the tree?

Mike, I see you added a dependency, do I need to hold off making a fix?
(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> Thanks Nick and Mike! Sorry, I'm a newbie. I'll add another patch to this
> bug and get it reviewed and checked in asap. Odd, inbound is still showing
> as green and it doesn't appear to have been backed out. Did this break the
> tree?

It breaks the tree for some people, not on build bots.
 
> Mike, I see you added a dependency, do I need to hold off making a fix?

No, the dependency is just not in the right direction.
Blocks: 822978
No longer depends on: 822978
(Assignee)

Comment 22

5 years ago
Created attachment 693939 [details] [diff] [review]
Followup: Fixes implicit function warning that broke linkage + symmetric fixes

This adds the missing includes that broke linkage + also, dmose pointed out that not all changes were applied symmetrically (to android, darwin, linux and win32), now fixed.
Attachment #693939 - Flags: review?(rjesup)
(Assignee)

Comment 23

5 years ago
Note that the last patch is a follow-up, to be applied on-top of the first patch that has already landed on inbound.
(Assignee)

Updated

5 years ago
Attachment #689908 - Attachment is obsolete: true
(Reporter)

Updated

5 years ago
Attachment #693939 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/5c073841902c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Attachment #693939 - Flags: checkin?(ethanhugg)
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 25

5 years ago
Comment on attachment 693939 [details] [diff] [review]
Followup: Fixes implicit function warning that broke linkage + symmetric fixes


http://hg.mozilla.org/integration/mozilla-inbound/rev/bafbc8d9c6b8
Attachment #693939 - Flags: checkin?(ethanhugg) → checkin+
https://hg.mozilla.org/mozilla-central/rev/bafbc8d9c6b8
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [WebRTC], [blocking-webrtc-] → [WebRTC], [blocking-webrtc-] [qa-]

Updated

5 years ago
Duplicate of this bug: 740254
You need to log in before you can comment on or make changes to this bug.