545924 - IPDL: Invoke actor->Dealloc*() on ExitedCxxStack() callbacks

Status: NEW

(Core :: IPC, defect)

Description

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

Even with stateful protocols and non-racy dtors, rpc messages make handling __delete__() at the top of the stack a dicey matter, because |this| can be deleted out from under a lot of |this| stack frames.  *Racy* rpc (even if the race is between two non-__delete__ messages) complicate things further.

When we have non-racy __delete__, we also need this feature to convert use-after-__delete__ errors into protocol errors when possible rather than segfaults.

Toplevel actors will instead maintain an mDeallocQueue; on __delete__(), the deleted actor with DestroySubtree() itself, but instead of immediately invoking DeallocSubtree(), enqueue itself in mDeallocQueue.  The toplevel actor will override OnExitedCxxStack(), first invoking the concrete actor's ExitedCxxStack() hook, then flushing mDeallocQueue.  It's important that this be a queue so that Dealloc()s happen in the same order as the __delete__()s.

We only need this for rpc protocol trees, but unfortunately that's all two of them so far.

It's not a high-priority goal, but I'm pretty sure this feature will work around bug 543917 as a side effect.

Comment 1

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

Attachment: Defer Dealloc*() of actors until all IPDL-related code is off the C++ stack.

Comment 2

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

I had thought this would work around the problem in bug 543917, but I still see a (different) crash

==9600== Invalid read of size 8
==9600==    at 0x686AD0D: mozilla::plugins::PPluginModuleParent::FlushDeallocQ() (PPluginModuleParent.cpp:718)
==9600==    by 0x686A790: mozilla::plugins::PPluginModuleParent::OnExitedCxxStack() (PPluginModuleParent.cpp:553)
==9600==    by 0x68621E9: mozilla::ipc::RPCChannel::ExitedCxxStack() (RPCChannel.cpp:583)
==9600==    by 0x6862BBD: mozilla::ipc::RPCChannel::CxxStackFrame::~CxxStackFrame() (RPCChannel.h:183)
==9600==    by 0x68617F9: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:369)
==9600==    by 0x6865FBB: void DispatchToMethod<mozilla::ipc::RPCChannel, void (mozilla::ipc::RPCChannel::*)()>(mozilla::ipc::RPCChannel*, void (mozilla::ipc::RPCChannel::*)(), Tuple0 const&) (tuple.h:383)
==9600==    by 0x6865E11: RunnableMethod<mozilla::ipc::RPCChannel, void (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run() (task.h:307)
==9600==    by 0x6940729: MessageLoop::RunTask(Task*) (message_loop.cc:331)
==9600==    by 0x6940799: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (message_loop.cc:339)
==9600==    by 0x6940B97: MessageLoop::DoWork() (message_loop.cc:439)
==9600==    by 0x685F45C: mozilla::ipc::DoWorkRunnable::Run() (MessagePump.cpp:75)
==9600==    by 0x6A46D80: nsThread::ProcessNextEvent(int, int*) (nsThread.cpp:527)
==9600==  Address 0x1a344468 is 8 bytes inside a block of size 64 free'd
==9600==    at 0x4C24A7A: operator delete(void*) (vg_replace_malloc.c:346)
==9600==    by 0x68593F4: mozilla::plugins::BrowserStreamParent::~BrowserStreamParent() (BrowserStreamParent.cpp:19)
==9600==    by 0x6843229: mozilla::plugins::PluginInstanceParent::DeallocPBrowserStream(mozilla::plugins::PBrowserStreamParent*) (PluginInstanceParent.cpp:171)
==9600==    by 0x68771A2: mozilla::plugins::PPluginInstanceParent::RemoveManagee(int, mozilla::ipc::RPCChannel::RPCListener*) (PPluginInstanceParent.cpp:1430)
==9600==    by 0x688997D: mozilla::plugins::PBrowserStreamParent::Dealloc() (PBrowserStreamParent.cpp:400)
==9600==    by 0x686AD1F: mozilla::plugins::PPluginModuleParent::FlushDeallocQ() (PPluginModuleParent.cpp:718)
==9600==    by 0x686A790: mozilla::plugins::PPluginModuleParent::OnExitedCxxStack() (PPluginModuleParent.cpp:553)
==9600==    by 0x68621E9: mozilla::ipc::RPCChannel::ExitedCxxStack() (RPCChannel.cpp:583)
==9600==    by 0x6862BBD: mozilla::ipc::RPCChannel::CxxStackFrame::~CxxStackFrame() (RPCChannel.h:183)
==9600==    by 0x68617F9: mozilla::ipc::RPCChannel::OnMaybeDequeueOne() (RPCChannel.cpp:369)
==9600==    by 0x6865FBB: void DispatchToMethod<mozilla::ipc::RPCChannel, void (mozilla::ipc::RPCChannel::*)()>(mozilla::ipc::RPCChannel*, void (mozilla::ipc::RPCChannel::*)(), Tuple0 const&) (tuple.h:383)
==9600==    by 0x6865E11: RunnableMethod<mozilla::ipc::RPCChannel, void (mozilla::ipc::RPCChannel::*)(), Tuple0>::Run() (task.h:307)


This error implies that we not only use-after-free, but double delete (why we're not getting an IPDL use-after-__delete__ error, I'm not quite sure of yet).  We could hack around double-delete by ensuring that mDeallocQ doesn't contain duplicates.  If the remaining work towards bug 532208 takes a while, it be worth doing.

Comment 3

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

Attachment: Defer Dealloc*() of actors until all IPDL-related code is off the C++ stack, unbitrotted

Comment 4

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

Comment on attachment 427196 [details] [diff] [review]
Defer Dealloc*() of actors until all IPDL-related code is off the C++ stack, unbitrotted

Looks good!

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:jld, could you have a look please?
For more information, please visit auto_nag documentation.