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

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: slee, Assigned: slee)

Tracking

unspecified
mozilla18
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

As the comment in https://bugzilla.mozilla.org/show_bug.cgi?id=790499#c24, 
we need such mechanism.
Blocks: 790499
Posted patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → slee
Status: NEW → ASSIGNED
Attachment #664383 - Flags: review?(jones.chris.g)
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.
(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.
(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 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.
Attachment #664383 - Flags: review?(jones.chris.g)
(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.
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
(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.
My recommendation is to not attempt to unregister the observer, but make sure that subprocess observers are cleaned up in HalParent::ActorDestroy().
(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
(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.
(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.
(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?
(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 :).
(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.
(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, :(.
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 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.
Attachment #664383 - Flags: review+
Posted patch patch v2Splinter Review
Attachment #664383 - Attachment is obsolete: true
Attachment #668368 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/834fb95232af
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 808983
You need to log in before you can comment on or make changes to this bug.