Closed Bug 1102439 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: schien, Assigned: schien)

References

Details

(Whiteboard: [PBg-HTTP-M2])

Attachments

(3 obsolete files)

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
1. Automatically close BackgroundChild for each nsThread.
2. For main thread we'll need to close BackgroundChild while ContentChild is destroyed.
Attachment #8526234 - Flags: review?(khuey)
Attachment #8526234 - Flags: feedback?(bent.mozilla)
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
Attachment #8526234 - Flags: feedback?(bent.mozilla) → feedback+
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.
/r/913 - Bug 1102439 - clean up child-side PBackground before thread shutdown. r=khuey.

Pull down this commit:

hg pull review -r 29727d0c509fd6208ba9eaee7dacbb801a42d836
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().
Attachment #8527020 - Flags: feedback?(bent.mozilla)
Attachment #8526234 - Attachment is obsolete: true
Attachment #8526234 - Flags: review?(khuey)
Attachment #8527020 - Flags: feedback?(bent.mozilla) → feedback+
Comment on attachment 8527020 [details]
MozReview Request: bz://1102439/schien

r=me
Attachment #8527020 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/a011ca7ed14c
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8527020 - Attachment is obsolete: true
Attachment #8618663 - Flags: review+
Whiteboard: [PBg-HTTP-M2]
Attachment #8618663 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.