Mac: Ensure that Mach messages always get a reply

Mac: Ensure that Mach messages always get a reply

It may be that not getting replies from Mach messages was causing the
renderer to hang (appearing as a persistent blank white screen).

In particular, we send IOSurface Register and Acquire functions with the
flags MACH_SEND_MSG | MACH_RCV_MSG, but, if the browser doesn't
understand the request, it never sends the message back to the renderer,
so the receive part hangs forever.

Add a flag containing the result of the browser's operation, and, if
the renderer would not have gotten a message back (and would have hung),
just crash the renderer.

BUG=532149
Committed: https://crrev.com/7c766ff43c0b84fb97e2a58c3be183c3d1339f23
Cr-Commit-Position: refs/heads/master@{#349743}

[ccameron, 2015-09-16 23:47:44]

ccameron@chromium.org changed reviewers:
+ avi@chromium.org, reveman@chromium.org

[ccameron, 2015-09-16 23:51:28]

For now I have a CHECK (because the previous behavior was hanging the thread &
leaving the renderer dead).

In parallel, I'm looking to see why we may be hitting these races.

[reveman, 2015-09-17 03:58:22]

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:241: return;
don't we have to set some of the reply fields in this case?

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:288: return;
Same here. Don't we have some set some reply fields when no longer returning
false?

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.h:87: bool* result);
can we keep this as a return value instead?

[Avi (use Gerrit), 2015-09-17 15:12:31]

I'll leave David as an area expert, but I'm OK with this.

LGTM, tho you'll need IPC approval too.

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:207: NULL);
nullptr

[ccameron, 2015-09-17 18:35:05]

Thanks -- updated.

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:207: NULL);
On 2015/09/17 15:12:30, Avi wrote:
> nullptr

Done.

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:241: return;
On 2015/09/17 03:58:22, reveman wrote:
> don't we have to set some of the reply fields in this case?

This gets called from HandleRequest, which sets the whole structure to 0
(ensuring that reply->result will be false).

That said, it's probably more clear to set reply->result here. Updated.

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:288: return;
On 2015/09/17 03:58:22, reveman wrote:
> Same here. Don't we have some set some reply fields when no longer returning
> false?

Done.

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.h (right):

https://codereview.chromium.org/1351753002/diff/20001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.h:87: bool* result);
On 2015/09/17 03:58:22, reveman wrote:
> can we keep this as a return value instead?

Done.

[reveman, 2015-09-17 18:58:17]

https://codereview.chromium.org/1351753002/diff/40001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/40001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:240: reply->result = false;
You'll need to set reply->header.msgh_bits/msgh_remote_port/msgh_size here as
well to make the reply message valid. Maybe just move the reply->header lines
below to above this..

https://codereview.chromium.org/1351753002/diff/40001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:284: reply->result = false;
Same here.

[ccameron, 2015-09-18 18:06:13]

Thanks -- updated!

https://codereview.chromium.org/1351753002/diff/40001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/40001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:240: reply->result = false;
On 2015/09/17 18:58:16, reveman wrote:
> You'll need to set reply->header.msgh_bits/msgh_remote_port/msgh_size here as
> well to make the reply message valid. Maybe just move the reply->header lines
> below to above this..

Done.

https://codereview.chromium.org/1351753002/diff/40001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:284: reply->result = false;
On 2015/09/17 18:58:16, reveman wrote:
> Same here.

Done.

[reveman, 2015-09-18 18:22:31]

lgtm with nit and minor request

https://codereview.chromium.org/1351753002/diff/60001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/60001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:282: reply->result = false;
Request: This is not new to this patch but can you change "reply->result =
false;" to the following instead of depending on the caller initializing |reply|
to 0:

reply->body.msgh_descriptor_count = 0;
reply->result = false;
reply->io_surface_port.name = MACH_PORT_NULL;
reply->io_surface_port.disposition = 0;
reply->io_surface_port.type = 0;

https://codereview.chromium.org/1351753002/diff/60001/content/child/child_io_...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/60001/content/child/child_io_...
content/child/child_io_surface_manager_mac.cc:136: if (!data.reply.msg.result) {
nit: this if-statement seems unnecessary with the check above

[ccameron, 2015-09-18 18:36:01]

Thanks!

https://codereview.chromium.org/1351753002/diff/60001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/60001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:282: reply->result = false;
On 2015/09/18 18:22:30, reveman wrote:
> Request: This is not new to this patch but can you change "reply->result =
> false;" to the following instead of depending on the caller initializing
|reply|
> to 0:
> 
> reply->body.msgh_descriptor_count = 0;
> reply->result = false;
> reply->io_surface_port.name = MACH_PORT_NULL;
> reply->io_surface_port.disposition = 0;
> reply->io_surface_port.type = 0;

Done. Also changed Register reply to set body.* to 0 as well.

https://codereview.chromium.org/1351753002/diff/60001/content/child/child_io_...
File content/child/child_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/60001/content/child/child_io_...
content/child/child_io_surface_manager_mac.cc:136: if (!data.reply.msg.result) {
On 2015/09/18 18:22:30, reveman wrote:
> nit: this if-statement seems unnecessary with the check above

Removed for now.

[reveman, 2015-09-18 18:47:49]

https://codereview.chromium.org/1351753002/diff/60001/content/browser/browser...
File content/browser/browser_io_surface_manager_mac.cc (right):

https://codereview.chromium.org/1351753002/diff/60001/content/browser/browser...
content/browser/browser_io_surface_manager_mac.cc:282: reply->result = false;
On 2015/09/18 at 18:36:00, ccameron wrote:
> On 2015/09/18 18:22:30, reveman wrote:
> > Request: This is not new to this patch but can you change "reply->result =
> > false;" to the following instead of depending on the caller initializing
|reply|
> > to 0:
> > 
> > reply->body.msgh_descriptor_count = 0;
> > reply->result = false;
> > reply->io_surface_port.name = MACH_PORT_NULL;
> > reply->io_surface_port.disposition = 0;
> > reply->io_surface_port.type = 0;
> 
> Done. Also changed Register reply to set body.* to 0 as well.

Ah, I guess we should really be doing a bzero(reply, sizeof(*reply)) to not
depend on reply already being 0 initialized... current patch is fine though

[ccameron, 2015-09-18 19:37:33]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2015-09-18 19:37:33]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
reveman@chromium.org
Link to the patchset: https://codereview.chromium.org/1351753002/#ps100001
(title: "IRF")

[commit-bot: I haz the power, 2015-09-18 19:37:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1351753002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1351753002/100001

[commit-bot: I haz the power, 2015-09-18 19:43:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-18 19:44:01]

Patchset 6 (id:??) landed as
https://crrev.com/7c766ff43c0b84fb97e2a58c3be183c3d1339f23
Cr-Commit-Position: refs/heads/master@{#349743}