[Mac] Make app shims load the same framework version as the running Chrome process.

[Mac] Make app shims load the same framework version as the running Chrome process.

When a new version of Chrome has been downloaded but the old version is
still running, any app shims that start will load the new framework version.
This is problematic as the IPC codes (See ipc_message_start.h) may not match.

With this CL, app shims will check a version file written by the Chrome
process and load the corresponding framework version.

This also increments kCurrentAppShortcutsVersion, which causes all app
shims to be rebuilt.

This also moves AppModeChromeLocatorTest to browser_tests. It's not really
necessary for them to have their own target (app_mode_app_tests), and this
way they actually get run.

BUG=405347
Committed: https://crrev.com/aeac008d6856655cda70806584e74c6e67c4ca62
Cr-Commit-Position: refs/heads/master@{#292563}

[jackhou1, 2014-08-26 05:36:18]

jackhou@chromium.org changed reviewers:
+ tapted@chromium.org

[jackhou1, 2014-08-26 05:36:18]

tapted, could you take a look?

I'm hoping to get this merged into M38, so I'll remove app_mode_app_tests in a
follow-up.

Also, what do you think of moving app_mode_chrome_locator to apps/app_shim/
(which will eventually go to chrome/browser/app_shim) ?

[tapted, 2014-08-26 07:12:48]

I think the plan is good. One real concern below. Also we'll need a security
review - rsesek might have some ideas, would be good to loop him in soon.

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:92: // The user_data_dir for shims actually
contains the app_data_path.
for consistency: // ** 2: Read the running Chrome version.

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:94: base::FilePath user_data_dir =
Can you keep the ChromeAppModeInfo populating together if you do something like

  base::FilePath app_data_dir = base::mac::NSStringToFilePath([app_bundle
      objectForInfoDictionaryKey:app_mode::kCrAppModeUserDataDirKey]);
  base::FilePath user_data_dir = app_data_dir.DirName().DirName().DirName();

(i.e. what's done above for cr_bundle_id)

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:111: cr_version_str.value(),
I think we can only pass this in when we know for sure that Chrome is running. 

Otherwise, e.g. if Chrome crashes, the call to
`base::mac::OpenApplicationWithPath(base::mac::OuterBundlePath(), ...` in
ChromeAppModeStart could end up loading a newer version of Chrome

https://codereview.chromium.org/501303002/diff/40001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1996: 'app_mode_app_support',
might need an os==mac guard?

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator.mm:56: cr_version,
Do we need to handle the case that this path doesn't exist? Like.. Chrome
crashes, leaves this version goo in place. User rolls back to stable channel
which never updates the version. etc.

Perhaps the caller can just try calling again with an empty version_str for the
case GetChromeBundleInfo returns false

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator_unittest.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator_unittest.mm:114:
ASSERT_TRUE(base::CreateSymbolicLink(
I'm not sure what this test is for - comment?... (but here I think tests need to
clean things like this up after themselves?)

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_common.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_common.mm:14: extern const char
kAppShimVersionSymlinkName[] = "AppShimVersion";
no `extern` here.  Also maybe 'RunningChromeVersion` - I think that's a clearer
name, (although I guess at the cost of disassociating it with app shims, but
maybe someone else will want it).

[jackhou1, 2014-08-26 08:15:14]

Deleted app_mode_app_tests since it doesn't compile any more. Also it wasn't as
much work as I thought (unless I've missed stuff).

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:92: // The user_data_dir for shims actually
contains the app_data_path.
On 2014/08/26 07:12:47, tapted wrote:
> for consistency: // ** 2: Read the running Chrome version.

Done.

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:94: base::FilePath user_data_dir =
On 2014/08/26 07:12:47, tapted wrote:
> Can you keep the ChromeAppModeInfo populating together if you do something
like
> 
>   base::FilePath app_data_dir = base::mac::NSStringToFilePath([app_bundle
>       objectForInfoDictionaryKey:app_mode::kCrAppModeUserDataDirKey]);
>   base::FilePath user_data_dir = app_data_dir.DirName().DirName().DirName();
> 
> (i.e. what's done above for cr_bundle_id)

Done.

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:111: cr_version_str.value(),
On 2014/08/26 07:12:47, tapted wrote:
> I think we can only pass this in when we know for sure that Chrome is running.

> 
> Otherwise, e.g. if Chrome crashes, the call to
> `base::mac::OpenApplicationWithPath(base::mac::OuterBundlePath(), ...` in
> ChromeAppModeStart could end up loading a newer version of Chrome

Hmm... I think we can read the SingletonLock to confirm the pid is still
running. If not, load the newer version. WDYT?

https://codereview.chromium.org/501303002/diff/40001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:1996: 'app_mode_app_support',
On 2014/08/26 07:12:48, tapted wrote:
> might need an os==mac guard?

Done.

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator.mm:56: cr_version,
On 2014/08/26 07:12:48, tapted wrote:
> Do we need to handle the case that this path doesn't exist? Like.. Chrome
> crashes, leaves this version goo in place. User rolls back to stable channel
> which never updates the version. etc.
> 
> Perhaps the caller can just try calling again with an empty version_str for
the
> case GetChromeBundleInfo returns false

I think it's reasonable to handle it here. If the version passed in does not
correspond to a valid directory, then default to the latest version.

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator_unittest.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator_unittest.mm:114:
ASSERT_TRUE(base::CreateSymbolicLink(
On 2014/08/26 07:12:48, tapted wrote:
> I'm not sure what this test is for - comment?... (but here I think tests need
to
> clean things like this up after themselves?)

Done.

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_common.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_common.mm:14: extern const char
kAppShimVersionSymlinkName[] = "AppShimVersion";
On 2014/08/26 07:12:48, tapted wrote:
> no `extern` here.  Also maybe 'RunningChromeVersion` - I think that's a
clearer
> name, (although I guess at the cost of disassociating it with app shims, but
> maybe someone else will want it).

Done.

[jackhou1, 2014-08-26 08:16:56]

jackhou@chromium.org changed reviewers:
+ rsesek@chromium.org

[jackhou1, 2014-08-26 08:16:56]

rsesek, could you please review this for security?

[tapted, 2014-08-26 08:43:47]

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:111: cr_version_str.value(),
On 2014/08/26 08:15:14, jackhou1 wrote:
> On 2014/08/26 07:12:47, tapted wrote:
> > I think we can only pass this in when we know for sure that Chrome is
running.
> 
> > 
> > Otherwise, e.g. if Chrome crashes, the call to
> > `base::mac::OpenApplicationWithPath(base::mac::OuterBundlePath(), ...` in
> > ChromeAppModeStart could end up loading a newer version of Chrome
> 
> Hmm... I think we can read the SingletonLock to confirm the pid is still
> running. If not, load the newer version. WDYT?

yeah I think that should be OK - it's only a remote possibility to begin with,
so worrying about something else grabbing the same pid in the meantime is super
unlikely.  You could also use `[NSRunningApplication
runningApplicationsWithBundleIdentifier:bundle_id]` but I think it can have
multiple things if someone runs Chrome on Mac with --user-data-dir (or on a
multi-user machine).

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator.mm:56: cr_version,
On 2014/08/26 08:15:14, jackhou1 wrote:
> On 2014/08/26 07:12:48, tapted wrote:
> > Do we need to handle the case that this path doesn't exist? Like.. Chrome
> > crashes, leaves this version goo in place. User rolls back to stable channel
> > which never updates the version. etc.
> > 
> > Perhaps the caller can just try calling again with an empty version_str for
> the
> > case GetChromeBundleInfo returns false
> 
> I think it's reasonable to handle it here. If the version passed in does not
> correspond to a valid directory, then default to the latest version.

lg

https://codereview.chromium.org/501303002/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/501303002/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:2412: 'app_mode_app_support',
you .. might need ios as well in the condition :/

[Robert Sesek, 2014-08-26 16:41:08]

LGTM

https://codereview.chromium.org/501303002/diff/80001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/501303002/diff/80001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator.mm:30: arrayWithObjects:bundle_path,
@"Contents", @"Versions", version, nil];
You can use @[ ] syntax instead.

[jackhou1, 2014-08-27 03:04:52]

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/40001/chrome/app/app_mode_load...
chrome/app/app_mode_loader_mac.mm:111: cr_version_str.value(),
On 2014/08/26 08:43:47, tapted wrote:
> On 2014/08/26 08:15:14, jackhou1 wrote:
> > On 2014/08/26 07:12:47, tapted wrote:
> > > I think we can only pass this in when we know for sure that Chrome is
> running.
> > 
> > > 
> > > Otherwise, e.g. if Chrome crashes, the call to
> > > `base::mac::OpenApplicationWithPath(base::mac::OuterBundlePath(), ...` in
> > > ChromeAppModeStart could end up loading a newer version of Chrome
> > 
> > Hmm... I think we can read the SingletonLock to confirm the pid is still
> > running. If not, load the newer version. WDYT?
> 
> yeah I think that should be OK - it's only a remote possibility to begin with,
> so worrying about something else grabbing the same pid in the meantime is
super
> unlikely.  You could also use `[NSRunningApplication
> runningApplicationsWithBundleIdentifier:bundle_id]` but I think it can have
> multiple things if someone runs Chrome on Mac with --user-data-dir (or on a
> multi-user machine).

Reading SingletonLock doesn't seem to work, maybe we can't read it while Chrome
has a lock on it. In any case, it might be flaky if the singleton code changes.
Changed this to use NSRunningApplication.

https://codereview.chromium.org/501303002/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/501303002/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:2412: 'app_mode_app_support',
On 2014/08/26 08:43:47, tapted wrote:
> you .. might need ios as well in the condition :/

You mean OS=="mac" includes OS=="ios"? So I need OS=="mac" and OS!="ios"?

https://codereview.chromium.org/501303002/diff/80001/chrome/common/mac/app_mo...
File chrome/common/mac/app_mode_chrome_locator.mm (right):

https://codereview.chromium.org/501303002/diff/80001/chrome/common/mac/app_mo...
chrome/common/mac/app_mode_chrome_locator.mm:30: arrayWithObjects:bundle_path,
@"Contents", @"Versions", version, nil];
On 2014/08/26 16:41:08, rsesek wrote:
> You can use @[ ] syntax instead.

Done.

[tapted, 2014-08-27 03:51:16]

It would be good to double-check the PID, but I guess it's a super-remote
possibility.

And it looks like some tests are not happy on the bots too, but the rest lgtm if
its an easy fix.

https://codereview.chromium.org/501303002/diff/80001/chrome/chrome_tests_unit...
File chrome/chrome_tests_unit.gypi (right):

https://codereview.chromium.org/501303002/diff/80001/chrome/chrome_tests_unit...
chrome/chrome_tests_unit.gypi:2412: 'app_mode_app_support',
On 2014/08/27 03:04:52, jackhou1 wrote:
> On 2014/08/26 08:43:47, tapted wrote:
> > you .. might need ios as well in the condition :/
> 
> You mean OS=="mac" includes OS=="ios"? So I need OS=="mac" and OS!="ios"?

Ah, maybe not :). Depends what the bots say. OS_MACOSX includes ios, but I guess
it would make sense that OS=="mac" doesn't.

https://codereview.chromium.org/501303002/diff/100001/chrome/app/app_mode_loa...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/100001/chrome/app/app_mode_loa...
chrome/app/app_mode_loader_mac.mm:96: }
  // TODO(): Also check that the running Chrome has a matching user_data_dir.

[jackhou1, 2014-08-28 01:04:56]

https://codereview.chromium.org/501303002/diff/100001/chrome/app/app_mode_loa...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/100001/chrome/app/app_mode_loa...
chrome/app/app_mode_loader_mac.mm:96: }
On 2014/08/27 03:51:16, tapted wrote:
>   // TODO(): Also check that the running Chrome has a matching user_data_dir.

chrome_main_app_mode_mac checks for a running Chrome the same way. If it's not
using the same user_data_dir as this shim, the shim will just connect to a
non-existent (or dead) IPC socket, so it won't cause any problems.

But this does move app shims closer to supporting multiple user data dirs. Filed
a bug: http://crbug.com/Issue 407973

[jackhou1, 2014-08-28 01:06:07]

jackhou@chromium.org changed reviewers:
+ sky@chromium.org

[jackhou1, 2014-08-28 01:06:07]

sky, could you please review for OWNERS of:

chrome/app/
chrome/common/chrome_version_info.h

[sky, 2014-08-28 13:24:17]

sky@chromium.org changed reviewers:
+ thakis@chromium.org
- sky@chromium.org

[sky, 2014-08-28 13:24:17]

Please get a mac owner for this, maybe thakis?
Also, you didn't change: chrome/common/chrome_version_info.h

[jackhou1, 2014-08-28 22:41:57]

jackhou@chromium.org changed reviewers:
+ avi@chromium.org

[jackhou1, 2014-08-28 22:41:57]

avi, would you mind reviewing this while thakis is away?

chrome/app/app_mode_loader_mac.mm

[Avi (use Gerrit), 2014-08-28 23:36:20]

app_mode_loader_mac lgtm with nit

https://codereview.chromium.org/501303002/diff/140001/chrome/app/app_mode_loa...
File chrome/app/app_mode_loader_mac.mm (right):

https://codereview.chromium.org/501303002/diff/140001/chrome/app/app_mode_loa...
chrome/app/app_mode_loader_mac.mm:15: #import <Foundation/Foundation.h>
If you import Cocoa you don't need to import the other two. In fact, importing
Foundation should have replaced including Core Foundation.

[Avi (use Gerrit), 2014-08-28 23:37:04]

On 2014/08/28 22:41:57, jackhou1 wrote:
> avi, would you mind reviewing this while thakis is away?
> 
> chrome/app/app_mode_loader_mac.mm

BTW, I'm not an OWNER for chrome/app so even though I like it I don't think that
matters.

[jackhou1, 2014-08-28 23:58:24]

thakis, could you please stamp this now that avi has reviewed it?

[Nico, 2014-08-29 00:52:16]

lgtm

[jackhou1, 2014-08-29 00:57:18]

The CQ bit was checked by jackhou@chromium.org

[commit-bot: I haz the power, 2014-08-29 00:58:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jackhou@chromium.org/501303002/180001

[commit-bot: I haz the power, 2014-08-29 02:43:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-29 03:23:18]

Committed patchset #10 (id:180001) as ba348f9344e7cecad4b3e899dfeb9eff84a41a4c

[commit-bot: I haz the power, 2014-09-10 03:05:57]

Patchset 10 (id:??) landed as
https://crrev.com/aeac008d6856655cda70806584e74c6e67c4ca62
Cr-Commit-Position: refs/heads/master@{#292563}