Use loader-based binding of GetInstallDetailsPayload for install_static.

Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
Review-Url: https://codereview.chromium.org/2635953002
Cr-Commit-Position: refs/heads/master@{#444547}
Committed: https://chromium.googlesource.com/chromium/src/+/a47e8657c739f0ee2f11aecbd3558335c639a5eb

[grt (UTC plus 2), 2017-01-16 14:24:13]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[grt (UTC plus 2), 2017-01-16 14:24:14]

Description was changed from

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
==========

to

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[commit-bot: I haz the power, 2017-01-16 14:24:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-16 14:59:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 14:59:33]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[grt (UTC plus 2), 2017-01-16 16:22:32]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 16:22:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-01-16 16:54:01]

grt@chromium.org changed reviewers:
+ siggi@chromium.org

[grt (UTC plus 2), 2017-01-16 16:54:01]

Hi Siggi. Would you take a look, please?

[commit-bot: I haz the power, 2017-01-16 17:01:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-16 17:01:22]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Sigurður Ásgeirsson, 2017-01-16 18:28:59]

I think you need to add the exported functions to the chrome_elf DEF file for
them to exist in the import lib.
As a rule, IMHO we should use .DEF declarations in favor of the @#$%!
__declspec(dllexport) declarations, which make it impossible to reason about
where they're going to appear as exports (from sources.)

[Sigurður Ásgeirsson, 2017-01-16 18:31:58]

looks good for the most part, but doesn't seem to be linking ATM?

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/g...
File chrome/install_static/get_install_details_payload.cc (right):

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/g...
chrome/install_static/get_install_details_payload.cc:7: extern "C" const
install_static::InstallDetails::Payload __declspec(dllexport) *
yeah, I don't like the dllexport declarations, nor does the linker.

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/i...
File chrome/install_static/initialize_from_primary_module.h (right):

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/i...
chrome/install_static/initialize_from_primary_module.h:12: // function for this
express purpose).
doesn't this need to be in an extern "C" declaration for linking to work?

[grt (UTC plus 2), 2017-01-16 20:26:33]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 20:26:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-01-16 20:27:33]

Thanks. PTAL.

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/g...
File chrome/install_static/get_install_details_payload.cc (right):

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/g...
chrome/install_static/get_install_details_payload.cc:7: extern "C" const
install_static::InstallDetails::Payload __declspec(dllexport) *
On 2017/01/16 18:31:58, Sigurður Ásgeirsson wrote:
> yeah, I don't like the dllexport declarations, nor does the linker.

Removed in favor of .def file. The linker error was due to a missing dependency
on chrome_elf.dll by chrome_child.dll. I think I've fixed it.

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/i...
File chrome/install_static/initialize_from_primary_module.h (right):

https://codereview.chromium.org/2635953002/diff/20001/chrome/install_static/i...
chrome/install_static/initialize_from_primary_module.h:12: // function for this
express purpose).
On 2017/01/16 18:31:58, Sigurður Ásgeirsson wrote:
> doesn't this need to be in an extern "C" declaration for linking to work?

This function isn't used across DLL boundaries. It is a helper used by
"secondary" modules (i.e., chrome.exe, chrome.dll, chrome_watcher.dll, and
chrome_child.dll) which calls into the fn exported by the "primary" module
(i.e., chrome_elf).

[grt (UTC plus 2), 2017-01-16 20:28:21]

Description was changed from

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

[grt (UTC plus 2), 2017-01-16 20:28:21]

grt@chromium.org changed reviewers:
+ robertshield@chromium.org

[grt (UTC plus 2), 2017-01-16 20:29:31]

grt@chromium.org changed reviewers:
+ scottmg@chromium.org

[grt (UTC plus 2), 2017-01-16 20:30:53]

+OWNERS:
robertshield: chrome_elf
scottmg: chrome/BUILD.gn
Thanks

[Sigurður Ásgeirsson, 2017-01-16 20:35:50]

LGTM - except I think you forgot to add the .DEF file to this CL?

[grt (UTC plus 2), 2017-01-16 20:42:13]

On 2017/01/16 20:35:50, Sigurður Ásgeirsson wrote:
> LGTM - except I think you forgot to add the .DEF file to this CL?

Believe it or not, it was there the whole time from r432270. :-)

[scottmg, 2017-01-16 21:01:15]

chrome/BUILD.gn lgtm

https://codereview.chromium.org/2635953002/diff/40001/chrome/install_static/B...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/2635953002/diff/40001/chrome/install_static/B...
chrome/install_static/BUILD.gn:58: # InstallDetails instance.
It would help me (a future naïve reader) to say that the canonical example of
this module is chrome_elf, and below, chrome. (Assuming that's correct).

[grt (UTC plus 2), 2017-01-16 21:04:28]

The CQ bit was checked by grt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-16 21:04:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-01-16 21:06:47]

Thanks.

https://codereview.chromium.org/2635953002/diff/40001/chrome/install_static/B...
File chrome/install_static/BUILD.gn (right):

https://codereview.chromium.org/2635953002/diff/40001/chrome/install_static/B...
chrome/install_static/BUILD.gn:58: # InstallDetails instance.
On 2017/01/16 21:01:15, scottmg wrote:
> It would help me (a future naïve reader) to say that the canonical example of
> this module is chrome_elf, and below, chrome. (Assuming that's correct).

Great suggestion. Comments added. Future me will appreciate them, too.

[commit-bot: I haz the power, 2017-01-17 02:12:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-17 02:12:13]

Dry run: Try jobs failed on following builders:
  win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)

[robertshield, 2017-01-17 20:02:54]

chrome_elf LGTM, though note that I don't see a .def file if one is supposed to
be included.

[grt (UTC plus 2), 2017-01-18 03:27:52]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-01-18 03:27:53]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org,
scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2635953002/#ps60001
(title: "scottmg comments")

[commit-bot: I haz the power, 2017-01-18 03:28:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[grt (UTC plus 2), 2017-01-18 03:28:52]

On 2017/01/17 20:02:54, robertshield_slow_reviews wrote:
> chrome_elf LGTM, though note that I don't see a .def file if one is supposed
to
> be included.

Thanks. The .def file is already there. This preserves the existing export name.

[commit-bot: I haz the power, 2017-01-18 03:30:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 03:30:21]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[grt (UTC plus 2), 2017-01-18 07:28:47]

The CQ bit was checked by grt@chromium.org

[grt (UTC plus 2), 2017-01-18 07:28:47]

The patchset sent to the CQ was uploaded after l-g-t-m from siggi@chromium.org,
robertshield@chromium.org, scottmg@chromium.org
Link to the patchset: https://codereview.chromium.org/2635953002/#ps80001
(title: "sync to position 444298")

[commit-bot: I haz the power, 2017-01-18 07:29:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 11:37:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 11:37:26]

Try jobs failed on following builders:
  win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)

[grt (UTC plus 2), 2017-01-18 13:40:16]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2017-01-18 13:40:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 18:34:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 18:34:22]

Try jobs failed on following builders:
  win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)

[grt (UTC plus 2), 2017-01-18 23:36:37]

Description was changed from

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
==========

to

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
==========

[grt (UTC plus 2), 2017-01-18 23:37:12]

(+CC wfh)
removing master.tryserver.chromium.win:win10_chromium_x64_rel_ng which seems
broken.

[grt (UTC plus 2), 2017-01-18 23:37:20]

The CQ bit was checked by grt@chromium.org

[commit-bot: I haz the power, 2017-01-18 23:37:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 23:45:10]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484782640465040,
"parent_rev": "df85f01f89ed4d169fde3ffff938f21a1ad5edd5", "commit_rev":
"a47e8657c739f0ee2f11aecbd3558335c639a5eb"}

[commit-bot: I haz the power, 2017-01-18 23:45:43]

Description was changed from

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541
==========

to

==========
Use loader-based binding of GetInstallDetailsPayload for install_static.

The previous strategy of using GetModuleHandle + GetProcAddress was
overly complex and was causing some 3rd party software to tip renderers
over.

BUG=674541

Review-Url: https://codereview.chromium.org/2635953002
Cr-Commit-Position: refs/heads/master@{#444547}
Committed:
https://chromium.googlesource.com/chromium/src/+/a47e8657c739f0ee2f11aecbd355...
==========

[commit-bot: I haz the power, 2017-01-18 23:45:44]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/a47e8657c739f0ee2f11aecbd355...