Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1809383004:
Set current directory when launching Native Messaging processes. (Closed)
|
Created:
4 years, 9 months ago by Sergey Ulanov Modified:
4 years, 8 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet current directory when launching Native Messaging processes.
Previously NM host processes were started in the chrome directory,
which may interfere with chrome updates sometimes. Now they are
started in the directory in which the binary is located.
BUG=437339
Committed: https://crrev.com/78205516908eda56c64541cf38f963a991654acc
Cr-Commit-Position: refs/heads/master@{#385408}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : #Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : Fix Mac #
Total comments: 2
Patch Set 6 : #
Messages
Total messages: 56 (24 generated)
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/20001
sergeyu@chromium.org changed reviewers: + fsamuel@chromium.org, thestig@chromium.org
thestig@: please review base/ fsamuel@: please review everything else
Patchset #1 (id:1) has been deleted
On 2016/03/24 20:38:00, Sergey Ulanov wrote: > thestig@: please review base/ > fsamuel@: please review everything else I'm not an OWNER here: please pass this along to rdevlin.cronin@ Thanks!
sergeyu@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Description was changed from ========== Set current directory when launching Native Messaging processes. Previously NM host processes were started in the chrome directory, which may interfere with chrome updates sometimes. Now they are started in the directory in which the binary is located. BUG=437339 ========== to ========== Set current directory when launching Native Messaging processes. Previously NM host processes were started in the chrome directory, which may interfere with chrome updates sometimes. Now they are started in the directory in which the binary is located. BUG=437339 ==========
sergeyu@chromium.org changed reviewers: - fsamuel@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
You have a few red bots. Also, do we have an idea what the likelihood is that this will break any existing extensions, if they were relying on the native message binary to be launched with Chrome's cwd? https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/nativeMessaging.html (right): https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/nativeMessaging.html:51: manifest file is located. The host process is started with current directory with *the* current directory https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/nativeMessaging.html:52: set to the directory that contains the host binary. E.g. if this parameter is nit: line wrapping https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/nativeMessaging.html:52: set to the directory that contains the host binary. E.g. if this parameter is nit: let's say "For example" instead of e.g. here (this is the public documentation, and E.g. looks funny as the first part of a sentence).
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/40001
On 2016/03/24 21:27:59, Devlin wrote: > You have a few red bots. These were because I had an unrelated change in PS2, reverted it now. > Also, do we have an idea what the likelihood is that > this will break any existing extensions, if they were relying on the native > message binary to be launched with Chrome's cwd? I think it's very unlikely that this change will break any existing hosts. AFAIK chrome doesn't sets CWD, it just keeps the value set by the OS. So anything that relies on CWD is already very fragile. We were not setting it for NM processes, so they were inheriting it from chrome. This means that that on windows cwd was normally set to chrome's dir, but that's not guaranteed. On OSX, I believe, CWD is normally set to the bundle base, but it depends on how the process is started. On Linux CWD depends on how the process is started - it may be home directory, may be root, may be anything else. https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/nativeMessaging.html (right): https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/nativeMessaging.html:51: manifest file is located. The host process is started with current directory On 2016/03/24 21:27:59, Devlin wrote: > with *the* current directory Done. https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/nativeMessaging.html:52: set to the directory that contains the host binary. E.g. if this parameter is On 2016/03/24 21:27:59, Devlin wrote: > nit: let's say "For example" instead of e.g. here (this is the public > documentation, and E.g. looks funny as the first part of a sentence). Done. https://codereview.chromium.org/1809383004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/nativeMessaging.html:52: set to the directory that contains the host binary. E.g. if this parameter is On 2016/03/24 21:27:59, Devlin wrote: > nit: line wrapping Done.
extensions lgtm
Port ProcessUtilTest.CurrentDirectory to Windows? https://codereview.chromium.org/1809383004/diff/60001/base/process/launch_win.cc File base/process/launch_win.cc (right): https://codereview.chromium.org/1809383004/diff/60001/base/process/launch_win... base/process/launch_win.cc:294: ? NULL nullptr in new code?
On 2016/03/29 00:57:55, Lei Zhang wrote: > Port ProcessUtilTest.CurrentDirectory to Windows? Done. https://codereview.chromium.org/1809383004/diff/60001/base/process/launch_win.cc File base/process/launch_win.cc (right): https://codereview.chromium.org/1809383004/diff/60001/base/process/launch_win... base/process/launch_win.cc:294: ? NULL On 2016/03/29 00:57:54, Lei Zhang wrote: > nullptr in new code? Done.
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
On 2016/04/04 18:38:42, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) Unfortunately ProcessUtilTest.CurrentDirectory is failing on Mac. Do you have time to look into that, or do you want to just port ProcessUtilTest.CurrentDirectory to Windows (and not Mac) for now?
I'm trying to figure out why it fails on Mac. On 2016/04/04 21:15:48, Lei Zhang wrote: > On 2016/04/04 18:38:42, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, > > > http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) > > Unfortunately ProcessUtilTest.CurrentDirectory is failing on Mac. Do you have > time to look into that, or do you want to just port > ProcessUtilTest.CurrentDirectory to Windows (and not Mac) for now?
Patchset #5 (id:100001) has been deleted
The CQ bit was checked by sergeyu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
It's green now on Mac on Android. PTAL
On 2016/04/05 20:43:41, Sergey Ulanov wrote: > It's green now on Mac on Android. PTAL lgtm, and thanks for porting the test!
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1809383004/#ps120001 (title: "Fix Mac")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Oh, Android is still red. I also had a comment that I forgot to send. https://codereview.chromium.org/1809383004/diff/120001/base/process/process_u... File base/process/process_util_unittest.cc (right): https://codereview.chromium.org/1809383004/diff/120001/base/process/process_u... base/process/process_util_unittest.cc:223: CHECK(expected == actual) << "Expected: " << expected.value() Does CHECK_EQ() properly print out FilePaths? If yes, use that instead?
On 2016/04/05 23:35:22, Lei Zhang wrote: > Oh, Android is still red. I also had a comment that I forgot to send. Looks like on Android SpawnChild() doesn't call LunchProcess. It just forks the current process and calls the target function, see base/test/multiprocess_test_android.cc So I think I'll just disable the test on Android.
https://codereview.chromium.org/1809383004/diff/120001/base/process/process_u... File base/process/process_util_unittest.cc (right): https://codereview.chromium.org/1809383004/diff/120001/base/process/process_u... base/process/process_util_unittest.cc:223: CHECK(expected == actual) << "Expected: " << expected.value() On 2016/04/05 23:35:22, Lei Zhang wrote: > Does CHECK_EQ() properly print out FilePaths? If yes, use that instead? No, it doesn't work.
The CQ bit was checked by sergeyu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/1809383004/#ps140001 (title: " ")
On 2016/04/06 00:35:15, Sergey Ulanov wrote: > On 2016/04/05 23:35:22, Lei Zhang wrote: > > Oh, Android is still red. I also had a comment that I forgot to send. > > Looks like on Android SpawnChild() doesn't call LunchProcess. It just forks the > current process and calls the target function, see > base/test/multiprocess_test_android.cc > So I think I'll just disable the test on Android. Ack.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1809383004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1809383004/140001
Message was sent while issue was closed.
Description was changed from ========== Set current directory when launching Native Messaging processes. Previously NM host processes were started in the chrome directory, which may interfere with chrome updates sometimes. Now they are started in the directory in which the binary is located. BUG=437339 ========== to ========== Set current directory when launching Native Messaging processes. Previously NM host processes were started in the chrome directory, which may interfere with chrome updates sometimes. Now they are started in the directory in which the binary is located. BUG=437339 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Set current directory when launching Native Messaging processes. Previously NM host processes were started in the chrome directory, which may interfere with chrome updates sometimes. Now they are started in the directory in which the binary is located. BUG=437339 ========== to ========== Set current directory when launching Native Messaging processes. Previously NM host processes were started in the chrome directory, which may interfere with chrome updates sometimes. Now they are started in the directory in which the binary is located. BUG=437339 Committed: https://crrev.com/78205516908eda56c64541cf38f963a991654acc Cr-Commit-Position: refs/heads/master@{#385408} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/78205516908eda56c64541cf38f963a991654acc Cr-Commit-Position: refs/heads/master@{#385408} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
