dangling AbstractThread pointer of PBackground thread after MessageChannel is destroyed

RESOLVED FIXED in Firefox 55

Status

()

Core
IPC
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: schien, Assigned: kanru)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Random test case failed on try while I was writing patches for PBackground-ify HTTP ODA using the new async IPC return value mechanism.
It turns out that the life time of PBackground Thread is longer than the MessageChannel, so the instance of AbstractThread will be destroyed but a raw pointer stored in thread-local storage is still kept. Therefore, AbstractThread::GetCurrent() will get the dangling pointer.
[call stack when free]
>#0  0x000000000041b80e in arena_dalloc (__len=32, __ch=229, __dest=0x7f6576ada560) at /usr/include/x86_64-linux-gnu/bits/string3.h:90
>#1  0x000000000041b80e in arena_dalloc (mapelm=<optimized out>, ptr=0x7f6576ada560, chunk=0x7f6576a00000, arena=0x7f659d700040) at gecko/memory/mozjemalloc/jemalloc.c:4572
>#2  0x000000000041b80e in arena_dalloc (ptr=ptr@entry=0x7f6576ada560, offset=offset@entry=894304) at gecko/memory/mozjemalloc/jemalloc.c:4698
>#3  0x000000000041dda1 in je_free (ptr=0x7f6576ada560) at gecko/memory/mozjemalloc/jemalloc.c:6374
>#4  0x00007f658e982150 in mozilla::AbstractThread::Release() (this=0x7f6576ada560) at gecko/obj-firefox/dist/include/mozilla/AbstractThread.h:68
>#5  0x00007f658ee07d56 in mozilla::ipc::MessageChannel::~MessageChannel() (aPtr=<optimized out>) at gecko/obj-firefox/dist/include/mozilla/RefPtr.h:395
>#6  0x00007f658ee07d56 in mozilla::ipc::MessageChannel::~MessageChannel() (this=0x7f6575160160, __in_chrg=<optimized out>) at gecko/obj-firefox/dist/include/mozilla/RefPtr.h:78
>#7  0x00007f658ee07d56 in mozilla::ipc::MessageChannel::~MessageChannel() (this=0x7f6575160120, __in_chrg=<optimized out>) at gecko/ipc/glue/MessageChannel.cpp:563
>#8  0x00007f658f07ed62 in mozilla::ipc::PBackgroundParent::~PBackgroundParent() (this=0x7f6575160000, __in_chrg=<optimized out>) at gecko/obj-firefox/ipc/ipdl/PBackgroundParent.cpp:212
>#9  0x00007f658edf58a8 in (anonymous namespace)::ParentImpl::~ParentImpl() (this=0x7f6575160000, __in_chrg=<optimized out>) at gecko/ipc/glue/BackgroundImpl.cpp:256
>#10 0x00007f658edf5940 in (anonymous namespace)::ParentImpl::Release() (this=0x7f6575160000) at gecko/ipc/glue/BackgroundImpl.cpp:200
>#11 0x00007f658edeb449 in mozilla::detail::RunnableMethodImpl<(anonymous namespace)::ParentImpl*, void ((anonymous namespace)::ParentImpl::*)(), false, false>::Run() (args=..., m=<optimized out>, o=<optimized out>) at gecko/xpcom/threads/nsThreadUtils.h:874
>#12 0x00007f658edeb449 in mozilla::detail::RunnableMethodImpl<(anonymous namespace)::ParentImpl*, void ((anonymous namespace)::ParentImpl::*)(), false, false>::Run() (this=<optimized out>, m=<optimized out>, o=<optimized out>) at gecko/xpcom/threads/nsThreadUtils.h:881
>#13 0x00007f658edeb449 in mozilla::detail::RunnableMethodImpl<(anonymous namespace)::ParentImpl*, void ((anonymous namespace)::ParentImpl::*)(), false, false>::Run() (this=<optimized out>) at gecko/xpcom/threads/nsThreadUtils.h:909
>#14 0x00007f658e98fd8a in nsThread::ProcessNextEvent(bool, bool*) (this=0x7f658c273e80, aMayWait=<optimized out>, aResult=0x7ffdf72317b7) at gecko/xpcom/threads/nsThread.cpp:1270
>#15 0x00007f658e991bd2 in NS_ProcessNextEvent(nsIThread*, bool) (aThread=aThread@entry=0x7f658c273e80, aMayWait=aMayWait@entry=true) at gecko/xpcom/threads/nsThreadUtils.cpp:393
>#16 0x00007f658ee01e61 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (this=0x7f658c2ff880, aDelegate=0x7f659d670f00) at gecko/ipc/glue/MessagePump.cpp:124
>#17 0x00007f658edaea2c in MessageLoop::RunInternal() (this=0x7f659d670f00) at gecko/ipc/chromium/src/base/message_loop.cc:238
>#18 0x00007f658edaea80 in MessageLoop::Run() (this=<optimized out>) at gecko/ipc/chromium/src/base/message_loop.cc:231
>#19 0x00007f658edaea80 in MessageLoop::Run() (this=<optimized out>) at gecko/ipc/chromium/src/base/message_loop.cc:211
>#20 0x00007f659079b8b9 in nsBaseAppShell::Run() (this=0x7f6585e976d0) at gecko/widget/nsBaseAppShell.cpp:156
>#21 0x00007f65916da5bb in nsAppStartup::Run() (this=0x7f6587e59a60) at gecko/toolkit/components/startup/nsAppStartup.cpp:283
>#22 0x00007f659176194d in XREMain::XRE_mainRun() (this=this@entry=0x7ffdf7231b78) at gecko/toolkit/xre/nsAppRunner.cpp:4546
>#23 0x00007f6591762179 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) (this=this@entry=0x7ffdf7231b78, argc=argc@entry=5, argv=argv@entry=0x7ffdf7232eb8, aConfig=...) at gecko/toolkit/xre/nsAppRunner.cpp:4726
>#24 0x00007f6591762437 in XRE_main(int, char**, mozilla::BootstrapConfig const&) (argc=5, argv=0x7ffdf7232eb8, aConfig=...) at gecko/toolkit/xre/nsAppRunner.cpp:4819
>#25 0x00000000004062cf in do_main(int, char**, char**) (argc=argc@entry=5, argv=argv@entry=0x7ffdf7232eb8, envp=envp@entry=0x7ffdf7232ee8) at gecko/browser/app/nsBrowserApp.cpp:236
>#26 0x0000000000405d05 in main(int, char**, char**) (argc=5, argv=0x7ffdf7232eb8, envp=0x7ffdf7232ee8) at gecko/browser/app/nsBrowserApp.cpp:307
(Assignee)

Comment 2

a year ago
It's because on the background thread many PBackground top-level protocol can be created and destroyed. We need to make sure the life time of the AbstractThread wrapper is as long as the thread instead of the MessageChannel. This is not a security issue currently because this function hasn't been used in this way. I think SC is the first one tried to do this.

In turns of solutions, it seems using thread_local with thread-storage destructor is the most elegant but our mfbt/ThreadLocal.h doesn't support that. Many other places use PR_SetThreadPrivate with a custom index table destructor but I don't want to use NSPR here. Fortunately MessageLoop defines a DestructionObserver so we can use that to manage the wrappers.
(Assignee)

Comment 3

a year ago
Created attachment 8865325 [details] [diff] [review]
patch

SC, could you try this patch?
Flags: needinfo?(schien)
looks ok on my local environment, submit a try for your reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5a95703985fbb0784bcb5f7731ed5e3c9583868
Flags: needinfo?(schien)
(Assignee)

Updated

a year ago
Attachment #8865325 - Flags: review?(wmccloskey)
Attachment #8865325 - Flags: review?(wmccloskey) → review+

Comment 5

a year ago
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/690fcdc70b82
Only delete AbstractThread wrapper when MessageLoop is about to destroy. r=billm
backed out for build bustage at MessageChannel.cpp: bad implicit conversion constructor for 'AbstractThreadWrapperCleanup' like https://treeherder.mozilla.org/logviewer.html#?job_id=98247365&repo=mozilla-inbound&lineNumber=10001
Flags: needinfo?(kchen)

Comment 7

a year ago
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7192279cfc4a
Backed out changeset 690fcdc70b82 for build bustage at MessageChannel.cpp: bad implicit conversion constructor for 'AbstractThreadWrapperCleanup'

Comment 8

a year ago
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c7773fcef0
Only delete AbstractThread wrapper when MessageLoop is about to destroy. r=billm
(Assignee)

Updated

a year ago
Flags: needinfo?(kchen)
https://hg.mozilla.org/mozilla-central/rev/b3c7773fcef0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
No longer blocks: 1015466
You need to log in before you can comment on or make changes to this bug.