555275 - IPDL: Implement DeallocShmem() and clean up open segments on shutdown

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Shmem segments currently just leak.  We need to add a DeallocShmem() interface for closing them at any time, and we also need to free still-open segments during DeallocSubtree().

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bug 548437 refactors shmem a bit and should land on m-c soon, don't want to merge.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Implement a DeallocShmem() interface

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Clean up shared memory segments when the top-level actor dies

Comment 4

[Benjamin Smedberg]

bent, this really needs to block the next devpreview, can you get to it?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 439187 [details] [diff] [review]
Implement a DeallocShmem() interface

>+Shmem::UnshareFrom(IHadBetterBeIPDLCodeCallingThis_OtherwiseIAmADoodyhead,

I know this has precendent but I feel weird r+'ing something with this in there...

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 439188 [details] [diff] [review]
Clean up shared memory segments when the top-level actor dies

>diff --git a/ipc/chromium/src/base/id_map.h b/ipc/chromium/src/base/id_map.h
>--- a/ipc/chromium/src/base/id_map.h
>+++ b/ipc/chromium/src/base/id_map.h
>@@ -67,16 +67,22 @@ class IDMap {
>     }
>     data_.erase(i);
>   }
> 
>   bool IsEmpty() const {
>     return data_.empty();
>   }
> 
>+#if defined(CHROMIUM_MOZILLA_BUILD)
>+  void Clear() {
>+    data_.clear();
>+  }
>+#endif
>+
>   T* Lookup(int32 id) const {
>     const_iterator i = data_.find(id);
>     if (i == data_.end())
>       return NULL;
>     return i->second;
>   }
> 
>   size_t size() const {
>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
>--- a/ipc/ipdl/ipdl/lower.py
>+++ b/ipc/ipdl/ipdl/lower.py
>@@ -1467,16 +1467,25 @@ class Protocol(ipdl.ast.Protocol):
>     def managerArrayExpr(self, thisvar, side):
>         """The member var my manager keeps of actors of my type."""
>         assert self.decl.type.isManaged()
>         return ExprSelect(
>             ExprCall(self.managerMethod(thisvar)),
>             '->', 'mManaged'+ _actorName(self.decl.type.name(), side))
> 
>     # shmem stuff
>+    def shmemMapType(self):
>+        assert self.decl.type.isToplevel()
>+        return Type('IDMap', T=_rawShmemType())
>+
>+    def shmemIteratorType(self):
>+        assert self.decl.type.isToplevel()
>+        # XXX breaks abstractions
>+        return Type('IDMap<SharedMemory>::const_iterator')
>+
>     def shmemMapVar(self):
>         assert self.decl.type.isToplevel()
>         return ExprVar('mShmemMap')
> 
>     def lastShmemIdVar(self):
>         assert self.decl.type.isToplevel()
>         return ExprVar('mLastShmemId')
> 
>@@ -2874,16 +2883,17 @@ class _GenerateProtocolActorCode(ipdl.as
>                 self.cls.addstmts([
>                     makeHandlerMethod('OnCallReceived', self.rpcSwitch,
>                                       hasReply=1, dispatches=dispatches),
>                     Whitespace.NL
>                 ])
> 
>         destroysubtreevar = ExprVar('DestroySubtree')
>         deallocsubtreevar = ExprVar('DeallocSubtree')
>+        deallocshmemvar = ExprVar('DeallocShmems')
> 
>         # OnReplyTimeout()
>         if toplevel.talksSync() or toplevel.talksRpc():
>             ontimeout = MethodDefn(
>                 MethodDecl('OnReplyTimeout', ret=Type.BOOL))
> 
>             if ptype.isToplevel():
>                 ontimeout.addstmt(StmtReturn(
>@@ -2915,30 +2925,32 @@ class _GenerateProtocolActorCode(ipdl.as
>             self.cls.addstmts([ onentered, onexited, onstack, Whitespace.NL ])
> 
>         # OnChannelClose()
>         onclose = MethodDefn(MethodDecl('OnChannelClose'))
>         if ptype.isToplevel():
>             onclose.addstmts([
>                 StmtExpr(ExprCall(destroysubtreevar,
>                                   args=[ _DestroyReason.NormalShutdown ])),
>-                StmtExpr(ExprCall(deallocsubtreevar))
>+                StmtExpr(ExprCall(deallocsubtreevar)),
>+                StmtExpr(ExprCall(deallocshmemvar))
>             ])
>         else:
>             onclose.addstmt(
>                 _runtimeAbort("`OnClose' called on non-toplevel actor"))
>         self.cls.addstmts([ onclose, Whitespace.NL ])
> 
>         # OnChannelError()
>         onerror = MethodDefn(MethodDecl('OnChannelError'))
>         if ptype.isToplevel():
>             onerror.addstmts([
>                 StmtExpr(ExprCall(destroysubtreevar,
>                                   args=[ _DestroyReason.AbnormalShutdown ])),
>-                StmtExpr(ExprCall(deallocsubtreevar))
>+                StmtExpr(ExprCall(deallocsubtreevar)),
>+                StmtExpr(ExprCall(deallocshmemvar))
>             ])
>         else:
>             onerror.addstmt(
>                 _runtimeAbort("`OnError' called on non-toplevel actor"))
>         self.cls.addstmts([ onerror, Whitespace.NL ])
> 
>         # FIXME/bug 535053: only manager protocols and non-manager
>         # protocols with union types need Lookup().  we'll give it to
>@@ -3118,17 +3130,40 @@ class _GenerateProtocolActorCode(ipdl.as
>                 foreachdealloc,
>                 StmtExpr(_callCxxArrayClear(p.managedVar(managed, self.side))),
> 
>             ])
>             deallocsubtree.addstmt(block)
>         # don't delete outselves: either the manager will do it, or
>         # we're toplevel
>         self.cls.addstmts([ deallocsubtree, Whitespace.NL ])
>-        
>+
>+        if ptype.isToplevel():
>+            ## DeallocShmem():
>+            #    for (cit = map.begin(); cit != map.end(); ++cit)
>+            #      Dealloc(cit->second)
>+            #    map.Clear()
>+            deallocshmem = MethodDefn(MethodDecl(deallocshmemvar.name))
>+
>+            citvar = ExprVar('cit')
>+            begin = ExprCall(ExprSelect(p.shmemMapVar(), '.', 'begin'))
>+            end = ExprCall(ExprSelect(p.shmemMapVar(), '.', 'end'))
>+            shmem = ExprSelect(citvar, '->', 'second')
>+            foreachdealloc = StmtFor(
>+                Param(p.shmemIteratorType(), citvar.name, begin),
>+                ExprBinary(citvar, '!=', end),
>+                ExprPrefixUnop(citvar, '++'))
>+            foreachdealloc.addstmt(StmtExpr(_shmemDealloc(shmem)))
>+
>+            deallocshmem.addstmts([
>+                foreachdealloc,
>+                StmtExpr(ExprCall(ExprSelect(p.shmemMapVar(), '.', 'Clear')))
>+            ])
>+            self.cls.addstmts([ deallocshmem, Whitespace.NL ])
>+
>         ## private members
>         self.cls.addstmt(StmtDecl(Decl(p.channelType(), 'mChannel')))
>         if ptype.isToplevel():
>             self.cls.addstmts([
>                 StmtDecl(Decl(Type('IDMap', T=Type('ChannelListener')),
>                               p.actorMapVar().name)),
>                 StmtDecl(Decl(_actorIdType(), p.lastActorIdVar().name)),
>                 StmtDecl(Decl(Type('ProcessHandle'),
>@@ -3137,18 +3172,17 @@ class _GenerateProtocolActorCode(ipdl.as
>         elif ptype.isManaged():
>             self.cls.addstmts([
>                 StmtDecl(Decl(_actorIdType(), p.idVar().name)),
>                 StmtDecl(Decl(p.managerInterfaceType(ptr=1),
>                               p.managerVar().name))
>             ])
>         if p.decl.type.isToplevel():
>             self.cls.addstmts([
>-                StmtDecl(Decl(Type('IDMap', T=_rawShmemType()),
>-                              p.shmemMapVar().name)),
>+                StmtDecl(Decl(p.shmemMapType(), p.shmemMapVar().name)),
>                 StmtDecl(Decl(_shmemIdType(), p.lastShmemIdVar().name))
>             ])
> 
>         for managed in ptype.manages:
>             self.cls.addstmts([
>                 Whitespace('// Sorted by pointer value\n', indent=1),
>                 StmtDecl(Decl(
>                     p.managedVarType(managed, self.side),

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #6)

Oops :(

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/4a02a072129b
http://hg.mozilla.org/mozilla-central/rev/9e2c5334e23b