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

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

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.

Comment 1

[Benjamin Smedberg]

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.

Comment 2

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

That's not possible with the generic cross-process layers.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?)

Comment 4

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

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.

Comment 5

[Benjamin Smedberg]

Note, I implement async painting a different way in comment 1, so this will need to be fixed your way.

Comment 6

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

Attachment: part 1: Add mozilla::ScopedClose to do just that to an fd

Comment 7

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

Attachment: part 2: Add a dummy base::FileDescriptor

This allows us to use base::FileDescriptor in IPDL.

Comment 8

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

Attachment: part 3: Have plugin parents keep a 'proxy ref' to plugin X resources by duping the plugin's X socket

Comment 9

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

Attachment: Test

Comment 10

[Karl Tomlinson (: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.

Comment 11

[Karl Tomlinson (:karlt)]

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.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 486010 [details] [diff] [review]
Test

Should probably skip this test if !testPluginIsOOP.

Comment 13

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

Oh neat, I didn't know we had that.  Yes, definitely.

Comment 14

[Benjamin Smedberg]

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.

Comment 15

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

(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