Closed Bug 1262898 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Graphics: Layers, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox48 --- fixed

People

(Reporter: nical, Assigned: bas.schouten)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
Blocks: 1264293
Blocks: 1245574
Blocks: 1232549
Whiteboard: [gfx-noted]
Blocks: 1264694
Blocks: 1252677
Assignee: nobody → bas
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.
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+
https://hg.mozilla.org/mozilla-central/rev/4c3b5f9bd44a
Status: NEW → RESOLVED
Closed: 5 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
tracking-e10s: --- → +
Priority: -- → P1
Depends on: 1276650
You need to log in before you can comment on or make changes to this bug.