793759 - Implement HalChild::ActorDestroy and prevent HalChild calls to HalParent after ActorDestroy is called

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[StevenLee[:slee]]

As the comment in https://bugzilla.mozilla.org/show_bug.cgi?id=790499#c24, 
we need such mechanism.

Comment 1

[StevenLee[:slee]]

Attachment: patch

Hi Chris,

When the connection between content process and chrome process is broken. Content process
should not call through ipc else the process will crash. I implemented ActorDestroy in
HalChild to set a flag and modified PROXY_IF_SANDBOXED and RETURN_PROXY_IF_SANDBOXED to 
prevent content process calls to ipc.

Can you help me review this patch or suggest someone to review it?

Thanks.

Comment 2

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

The content process will _exit(0) if that happens.  Why is that a problem?

Note, this is the case for all other IPC from content processes after the parent shuts down.

Comment 3

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> The content process will _exit(0) if that happens.  Why is that a problem?
> 
> Note, this is the case for all other IPC from content processes after the
> parent shuts down.
I ran the test case and found it crashed. Not sure which test case cause the problem. I apply
the patch of Bug 790499, set test path to dom/browser-element/mochitest and run the test 
cases.

Here is the brief of the patch.
I have a object whose destructor is called by KillClearOnShutdown. What the destructor does
is to call the unregister function in mozilla::hal. Because it is in the content process,
it goes to mozilla::hal_sandbox. At this moment, the ipc resource of content process is 
freed then the process is crashed.

Comment 4

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

(In reply to StevenLee from comment #3)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Here is the brief of the patch.
> I have a object whose destructor is called by KillClearOnShutdown. What the
> destructor does
> is to call the unregister function in mozilla::hal. Because it is in the
> content process,
> it goes to mozilla::hal_sandbox. At this moment, the ipc resource of content
> process is 
> freed then the process is crashed.

I see.  Does the parent process automatically unregister your listener when the child process crashes?  [1]

Is your listener an XPCOM object or a plain C++ object?

[1] http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#380

Comment 5

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

Comment on attachment 664383 [details] [diff] [review]
patch

Clearing request pending answers to questions in comment 4.  I'm not sure if this solves the problem you want it to.

Comment 6

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> Comment on attachment 664383 [details] [diff] [review]
> patch
> 
> Clearing request pending answers to questions in comment 4.  I'm not sure if
> this solves the problem you want it to.

I tried it on my desktop and it worked fine. I will push to try server and see if the crash problem is fixed.
Many thanks for your comments.

Comment 7

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

The questions I'm asking are

  1. Does the parent process automatically unregister your listener when the child process crashes?  [1]

  2. Is your listener an XPCOM object or a plain C++ object?

[1] http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#380

Comment 8

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #7)
> The questions I'm asking are
> 
>   1. Does the parent process automatically unregister your listener when the
> child process crashes?  [1]
> 
>   2. Is your listener an XPCOM object or a plain C++ object?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/hal/sandbox/SandboxHal.cpp#380

1. No. If I unregister the listener in HalParent::ActorDestroy, there is no crash in the test cases.
2. It's a plan C++ object.

Comment 9

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

My recommendation is to not attempt to unregister the observer, but make sure that subprocess observers are cleaned up in HalParent::ActorDestroy().

Comment 10

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> My recommendation is to not attempt to unregister the observer, but make
> sure that subprocess observers are cleaned up in HalParent::ActorDestroy().

I am sorry that I am confused.
How could the subprocess observers be remove in HalParent::ActorDestroy without not calling unregister functions?

I reproduced the problem and traced again. I got the flow that makes the problem.
Content process invokes ActorDestroy(NormalShutdown) in uiMessageLoop.MessageLoop::Run()[1]. But it does not call _exit(0) because this code exists only in release build. [2] Then content process calls ProcessChild::cleanUp [3]. In this function, it calls KillClearOnShutdown that eventually goes to hal_sandbox, uses the freed PHalChild then content process crashes. 

[1]http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#485
[2]http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#761
[3]http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#489

Comment 11

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

(In reply to StevenLee from comment #10)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> > My recommendation is to not attempt to unregister the observer, but make
> > sure that subprocess observers are cleaned up in HalParent::ActorDestroy().
> 
> I am sorry that I am confused.
> How could the subprocess observers be remove in HalParent::ActorDestroy
> without not calling unregister functions?
> 

They can.  You should unregister the parent-side observer from HalParent::ActorDestroy().

However, you should not try to clear the *content process* observer on shutdown ... *in the content process*.  Let it leak.

Comment 12

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> They can.  You should unregister the parent-side observer from
> HalParent::ActorDestroy().
> 
> However, you should not try to clear the *content process* observer on
> shutdown ... *in the content process*.  Let it leak.
I am sorry for asking dumb questions. Why cannot we add the flag and set this flag when HalChild::ActorDestroy is called? If we have such flag, we can prevent content process sending messages to parent process.
I unregister the observers from HalParent::ActorDestroy and push the patch to try server. Crash problem still exists. :( If I should not add the flag, I will discuss with Justin for other approach. Thanks.

Comment 13

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

(In reply to StevenLee from comment #12)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #11)
> > They can.  You should unregister the parent-side observer from
> > HalParent::ActorDestroy().
> > 
> > However, you should not try to clear the *content process* observer on
> > shutdown ... *in the content process*.  Let it leak.
> I am sorry for asking dumb questions. Why cannot we add the flag and set
> this flag when HalChild::ActorDestroy is called? If we have such flag, we
> can prevent content process sending messages to parent process.

There's no benefit to do doing that :).  No need to add code that doesn't do anything useful.

> I unregister the observers from HalParent::ActorDestroy and push the patch
> to try server. Crash problem still exists. :( If I should not add the flag,
> I will discuss with Justin for other approach. Thanks.

Did the patch also stop trying to unregister the observers on the content process side?

Comment 14

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> There's no benefit to do doing that :).  No need to add code that doesn't do
> anything useful.
OK. 
 
> Did the patch also stop trying to unregister the observers on the content
> process side?
No, the patch calls the unregister function. I just want to prevent memory leak as possible as I can. I will discuss with Justin about this and see if I need to change my method. Thanks for your help :).

Comment 15

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

(In reply to StevenLee from comment #14)
> > Did the patch also stop trying to unregister the observers on the content
> > process side?
> No, the patch calls the unregister function. I just want to prevent memory
> leak as possible as I can. I will discuss with Justin about this and see if
> I need to change my method. Thanks for your help :).

I looked at your patch and it seemed like we only have one observer for the entire lifetime of the process.  In that case, there's no leak; we hold onto the pointer while the process is alive.  We just let the OS clean it up for us.

If we instead registered an observer on every mouse click, for example, and then unregistered it soon afterward, it would be important to check for leaks.  We could have millions of those objects created while the process is alive, and then leaking them would eventually result in out-of-memory.

Comment 16

[StevenLee[:slee]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15)
> I looked at your patch and it seemed like we only have one observer for the
> entire lifetime of the process.  In that case, there's no leak; we hold onto
> the pointer while the process is alive.  We just let the OS clean it up for
> us.
Hi Chris, 

Thanks for your explanation. I know that there will be only one pointer and it will be cleaned by the OS. But if I don't unregister it, the log of debug build on try server will be full of memory leak errors, https://tbpl.mozilla.org/?tree=Try&rev=f2b896453307, :(.

Comment 17

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

Oh, the leak that's being reported is for nsTArrays, not your object specifically.  I didn't know that.  So it's the observer list that's causing a leak to be reported.

OK, your patch is fine.

Comment 18

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

Comment on attachment 664383 [details] [diff] [review]
patch

>diff --git a/hal/HalInternal.h b/hal/HalInternal.h

>+bool IsChildActorDestroy();

Call this |IsHalChildLive()| and add a comment describing what it
means.

>diff --git a/hal/sandbox/SandboxHal.cpp b/hal/sandbox/SandboxHal.cpp

>+static bool sChildActorDestroy = false;

Call this |sHalChildIsLive|.  Set it to true in the HalChild
constructor, and set it to false in ActorDestroy().

r=me with those changes.

Comment 19

[StevenLee[:slee]]

Attachment: patch v2

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/834fb95232af

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/834fb95232af