Shut the the gfx ipdl protocols down properly for child processes on Windows

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nical, Assigned: bas.schouten)

Tracking

(Blocks 1 bug)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox48 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

Reporter

Description

3 years ago
In bug 1215265 we improved the shutdown of gfx protocols in child processes, but it landed disabled on Windows.

It can be enabled using the pref "layers.child-process-shutdown".
On my last try push something was leaking a CompositorThreadHolder which causes the shutdown of the parent process to timeout. Let's figure the missing pieces on Windows and remove the pref.
Reporter

Updated

3 years ago
Blocks: 1264293
Reporter

Updated

3 years ago
Blocks: 1245574
Reporter

Updated

3 years ago
Blocks: 1232549
Whiteboard: [gfx-noted]
Reporter

Updated

3 years ago
Blocks: 1264694
Assignee

Updated

3 years ago
Blocks: 1252677
Assignee

Updated

3 years ago
Assignee: nobody → bas
Assignee

Comment 1

3 years ago
So the problem here is that ContentParent::ActorDestroy will post deletion of the GeckoChildProcessHost. However ImageBridgeParent or CompositorBridgeParent may still need to shutdown. Since GeckoChildProcessHost deletion will block the IO thread until the child process has died those shutdown messages will never be able to reach the parent. The GeckoChildProcessHost needs to stay alive until both the ImageBridgeParent and CompositorBridgeParent ActorDestroy functions have been called.
Reporter

Comment 3

3 years ago
Comment on attachment 8743752 [details]
MozReview Request: Bug 1262898: Keep the GeckoChildProcessHost alive for the lifetime of the CompositorBridge and ImageBridge parent actors. r=jimm r=nical

https://reviewboard.mozilla.org/r/48011/#review44805
Attachment #8743752 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8743752 [details]
MozReview Request: Bug 1262898: Keep the GeckoChildProcessHost alive for the lifetime of the CompositorBridge and ImageBridge parent actors. r=jimm r=nical

https://reviewboard.mozilla.org/r/48011/#review45389

Looks ok, I was looking for ways this might break all the odd shutdown patsh we have. AFAICt it just delays the destruction of the GeckoChildProcessHost object, and that seems safe. Didn't see any issues running this in the debugger either.

::: gfx/layers/ipc/CompositorBridgeParent.cpp:2263
(Diff revision 1)
>  CrossProcessCompositorBridgeParent::ActorDestroy(ActorDestroyReason aWhy)
>  {
>    RefPtr<CompositorLRU> lru = CompositorLRU::GetSingleton();
>    lru->Remove(this);
> +
> +  if (mSubprocess) {

Lets add a comment here explaining what this does - it sets this gecko child process host up for async deletion once additiona actors (mostly gfx) clear their associations.
Attachment #8743752 - Flags: review?(jmathies) → review+

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c3b5f9bd44a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Hey ryan, this is supposed to help with a bunch of e10s related test failures, some of which are in the blocking list.
Flags: needinfo?(ryanvm)
Yeah, it's running on Ash right now and looking very promising! The Windows M(5) permaleaks are gone :)
Flags: needinfo?(ryanvm)
Depends on: 1268332

Updated

3 years ago
tracking-e10s: --- → +
Priority: -- → P1

Updated

3 years ago
Depends on: 1276650
You need to log in before you can comment on or make changes to this bug.