Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2656443002:
Use chrome::kChromeVersion in installer::GetModulePath(). (Closed)
DescriptionUse chrome::kChromeVersion in installer::GetModulePath().
Before this CL, installer::GetModulePath() used the version in the
VS_VERSION_INFO resource of the current module to construct the path
to another module. Unfortunately, reading that resource sometimes
crashed. With this CL, installer::GetModulePath() uses the
chrome::kChromeVersion constant instead. This contains the same
value as the VS_VERSION_INFO resource.
BUG=682987
Review-Url: https://codereview.chromium.org/2656443002
Cr-Commit-Position: refs/heads/master@{#448055}
Committed: https://chromium.googlesource.com/chromium/src/+/58feb04ce1dda2ff33edaf1973de3f4fefb5f56e
Patch Set 1 #
Total comments: 4
Patch Set 2 : move to anonymous namespace and test with chrome.exe #Patch Set 3 : fix build error #Patch Set 4 : data_deps #Patch Set 5 : no += #Patch Set 6 : data #
Total comments: 2
Patch Set 7 : CR grt #29 (update test comment) #Patch Set 8 : rebase #
Messages
Total messages: 44 (33 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + grt@chromium.org
PTAL
Thanks for picking this up! https://codereview.chromium.org/2656443002/diff/1/chrome/common/chrome_consta... File chrome/common/chrome_constants_win_unittest.cc (right): https://codereview.chromium.org/2656443002/diff/1/chrome/common/chrome_consta... chrome/common/chrome_constants_win_unittest.cc:20: FileVersionInfo::CreateFileVersionInfoForModule(CURRENT_MODULE())); we want a test that will fail specifically if the version in chrome.exe doesn't match the version in kChromeVersion. is it possible to test that? //chrome/test:unit_tests has an indirect dependency on //chrome:chrome_initial (a.k.a. chrome.exe), so can this test read the version from chrome.exe in the build output directory? https://codereview.chromium.org/2656443002/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2656443002/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:39: // the current executable's directory. This is the expected location of "current executable" makes this a bit tricky. looking at it now, i see that the old implementation was only correct when called from within chrome.exe -- a hypothetical caller in chrome.dll (or another module in the process) would have taken the version out of its own module rather than chrome.exe. since this is only meant to be used by chrome.exe to find the main dll (including the child and the watcher), i think it's safest for this to be moved into chrome/app and to be built only into the chrome_initial target.
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ptanl https://codereview.chromium.org/2656443002/diff/1/chrome/common/chrome_consta... File chrome/common/chrome_constants_win_unittest.cc (right): https://codereview.chromium.org/2656443002/diff/1/chrome/common/chrome_consta... chrome/common/chrome_constants_win_unittest.cc:20: FileVersionInfo::CreateFileVersionInfoForModule(CURRENT_MODULE())); On 2017/01/24 09:37:16, grt (UTC plus 1) wrote: > we want a test that will fail specifically if the version in chrome.exe doesn't > match the version in kChromeVersion. is it possible to test that? > //chrome/test:unit_tests has an indirect dependency on //chrome:chrome_initial > (a.k.a. chrome.exe), so can this test read the version from chrome.exe in the > build output directory? Done. https://codereview.chromium.org/2656443002/diff/1/chrome/installer/util/modul... File chrome/installer/util/module_util_win.cc (right): https://codereview.chromium.org/2656443002/diff/1/chrome/installer/util/modul... chrome/installer/util/module_util_win.cc:39: // the current executable's directory. This is the expected location of On 2017/01/24 09:37:16, grt (UTC plus 1) wrote: > "current executable" makes this a bit tricky. looking at it now, i see that the > old implementation was only correct when called from within chrome.exe -- a > hypothetical caller in chrome.dll (or another module in the process) would have > taken the version out of its own module rather than chrome.exe. since this is > only meant to be used by chrome.exe to find the main dll (including the child > and the watcher), i think it's safest for this to be moved into chrome/app and > to be built only into the chrome_initial target. Done.
lgtm with a nit. thanks! https://codereview.chromium.org/2656443002/diff/100001/chrome/common/chrome_c... File chrome/common/chrome_constants_win_unittest.cc (right): https://codereview.chromium.org/2656443002/diff/100001/chrome/common/chrome_c... chrome/common/chrome_constants_win_unittest.cc:18: // resource of the current module. nit: update comment
fdoray@chromium.org changed reviewers: + jochen@chromium.org
jochen@chromium.org: Please review changes in chrome/common/chrome_constants_win_unittest.cc
jochen@chromium.org: Please review changes in chrome/common/chrome_constants_win_unittest.cc https://codereview.chromium.org/2656443002/diff/100001/chrome/common/chrome_c... File chrome/common/chrome_constants_win_unittest.cc (right): https://codereview.chromium.org/2656443002/diff/100001/chrome/common/chrome_c... chrome/common/chrome_constants_win_unittest.cc:18: // resource of the current module. On 2017/02/01 12:36:32, grt (UTC plus 1) wrote: > nit: update comment Done.
lgtm
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from grt@chromium.org Link to the patchset: https://codereview.chromium.org/2656443002/#ps120001 (title: "CR grt #29 (update test comment)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/test/BUILD.gn:
While running git apply --index -p1;
error: patch failed: chrome/test/BUILD.gn:3374
error: chrome/test/BUILD.gn: patch does not apply
Patch: chrome/test/BUILD.gn
Index: chrome/test/BUILD.gn
diff --git a/chrome/test/BUILD.gn b/chrome/test/BUILD.gn
index
53f50a306e76cef0f2f15be7863b99f0cea8c362..964fff667e88a2061cf1b44c675b456f31558277
100644
--- a/chrome/test/BUILD.gn
+++ b/chrome/test/BUILD.gn
@@ -3374,6 +3374,7 @@ test("unit_tests") {
"../browser/webshare/share_service_impl_unittest.cc",
"../browser/win/chrome_elf_init_unittest.cc",
"../browser/win/enumerate_modules_model_unittest.cc",
+ "../common/chrome_constants_win_unittest.cc",
"../common/chrome_content_client_unittest.cc",
"../common/chrome_paths_unittest.cc",
"../common/component_flash_hint_file_linux_unittest.cc",
@@ -3451,6 +3452,12 @@ test("unit_tests") {
if (is_linux || is_win) {
data += [ "$root_out_dir/chrome_200_percent.pak" ]
}
+ if (is_win) {
+ data_deps = [
+ "//chrome:chrome_initial",
+ ]
+ data += [ "$root_out_dir/chrome.exe" ]
+ }
defines = []
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, grt@chromium.org Link to the patchset: https://codereview.chromium.org/2656443002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1486144905231210,
"parent_rev": "d6c9a754a1e61690287f87051318c1ccac8d063c", "commit_rev":
"58feb04ce1dda2ff33edaf1973de3f4fefb5f56e"}
Message was sent while issue was closed.
Description was changed from ========== Use chrome::kChromeVersion in installer::GetModulePath(). Before this CL, installer::GetModulePath() used the version in the VS_VERSION_INFO resource of the current module to construct the path to another module. Unfortunately, reading that resource sometimes crashed. With this CL, installer::GetModulePath() uses the chrome::kChromeVersion constant instead. This contains the same value as the VS_VERSION_INFO resource. BUG=682987 ========== to ========== Use chrome::kChromeVersion in installer::GetModulePath(). Before this CL, installer::GetModulePath() used the version in the VS_VERSION_INFO resource of the current module to construct the path to another module. Unfortunately, reading that resource sometimes crashed. With this CL, installer::GetModulePath() uses the chrome::kChromeVersion constant instead. This contains the same value as the VS_VERSION_INFO resource. BUG=682987 Review-Url: https://codereview.chromium.org/2656443002 Cr-Commit-Position: refs/heads/master@{#448055} Committed: https://chromium.googlesource.com/chromium/src/+/58feb04ce1dda2ff33edaf1973de... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/58feb04ce1dda2ff33edaf1973de... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
