1102439 - [PBackground] should close child-side PBackground before thread shutdown

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

MessageLoop might be closed before we clean up child-side PBackground.

[crash stacktrace in debug build]
#0  0x00007fffee7d79bd in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007fffee7d7854 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007ffff32693b7 in ah_crap_handler (signum=11) at /home/hellfire/workspace/hg/mozilla-central/toolkit/xre/nsSigHandlers.cpp:101
#3  0x00007ffff3269402 in child_ah_crap_handler (signum=11) at /home/hellfire/workspace/hg/mozilla-central/toolkit/xre/nsSigHandlers.cpp:113
#4  0x00007ffff403cdd4 in AsmJSFaultHandler (signum=11, info=0x7fffe0afe630, context=0x7fffe0afe500) at /home/hellfire/workspace/hg/mozilla-central/js/src/asmjs/AsmJSSignalHandlers.cpp:982
#5  <signal handler called>
#6  0x00007ffff01fcb30 in MessageLoop::id (this=0x0) at /home/hellfire/workspace/hg/mozilla-central/ipc/chromium/src/base/message_loop.h:233
#7  0x00007ffff01fd081 in mozilla::ipc::MessageChannel::AssertWorkerThread (this=0x7fffe0dde460) at ../../dist/include/mozilla/ipc/MessageChannel.h:374
#8  0x00007ffff01f6a09 in mozilla::ipc::MessageChannel::Close (this=0x7fffe0dde460) at /home/hellfire/workspace/hg/mozilla-central/ipc/glue/MessageChannel.cpp:1633
#9  0x00007ffff02309fa in mozilla::ipc::PBackgroundChild::Close (this=0x7fffe0dde400) at /home/hellfire/workspace/hg/mozilla-central/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:106
#10 0x00007ffff01e9122 in (anonymous namespace)::ChildImpl::ThreadLocalDestructor (aThreadLocal=0x7fffe0d84a40) at /home/hellfire/workspace/hg/mozilla-central/ipc/glue/BackgroundImpl.cpp:435
#11 0x00007ffff7983e32 in _PR_DestroyThreadPrivate (self=0x7fffe5e5f420) at /home/hellfire/workspace/hg/mozilla-central/nsprpub/pr/src/threads/prtpd.c:237
#12 0x00007ffff79a1db2 in _pt_root (arg=0x7fffe5e5f420) at /home/hellfire/workspace/hg/mozilla-central/nsprpub/pr/src/pthreads/ptthread.c:252
#13 0x00007ffff7bc4182 in start_thread (arg=0x7fffe0aff700) at pthread_create.c:312
#14 0x00007fffee810fbd in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: clean-up-background-child-earlier.patch

1. Automatically close BackgroundChild for each nsThread.
2. For main thread we'll need to close BackgroundChild while ContentChild is destroyed.

Comment 2

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

Comment on attachment 8526234 [details] [diff] [review]
clean-up-background-child-earlier.patch

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

::: dom/ipc/ContentChild.cpp
@@ +1778,5 @@
>          mConsoleListener->mChild = nullptr;
>      }
>      mIsAlive = false;
>  
> +    BackgroundChild::CloseForCurrentThread();

This shouldn't be needed, the main thread is already handled specifically (see ChildImpl::ShutdownObserver in BackgroundImpl.cpp)

::: ipc/glue/BackgroundImpl.cpp
@@ +345,5 @@
>  
>      nsRefPtr<ChildImpl> mActor;
>      nsTArray<nsCOMPtr<nsIIPCBackgroundChildCreateCallback>> mCallbacks;
>      nsAutoPtr<BackgroundChildImpl::ThreadLocal> mConsumerThreadLocal;
> +    bool mClosed;

Let's make this DebugOnly<bool>

@@ +1773,5 @@
>    if (!threadLocalInfo) {
> +    return;
> +  }
> +
> +  if (threadLocalInfo) {

Your early return above means you don't need to check this again

::: xpcom/threads/nsThread.cpp
@@ +71,5 @@
>  using namespace mozilla::tasktracer;
>  #endif
>  
>  using namespace mozilla;
> +using mozilla::ipc::BackgroundChild;

You could move this inside ThreadFunc to scope it more narrowly

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8526234 [details] [diff] [review]
clean-up-background-child-earlier.patch

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

::: dom/ipc/ContentChild.cpp
@@ +1778,5 @@
>          mConsoleListener->mChild = nullptr;
>      }
>      mIsAlive = false;
>  
> +    BackgroundChild::CloseForCurrentThread();

I hit the MOZ_ASSERT(threadInfoLocal->mClosed) before I add this line of code. I don't know why but the timing of |ChildImpl::ShutdownObserver| is still too late.

::: ipc/glue/BackgroundImpl.cpp
@@ +345,5 @@
>  
>      nsRefPtr<ChildImpl> mActor;
>      nsTArray<nsCOMPtr<nsIIPCBackgroundChildCreateCallback>> mCallbacks;
>      nsAutoPtr<BackgroundChildImpl::ThreadLocal> mConsumerThreadLocal;
> +    bool mClosed;

Done.

@@ +1773,5 @@
>    if (!threadLocalInfo) {
> +    return;
> +  }
> +
> +  if (threadLocalInfo) {

Done.

::: xpcom/threads/nsThread.cpp
@@ +71,5 @@
>  using namespace mozilla::tasktracer;
>  #endif
>  
>  using namespace mozilla;
> +using mozilla::ipc::BackgroundChild;

Done.

Comment 4

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: MozReview Request: bz://1102439/schien

Comment 5

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

/r/913 - Bug 1102439 - clean up child-side PBackground before thread shutdown. r=khuey.

Pull down this commit:

hg pull review -r 29727d0c509fd6208ba9eaee7dacbb801a42d836

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8527020 [details]
MozReview Request: bz://1102439/schien

We should mark the threadInfoLocal for main thread as "closed" in ChildImpl::Shutdown() instead of ContentChild::ActorDestroy().

Comment 7

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

https://tbpl.mozilla.org/?tree=Try&rev=29727d0c509f

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8527020 [details]
MozReview Request: bz://1102439/schien

r=me

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a011ca7ed14c

Comment 10

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/mozilla-central/rev/a011ca7ed14c

Comment 12

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Attachment: MozReview Request: Bug 1102439 - clean up child-side PBackground before thread shutdown. r=khuey.