Skip to content

Commit

Permalink
[mac] Better handling of lost Apple Events during App Shim Startup.
Browse files Browse the repository at this point in the history
AESendMessage accepts a timeout argument, but it is effectively ignored
by Mac. For asynchronous Apple Events (kAEQueueReply), kAEWantReceipt
must also be set to receive a message on timeout. However,
kAEWantReceipt is "Deprecated and unsupported in Mac OS X".

Also, the "ping" apple event allows an app shim to ensure it doesn't try
to  connect to Chrome until it has finished starting up, when it is the
shim  that has just started Chrome. However, if Chrome is shutting down,
the apple event is lost and the shim process must wait for a long
timeout.

Instead, if the shim itself is not launching Chrome, this CL proceeds 
straight to init. This will either succeed quickly, fail to connect to 
the shim socket, or connect and get disconnected quickly. Both "fail" 
cases are preferable to a timeout. 

For the Chrome-not-running case, we still can not leave zombie processes
around that never get their Apple Event reply. For this case, emulate
the broken Apple Event timeout by posting a DelayedTask to the UI
MessageLoop in the app shim process.

BUG=318013
TEST=With Chrome not running, Start App Launcher via dock icon then
(really quickly) launch the app launcher again. Chrome must still be
shutting down, and the Apple Event is lost. After 1 minute the App
Launcher should work again.

Review URL: https://codereview.chromium.org/68523004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@234494 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
tapted@chromium.org committed Nov 12, 2013
1 parent def4bce commit 80da61e
Showing 1 changed file with 54 additions and 25 deletions.
79 changes: 54 additions & 25 deletions apps/app_shim/chrome_main_app_mode_mac.mm
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ - (void)disableRelaunchOnLogin;

namespace {

// Timeout in seconds to wait for a reply for the initial Apple Event. Note that
// kAEDefaultTimeout on Mac is "about one minute" according to Apple's
// documentation, but is no longer supported for asynchronous Apple Events.
const int kPingChromeTimeoutSeconds = 60;

const app_mode::ChromeAppModeInfo* g_info;
base::Thread* g_io_thread = NULL;

Expand Down Expand Up @@ -97,6 +102,10 @@ - (void)terminateNow;
// was sent, or when the ping fails (if |success| is false).
void OnPingChromeReply(bool success);

// Called |kPingChromeTimeoutSeconds| after startup, to allow a timeout on the
// ping event to be detected.
void OnPingChromeTimeout();

// Connects to Chrome and sends a LaunchApp message.
void Init();

Expand Down Expand Up @@ -134,14 +143,16 @@ bool SendFocusApp(apps::AppShimFocusType focus_type,
IPC::ChannelProxy* channel_;
base::scoped_nsobject<AppShimDelegate> delegate_;
bool launch_app_done_;
bool ping_chrome_reply_received_;

DISALLOW_COPY_AND_ASSIGN(AppShimController);
};

AppShimController::AppShimController()
: channel_(NULL),
delegate_([[AppShimDelegate alloc] init]),
launch_app_done_(false) {
launch_app_done_(false),
ping_chrome_reply_received_(false) {
// Since AppShimController is created before the main message loop starts,
// NSApp will not be set, so use sharedApplication.
[[NSApplication sharedApplication] setDelegate:delegate_];
Expand All @@ -153,6 +164,7 @@ bool SendFocusApp(apps::AppShimFocusType focus_type,
}

void AppShimController::OnPingChromeReply(bool success) {
ping_chrome_reply_received_ = true;
if (!success) {
[NSApp terminate:nil];
return;
Expand All @@ -161,6 +173,11 @@ bool SendFocusApp(apps::AppShimFocusType focus_type,
Init();
}

void AppShimController::OnPingChromeTimeout() {
if (!ping_chrome_reply_received_)
[NSApp terminate:nil];
}

void AppShimController::Init() {
DCHECK(g_io_thread);

Expand Down Expand Up @@ -476,9 +493,10 @@ - (void)pingProcess:(const ProcessSerialNumber&)psn {
targetDescriptor:target
returnID:kAutoGenerateReturnID
transactionID:kAnyTransactionID];
// And away we go.
// TODO(jeremya): if we don't care about the contents of the reply, can we
// pass NULL for the reply event parameter?

// Note that AESendMessage effectively ignores kAEDefaultTimeout, because this
// call does not pass kAEWantReceipt (which is deprecated and unsupported on
// Mac). Instead, rely on OnPingChromeTimeout().
OSStatus status = AESendMessage(
[initial_event aeDesc], &replyEvent_, kAEQueueReply, kAEDefaultTimeout);
if (status != noErr) {
Expand Down Expand Up @@ -577,14 +595,14 @@ int ChromeAppModeStart(const app_mode::ChromeAppModeInfo* info) {
pid = [[existing_chrome objectAtIndex:0] processIdentifier];
}

// Launch Chrome if it isn't already running.
ProcessSerialNumber psn;
if (pid > -1) {
OSStatus status = GetProcessForPID(pid, &psn);
if (status)
return 1;
AppShimController controller;
base::MessageLoopForUI main_message_loop;
main_message_loop.set_thread_name("MainThread");
base::PlatformThread::SetName("CrAppShimMain");

} else {
if (pid == -1) {
// Launch Chrome if it isn't already running.
ProcessSerialNumber psn;
CommandLine command_line(CommandLine::NO_PROGRAM);
command_line.AppendSwitch(switches::kSilentLaunch);
command_line.AppendSwitchPath(switches::kProfileDirectory,
Expand All @@ -596,22 +614,33 @@ int ChromeAppModeStart(const app_mode::ChromeAppModeInfo* info) {
&psn);
if (!success)
return 1;
}

AppShimController controller;
base::Callback<void(bool)> on_ping_chrome_reply =
base::Bind(&AppShimController::OnPingChromeReply,
base::Unretained(&controller));

// This code abuses the fact that Apple Events sent before the process is
// fully initialized don't receive a reply until its run loop starts. Once
// the reply is received, Chrome will have opened its IPC port, guaranteed.
[ReplyEventHandler pingProcess:psn
andCall:on_ping_chrome_reply];
base::Callback<void(bool)> on_ping_chrome_reply =
base::Bind(&AppShimController::OnPingChromeReply,
base::Unretained(&controller));

// This code abuses the fact that Apple Events sent before the process is
// fully initialized don't receive a reply until its run loop starts. Once
// the reply is received, Chrome will have opened its IPC port, guaranteed.
[ReplyEventHandler pingProcess:psn
andCall:on_ping_chrome_reply];

main_message_loop.PostDelayedTask(
FROM_HERE,
base::Bind(&AppShimController::OnPingChromeTimeout,
base::Unretained(&controller)),
base::TimeDelta::FromSeconds(kPingChromeTimeoutSeconds));
} else {
// Chrome already running. Proceed to init. This could still fail if Chrome
// is still starting up or shutting down, but the process will exit quickly,
// which is preferable to waiting for the Apple Event to timeout after one
// minute.
main_message_loop.PostTask(
FROM_HERE,
base::Bind(&AppShimController::Init,
base::Unretained(&controller)));
}

base::MessageLoopForUI main_message_loop;
main_message_loop.set_thread_name("MainThread");
base::PlatformThread::SetName("CrAppShimMain");
main_message_loop.Run();
return 0;
}

0 comments on commit 80da61e

Please sign in to comment.