Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294973003:
adb_chrome_public_command_line should write to old and new paths (Closed)
Descriptionadb_chrome_public_command_line should write to old and new paths
In Android N, Chrome can no longer read /data/local/chrome-command-line
due to SELinux (https://crbug.com/593720). Hence command line arguments
are read from /data/local/tmp/chrome-command-line instead on userdebug
or eng builds.
This patch makes adb_chrome_public_command_line write the same arguments
to both the old and the new location, so it works with both old and new
builds of Chrome.
BUG=593720
Committed: https://crrev.com/f1b179cce6d03443a0e2252a4bedcb49061099d3
Cr-Commit-Position: refs/heads/master@{#416523}
Patch Set 1 #
Total comments: 3
Messages
Total messages: 14 (4 generated)
johnme@chromium.org changed reviewers: + perezju@chromium.org
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
I think we should be using flag_changer (once we fix https://bugs.chromium.org/p/chromium/issues/detail?id=640228) in adb_command_line.py to handle the old & new paths. https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... File build/android/adb_chrome_public_command_line (right): https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... build/android/adb_chrome_public_command_line:20: $(dirname $0)/adb_command_line.py --device-path \ Should this be exec $(dirname $0)/adb_command_line.py like the line below?
> I think we should be using flag_changer (once we fix > https://bugs.chromium.org/p/chromium/issues/detail?id=640228) > in adb_command_line.py to handle the old & new paths. Hmm, I can see two problems with using the logic from https://codereview.chromium.org/2275863002 to decide whether to write to /data/local/ or /data/local/tmp/: 1. It assumes the build on the device is more recent than https://codereview.chromium.org/2160243002, but that might not be the case. 2. It assumes that unrooted devices that are not userdebug or eng builds should continue to use /data/local/. That's probably fine for automated use, but may not always be correct for manual use, since if someone marks Chrome as the debug app, Chrome will read from /data/local/tmp/. Even if those are resolved, switching adb_command_line.py over to flag_changer.py is likely to be a more complicated refactoring, so I think it makes sense to land this first anyway. https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... File build/android/adb_chrome_public_command_line (right): https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... build/android/adb_chrome_public_command_line:20: $(dirname $0)/adb_command_line.py --device-path \ On 2016/08/31 15:10:23, jbudorick wrote: > Should this be exec $(dirname $0)/adb_command_line.py like the line below? No, exec is an optimisation that causes bash to stop executing the current script and replace its process with the process being run (instead of forking). So if we exec here, we'll never run the line below.
On 2016/08/31 17:41:46, johnme wrote: > > I think we should be using flag_changer (once we fix > > https://bugs.chromium.org/p/chromium/issues/detail?id=640228) > > in adb_command_line.py to handle the old & new paths. > > Hmm, I can see two problems with using the logic from > https://codereview.chromium.org/2275863002 to decide whether to write to > /data/local/ or /data/local/tmp/: > > 1. It assumes the build on the device is more recent than > https://codereview.chromium.org/2160243002, but that might not be the case. The only scenario where this would come up would be someone using an up-to-date checkout and an old build. That's certainly not a case we're concerned about on the bots, and I'm not sure how often it'd come up for devs running locally. > > 2. It assumes that unrooted devices that are not userdebug or eng builds should > continue to use /data/local/. That's probably fine for automated use, but may > not always be correct for manual use, since if someone marks Chrome as the debug > app, Chrome will read from /data/local/tmp/. Yeah, looks like I missed the debug app case. > > Even if those are resolved, switching adb_command_line.py over to > flag_changer.py is likely to be a more complicated refactoring, so I think it > makes sense to land this first anyway. > > https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... > File build/android/adb_chrome_public_command_line (right): > > https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... > build/android/adb_chrome_public_command_line:20: $(dirname > $0)/adb_command_line.py --device-path \ > On 2016/08/31 15:10:23, jbudorick wrote: > > Should this be exec $(dirname $0)/adb_command_line.py like the line below? > > No, exec is an optimisation that causes bash to stop executing the current > script and replace its process with the process being run (instead of forking). > So if we exec here, we'll never run the line below.
On 2016/08/31 18:24:26, jbudorick wrote: > On 2016/08/31 17:41:46, johnme wrote: > > 1. It assumes the build on the device is more recent than > > https://codereview.chromium.org/2160243002, but that might not be the case. > > The only scenario where this would come up would be someone using an up-to-date > checkout and an old build. That's certainly not a case we're concerned about on > the bots, and I'm not sure how often it'd come up for devs running locally. adb_chrome_public_command_line sets the same command line file used by official builds of Clank. So it's normal to use this when testing arbitrary builds of Clank, whether preinstalled, from the Play Store (on a phone where updates were refused), or sideloaded in order to test an older version or bisect builds. So writing both definitely seems safer (when flags are unexpectedly not set, it's easy to lose half a day debugging the wrong thing).
lgtm I didn't think about the official build case. https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... File build/android/adb_chrome_public_command_line (right): https://codereview.chromium.org/2294973003/diff/1/build/android/adb_chrome_pu... build/android/adb_chrome_public_command_line:20: $(dirname $0)/adb_command_line.py --device-path \ On 2016/08/31 17:41:46, johnme wrote: > On 2016/08/31 15:10:23, jbudorick wrote: > > Should this be exec $(dirname $0)/adb_command_line.py like the line below? > > No, exec is an optimisation that causes bash to stop executing the current > script and replace its process with the process being run (instead of forking). > So if we exec here, we'll never run the line below. I'm betraying my limited knowledge of bash, it seems. sgtm.
I think this should be better fixed within the python script, using the flag changer. But as a quick short term solution lgtm.
The CQ bit was checked by johnme@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== adb_chrome_public_command_line should write to old and new paths In Android N, Chrome can no longer read /data/local/chrome-command-line due to SELinux (https://crbug.com/593720). Hence command line arguments are read from /data/local/tmp/chrome-command-line instead on userdebug or eng builds. This patch makes adb_chrome_public_command_line write the same arguments to both the old and the new location, so it works with both old and new builds of Chrome. BUG=593720 ========== to ========== adb_chrome_public_command_line should write to old and new paths In Android N, Chrome can no longer read /data/local/chrome-command-line due to SELinux (https://crbug.com/593720). Hence command line arguments are read from /data/local/tmp/chrome-command-line instead on userdebug or eng builds. This patch makes adb_chrome_public_command_line write the same arguments to both the old and the new location, so it works with both old and new builds of Chrome. BUG=593720 Committed: https://crrev.com/f1b179cce6d03443a0e2252a4bedcb49061099d3 Cr-Commit-Position: refs/heads/master@{#416523} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/f1b179cce6d03443a0e2252a4bedcb49061099d3 Cr-Commit-Position: refs/heads/master@{#416523} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
