Child processes that share X surfaces with parents need to send the parent a dup of their Display socket

RESOLVED FIXED

Status

()

Core
Graphics
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

unspecified
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

Attachments

(4 attachments)

With cross-process layers in plugins and content processes, the child side of the layers connection gives front surfaces to its parent.  If the child dies unexpectedly, the X server will destroy the front surface and leave the parent with a "dangling reference".  If the parent tries to draw it, the parent will die from an X error.

This has the potential to undo the stability gains we win from multiple processes.  To prevent this, we should have the child share its Display socket fd with the parent.  The X server will only destroy the child's resources when the socket closes, so the parent can keep the socket open until it detects the child's death and lets go of surfaces the child has shared with it.  (In theory!)

We can test this very easily with a OOPP mochitest that uses async plugin painting.  The effectiveness of this technique may vary from server to server, but we won't know until we try.
blocking2.0: --- → ?
blocking2.0: ? → final+
Assignee: nobody → jones.chris.g

Comment 1

7 years ago
FWIW, I want to flip it in just a bit so that the parent creates the surfaces, not the child. But I'm happy if there's a quick fix in the meantime.
That's not possible with the generic cross-process layers.
Uh, is this even valid to consider with X?  I don't see how it can possibly share a socket with two processes...

(or do you just mean give the parent the socket to hold on to, but not actually do any communication over it?)
Uh yeah, multiplexing over the same socket would be insane.  The latter.

The plan is worded a bit better at https://wiki.mozilla.org/Gecko:CrossProcessLayers#X11.
Summary: Child processes that share X surfaces with parents need to also share their Display socket → Child processes that share X surfaces with parents need to send the parent a dup of their Display socket

Comment 5

7 years ago
Note, I implement async painting a different way in comment 1, so this will need to be fixed your way.
Created attachment 486007 [details] [diff] [review]
part 1: Add mozilla::ScopedClose to do just that to an fd
Attachment #486007 - Flags: superreview?(benjamin)
Created attachment 486008 [details] [diff] [review]
part 2: Add a dummy base::FileDescriptor

This allows us to use base::FileDescriptor in IPDL.
Attachment #486008 - Flags: review?(benjamin)
Created attachment 486009 [details] [diff] [review]
part 3: Have plugin parents keep a 'proxy ref' to plugin X resources by duping the plugin's X socket
Attachment #486009 - Flags: review?(karlt)
Created attachment 486010 [details] [diff] [review]
Test
Attachment #486010 - Flags: review?(karlt)
Comment on attachment 486009 [details] [diff] [review]
part 3: Have plugin parents keep a 'proxy ref' to plugin X resources by duping the plugin's X socket

>+    SendBackUpXResources(FileDescriptor(xSocketFd, false/*don't close*/));

bool parameters ftw.

Looking at
http://hg.mozilla.org/mozilla-central/annotate/16c54dba9f6b/ipc/chromium/src/chrome/common/ipc_message.cc#l136

it seems (the first part of) this comment is wrong:
http://hg.mozilla.org/mozilla-central/annotate/16c54dba9f6b/ipc/chromium/src/chrome/common/ipc_message_utils.h#l754

and so this patch is good.
Attachment #486009 - Flags: review?(karlt) → review+
Comment on attachment 486010 [details] [diff] [review]
Test

I'm not sure how much the XSync here is achieving.  It sends remaining browser requests to the server and waits for all the responses and browser events.
I doubt this necessarily means that the server has checked all it's client connections, but at least it checks that the server is awake.

MozAfterPaint is sent after invalidation.  To be sure of a paint it would be better to wait and check mozPaintCount, but I don't know whether it's that critical.

I wondered whether using gdk_display_close(gdk_display_get_default()) might allow the plugin to live long enough to receive the cleanup crash, saving one crash, but that's not guaranteed and it doesn't really matter.
Attachment #486010 - Flags: review?(karlt) → review+
Comment on attachment 486010 [details] [diff] [review]
Test

Should probably skip this test if !testPluginIsOOP.
Oh neat, I didn't know we had that.  Yes, definitely.

Comment 14

7 years ago
Comment on attachment 486007 [details] [diff] [review]
part 1: Add mozilla::ScopedClose to do just that to an fd

I don't think this should be #ifdef XP_UNIX, r=me with the ifdef removed.

I'd kinda like "operator int" on this, just as we have operator PRFileDesc* on AutoFDClose, but I guess if you're not using it that can wait until there's a consumer.
Attachment #486007 - Flags: superreview?(benjamin) → superreview+

Updated

7 years ago
Attachment #486008 - Flags: review?(benjamin) → review+
(In reply to comment #14)
> Comment on attachment 486007 [details] [diff] [review]
> part 1: Add mozilla::ScopedClose to do just that to an fd
> 
> I don't think this should be #ifdef XP_UNIX, r=me with the ifdef removed.
> 

Done.

> I'd kinda like "operator int" on this, just as we have operator PRFileDesc* on
> AutoFDClose, but I guess if you're not using it that can wait until there's a
> consumer.

Will wait.

http://hg.mozilla.org/mozilla-central/rev/ed1d65957bd0
http://hg.mozilla.org/mozilla-central/rev/ed4fabb94ad8
http://hg.mozilla.org/mozilla-central/rev/f45d69fd9aaa
http://hg.mozilla.org/mozilla-central/rev/1c7a0925aa46
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 610646

Updated

7 years ago
Depends on: 611033
No longer depends on: 611033

Updated

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