Switch Chrome to dlopen() the Google Chrome Framework.framework.

Switch Chrome to dlopen() the Google Chrome Framework.framework.

Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag, because the restrict flag will not allow relative search
paths to be used for linking libraries.

BUG=523041
Committed: https://crrev.com/ae428cc69d7a7fbc5b321851ba94b93369628a7a
Cr-Commit-Position: refs/heads/master@{#358110}

[Greg K, 2015-10-20 19:10:13]

Patchset #1 (id:1) has been deleted

[Greg K, 2015-10-20 19:10:22]

Patchset #1 (id:20001) has been deleted

[Greg K, 2015-10-20 19:10:36]

Patchset #1 (id:40001) has been deleted

[Greg K, 2015-10-20 19:10:52]

Patchset #1 (id:60001) has been deleted

[Greg K, 2015-10-20 19:11:03]

Patchset #1 (id:80001) has been deleted

[Greg K, 2015-10-20 19:11:10]

Patchset #1 (id:100001) has been deleted

[Greg K, 2015-10-22 21:46:48]

Patchset #1 (id:120001) has been deleted

[Greg K, 2015-10-22 22:12:23]

Description was changed from

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag.

BUG=523041
==========

to

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag.

BUG=523041
==========

[Greg K, 2015-10-22 22:52:06]

kerrnel@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2015-10-22 23:15:12]

The CL description says that this is necessary for restrict, but not why. We
know why, but future readers might not.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_mac.c (right):

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:38: char* exec_path = malloc(exec_path_size);
fprintf(stderr, "malloc %zu: %s\n", framework_path_size, strerror(errno));

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:54: char* last_slash = strrchr(exec_path, '/');
Hmm, this will be messed-up with valid but stupid paths like “Google
Chrome.app/Contents/MacOS/////Google Chrome”. You can #include <libgen.h> and
use dirname() instead. Then you can free() the executable path. dirname()
doesn’t give you a trailing /, but you can deal with that later.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:73: framework_path[exec_path_len +
rel_path_len] = '\0';
snprintf(framework_path, framework_path_size, "%s/%s", exec_path, rel_path);

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:75: void* library = dlopen(framework_path,
RTLD_LAZY);
| RTLD_LOCAL | RTLD_FIRST

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:77: fprintf(stderr, "Unable to dlopen() the
framework.\n");
Say the framework name dlerror(). Something like

  fprintf(stderr, "dlopen %s: %s\n", framework_path, dlerror());

for brevity

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:79: }
free() framework_path now.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:81: ChromeMainPtr ChromeMain = dlsym(library,
"ChromeMain");
It’s still a variable, call it chrome_main

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:83: fprintf(stderr, "Could not resolve the
ChromeMain symbol.\n");
Similarly, fprintf(stderr, "dlopen ChromeMain: %s\n", dlerror());

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:88: // Free all resources here.
Technically the library handle is a freeable resource too, but you shouldn’t
bother freeing it. But you’ll have moved any frees up above, so this will go
away.

https://codereview.chromium.org/1412803002/diff/140001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/1412803002/diff/140001/chrome/chrome.gyp#newc...
chrome/chrome.gyp:172: 'app/chrome_exe_main_mac.c',
Depend on the version_header target

[Greg K, 2015-10-23 19:12:49]

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_mac.c (right):

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:38: char* exec_path = malloc(exec_path_size);
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> fprintf(stderr, "malloc %zu: %s\n", framework_path_size, strerror(errno));

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:54: char* last_slash = strrchr(exec_path, '/');
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> Hmm, this will be messed-up with valid but stupid paths like “Google
> Chrome.app/Contents/MacOS/////Google Chrome”. You can #include <libgen.h> and
> use dirname() instead. Then you can free() the executable path. dirname()
> doesn’t give you a trailing /, but you can deal with that later.

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:73: framework_path[exec_path_len +
rel_path_len] = '\0';
On 2015/10/22 23:15:12, Mark Mentovai wrote:
> snprintf(framework_path, framework_path_size, "%s/%s", exec_path, rel_path);

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:75: void* library = dlopen(framework_path,
RTLD_LAZY);
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> | RTLD_LOCAL | RTLD_FIRST

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:77: fprintf(stderr, "Unable to dlopen() the
framework.\n");
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> Say the framework name dlerror(). Something like
> 
>   fprintf(stderr, "dlopen %s: %s\n", framework_path, dlerror());
> 
> for brevity

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:79: }
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> free() framework_path now.

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:81: ChromeMainPtr ChromeMain = dlsym(library,
"ChromeMain");
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> It’s still a variable, call it chrome_main

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:83: fprintf(stderr, "Could not resolve the
ChromeMain symbol.\n");
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> Similarly, fprintf(stderr, "dlopen ChromeMain: %s\n", dlerror());

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:88: // Free all resources here.
On 2015/10/22 23:15:11, Mark Mentovai wrote:
> Technically the library handle is a freeable resource too, but you shouldn’t
> bother freeing it. But you’ll have moved any frees up above, so this will go
> away.

Done.

https://codereview.chromium.org/1412803002/diff/140001/chrome/chrome.gyp
File chrome/chrome.gyp (right):

https://codereview.chromium.org/1412803002/diff/140001/chrome/chrome.gyp#newc...
chrome/chrome.gyp:172: 'app/chrome_exe_main_mac.c',
On 2015/10/22 23:15:12, Mark Mentovai wrote:
> Depend on the version_header target

Done.

[Greg K, 2015-10-23 19:17:59]

Description was changed from

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag.

BUG=523041
==========

to

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag, because the restrict flag will not allow relative search paths to
be used for linking libraries.

BUG=523041
==========

[Mark Mentovai, 2015-10-23 20:56:35]

This is the good stuff. LGTM with these changes.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_mac.c (right):

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
Basically a new file, Copyright 2015, no (c)

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:36: "Unexpected return condition from
_NSGetExecutablePath().\n");
Let’s rationalize these too, so, like, "_NSGetExecutablePath: get [length|path]
failed\n"

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:65: // 2 accounts for a trailing NIL byte and
the '/' in the middle of the paths.
NUL, not NIL

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:83: fprintf(stderr, "dlopen ChromeMain: %s\n",
dlerror());
dlsym

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:86: int rv = chrome_main(argc, argv);
Share rc and rv (probably named rv)

[Mark Mentovai, 2015-10-23 20:58:15]

Actually, not LGTM until this is fixed.

You do want to build chrome_dll as dependency of chrome_main, but you don’t want
to link chrome_main against it.

It may be enough to set chrome_dll to loadable_module on Mac instead of
shared_library.

https://codereview.chromium.org/1412803002/diff/180001/chrome/chrome_exe.gypi
File chrome/chrome_exe.gypi (right):

https://codereview.chromium.org/1412803002/diff/180001/chrome/chrome_exe.gypi...
chrome/chrome_exe.gypi:274: 'chrome_dll',
Oops.

[Greg K, 2015-10-30 20:12:16]

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_mac.c (right):

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> Basically a new file, Copyright 2015, no (c)

Done.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:36: "Unexpected return condition from
_NSGetExecutablePath().\n");
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> Let’s rationalize these too, so, like, "_NSGetExecutablePath: get
[length|path]
> failed\n"

Done.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:65: // 2 accounts for a trailing NIL byte and
the '/' in the middle of the paths.
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> NUL, not NIL

Done.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:83: fprintf(stderr, "dlopen ChromeMain: %s\n",
dlerror());
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> dlsym

Done.

[Greg K, 2015-10-30 20:12:16]

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_mac.c (right):

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:1: // Copyright (c) 2011 The Chromium Authors.
All rights reserved.
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> Basically a new file, Copyright 2015, no (c)

Done.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:36: "Unexpected return condition from
_NSGetExecutablePath().\n");
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> Let’s rationalize these too, so, like, "_NSGetExecutablePath: get
[length|path]
> failed\n"

Done.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:65: // 2 accounts for a trailing NIL byte and
the '/' in the middle of the paths.
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> NUL, not NIL

Done.

https://codereview.chromium.org/1412803002/diff/180001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:83: fprintf(stderr, "dlopen ChromeMain: %s\n",
dlerror());
On 2015/10/23 20:56:35, Mark Mentovai wrote:
> dlsym

Done.

[Mark Mentovai, 2015-10-30 20:13:11]

You said something earlier about maybe not needing this, but I didn’t really
follow. What’s the story?

[Greg K, 2015-10-30 20:17:28]

On 2015/10/30 20:13:11, Mark Mentovai wrote:
> You said something earlier about maybe not needing this, but I didn’t really
> follow. What’s the story?

This, including the shim, is all necessary. The question is, do we want to
cleanup the code in Chromium that sets up @rpath as well? Because as long as it
isn't used when loading the libraries, OS X doesn't complain.

[Mark Mentovai, 2015-10-30 20:41:40]

I thought that the only @rpath stuff we had was for the component=shared_library
build. We don’t set up any rpaths elsewhere. You can verify by checking for the
absence of LC_RPATH in otool -l output for a non-component=shared_library build.

[Mark Mentovai, 2015-10-30 20:44:21]

Yes, yes, yes! LGTM.

[Greg K, 2015-10-30 20:47:13]

kerrnel@chromium.org changed reviewers:
+ jochen@chromium.org

[Greg K, 2015-10-30 20:47:14]

jochen@chromium.org: please provide an OWNERS review. Thank you.

[jochen (gone - plz use gerrit), 2015-11-03 07:15:03]

jochen@chromium.org changed reviewers:
+ brettw@chromium.org

[jochen (gone - plz use gerrit), 2015-11-03 07:15:04]

why's the gyp configuration so much more complicated than the gn config?
(+brettw for gn question)

Can you please update the CL description to be <=80c per line, and have a
summary as first line?

something like

summary.

Length description goes,
here.

[Greg K, 2015-11-03 17:44:41]

Description was changed from

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag, because the restrict flag will not allow relative search paths to
be used for linking libraries.

BUG=523041
==========

to

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework.

Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag, because the restrict flag will not allow relative search paths 
to be used for linking libraries.

BUG=523041
==========

[Greg K, 2015-11-03 17:45:01]

Description was changed from

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework.

Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag, because the restrict flag will not allow relative search paths 
to be used for linking libraries.

BUG=523041
==========

to

==========
Switch Chrome to dlopen() the Google Chrome Framework.framework.

Switch Chrome to dlopen() the Google Chrome Framework.framework,
instead of relying on the linker. This is necessary to opt into the
restrict flag, because the restrict flag will not allow relative search 
paths to be used for linking libraries.

BUG=523041
==========

[Greg K, 2015-11-04 01:05:46]

On 2015/11/03 07:15:04, jochen wrote:
> why's the gyp configuration so much more complicated than the gn config?
> (+brettw for gn question)
> 
> Can you please update the CL description to be <=80c per line, and have a
> summary as first line?
> 
> something like
> 
> summary.
> 
> Length description goes,
> here.

Ok, I updated the CL description, thanks. I'm waiting to hear from brettw more
about the gn configuration.

[brettw, 2015-11-04 05:37:05]

GN build LGTM. helper_app doesn't exist in GN Mac yet, and GN can also natively
express "a shared library that isn't linked to" which will avoid the need for
the dependency shim.

https://codereview.chromium.org/1412803002/diff/200001/chrome/app/chrome_exe_...
File chrome/app/chrome_exe_main_mac.c (right):

https://codereview.chromium.org/1412803002/diff/200001/chrome/app/chrome_exe_...
chrome/app/chrome_exe_main_mac.c:6: // bundle (browser) and helper app
(renderer, plugin, and friends).
Just to clarify, this file is a C-only file to avoid linking the C++ libraries
into the (small) binary? Can you mention that at the top of this file if so?

If instead this is a .c to save the extern statements, I would prefer to keep it
.cc so we can have more flexibility.

[Greg K, 2015-11-04 18:05:54]

On 2015/11/04 05:37:05, brettw wrote:
> GN build LGTM. helper_app doesn't exist in GN Mac yet, and GN can also
natively
> express "a shared library that isn't linked to" which will avoid the need for
> the dependency shim.

Thanks Brett, that sounds good.

>
https://codereview.chromium.org/1412803002/diff/200001/chrome/app/chrome_exe_...
> File chrome/app/chrome_exe_main_mac.c (right):
> 
>
https://codereview.chromium.org/1412803002/diff/200001/chrome/app/chrome_exe_...
> chrome/app/chrome_exe_main_mac.c:6: // bundle (browser) and helper app
> (renderer, plugin, and friends).
> Just to clarify, this file is a C-only file to avoid linking the C++ libraries
> into the (small) binary? Can you mention that at the top of this file if so?
> 
> If instead this is a .c to save the extern statements, I would prefer to keep
it
> .cc so we can have more flexibility.

This is a .c file because we may start statically linking libc++ into Chrome, in
which case, we wanted to avoid linking it here if this target uses no C++ code.

[jochen (gone - plz use gerrit), 2015-11-05 01:29:24]

lgtm

[Greg K, 2015-11-05 18:06:43]

The CQ bit was checked by kerrnel@chromium.org

[commit-bot: I haz the power, 2015-11-05 18:07:38]

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

[commit-bot: I haz the power, 2015-11-05 19:48:55]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2015-11-05 19:49:52]

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

[michaeln, 2015-11-07 02:20:29]

Could this change be the cause of this bug?
https://code.google.com/p/chromium/issues/detail?id=552655