Reland "Limit Windows-shell-based launches to one argument" and "Replace --single-argument= with --single-argument"
This reverts commit 2538167d3945fcf80201e8e4ec480d365fba4a02.
Reason for revert: This relands the two reverted CLs.
The original change to the command-line syntax (landed in two separate
CLs) caused the following issues:
* Temporary breakage after background update from `chrome.exe %1`
syntax to `chrome.exe --single-argument=%1` (crbug.com/1092913),
when a file is opened via the shell:
* Registry contains `--single-argument=%1`, running browser sees no
argument (only an unrecognized switch), opens new tab page
* Crashes after attempted fix that replaced `--single-argument=%1`
with `--single-argument %1` due to a CHECK in the first change
enforcing the presence of the `=` character:
* After background update, registry contains `--single-argument %1`,
running browser CHECKs for `=` and crashes (crbug.com/1096004)
* Windows 7 Dev channel (expecting `=`) default-browser check parses
Canary's registry command line (containing ` `) to get the default
browser path (crbug.com/1096964), triggering CHECK
* Crash when unregistering PWA file handlers on local Chromium build
(crbug.com/1096004#c3), presumed to be caused by parsing the
shell/open command written by file-handler registration on an
older version (as local Chromium builds don't background update)
What all the above issues have in common is that they are caused by
incompatibility between new and old versions. With this in mind, this
reland assumes that the old and new syntax can and will be mixed in
potentially unexpected ways.
This change replaces `chrome.exe %1` with
`chrome.exe --single-argument %1`, which is flexible enough to handle
the following cases:
* If registry contains old syntax and running browser expects new
syntax:
* Due to the absence of the `--single-argument` switch, the command
line will be parsed normally (potentially as multiple arguments)
* If registry contains new syntax and running browser expects old
syntax:
* The browser will ignore the unrecognized `--single-argument`
switch and parse the command line normally (as the argument still
appears after a space like in the current syntax)
* If browser parses command line from another channel with different
syntax:
* Covered by either the first or second case above
This reland also replaces the single remaining CHECK with a DCHECK.
Original change's description:
> Revert "Limit Windows-shell-based launches to one argument" and "Replace --single-argument= with --single-argument"
>
> This reverts commits 452f8315641a0aa502460e7fdc1752edcd6a73c8
> and 74ae85ac6ec12a198c9cf78b71b2e6328a543084.
>
> Reason for revert: this CL and crrev.com/c/2238270, which introduced
> the --single-argument flag to the Windows command line, have caused
> issues (crbug.com/1092913, crbug.com/1096004, and crbug.com/1096964)
> related to incompatibility between the Chrome command line in the
> registry and that expected by the running browser. crrev.com/c/2238270
> was an attempt to fix those issues, but outstanding bug
> crbug.com/1096964 is still not well-understood and has made it to the
> Dev channel. This change reverts both CLs to prevent further issues
> and to enable a future reland that incorporates lessons learned.
> Reverting both CLs simultaneously is necessary to prevent trybot
> failures due to the same registry-browser command-line incompatibility
> issues (i.e., browser-test trybots having the current command-line
> syntax "chrome.exe --single-argument %1" in their registry, and
> failing to recognize the argument in the between-changes syntax
> "chrome.exe --single-argument=%1").
>
> Original change's description:
> > Limit Windows-shell-based launches to one argument
> >
> > This change adds "--single-argument" to launches done via the Windows
> > shell, which makes Chrome treat all text after "--single-argument=" as
> > Chrome's one and only argument. This limits shell-based launches to
> > passing only one argument to Chrome.
> >
> > Previously, Chrome's command line as registered with the Windows shell
> > was `chrome.exe "%1"`, %1 being Windows' filename placeholder. The
> > shell replaces this placeholder with the file/URL that Chrome has been
> > invoked on (e.g., if the link "https://www.chromium.org" were clicked,
> > Chrome would be run with command line
> > `chrome.exe "https://www.chromium.org"`.
> >
> > With this change, Chrome's command line is
> > `chrome.exe --single-argument=%1`, and the contents of %1 are treated
> > as a single argument regardless of quotes or spacing.
> >
> > Code that creates the command line string for the Windows shell (e.g.
> > code writing Chrome's command line to the registry) must use the new
> > format by calling GetCommandLineStringForShell(), which appends
> > "--single-argument=%1" to the returned string.
> >
> > Bug: 937179
> > Change-Id: I6c0d6f0abce7a8c9f65ca8b90d15438310db7c92
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2116596
> > Commit-Queue: Jesse McKenna <[email protected]>
> > Reviewed-by: Lei Zhang <[email protected]>
> > Reviewed-by: Greg Thompson <[email protected]>
> > Cr-Commit-Position: refs/heads/master@{#773398}
>
> [email protected],[email protected],[email protected]
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 937179
> Change-Id: I014cd0b1acb5080b16b68268ea8d20eb18f9b431
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2255138
> Commit-Queue: Jesse McKenna <[email protected]>
> Reviewed-by: Jesse McKenna <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#780559}
[email protected],[email protected],[email protected]
# Not skipping CQ checks because original CL landed > 1 day ago.
Bug: 937179
Change-Id: I3d83b82bddaf1a40235273bce94541c63322cc7a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2273598
Reviewed-by: Greg Thompson <[email protected]>
Reviewed-by: Lei Zhang <[email protected]>
Reviewed-by: Jesse McKenna <[email protected]>
Commit-Queue: Jesse McKenna <[email protected]>
Cr-Commit-Position: refs/heads/master@{#789641}
diff --git a/base/command_line_unittest.cc b/base/command_line_unittest.cc
index 7285451..3077edd 100644
--- a/base/command_line_unittest.cc
+++ b/base/command_line_unittest.cc
@@ -195,27 +195,23 @@
static const char kSecondArgName[] = "arg2";
static const char kThirdArgName[] = "arg with space";
static const char kFourthArgName[] = "nospace";
- static const char kFifthArgName[] = "%1";
CommandLine cl(CommandLine::NO_PROGRAM);
cl.AppendSwitchPath(kFirstArgName, FilePath(kPath1));
cl.AppendSwitchPath(kSecondArgName, FilePath(kPath2));
cl.AppendArg(kThirdArgName);
cl.AppendArg(kFourthArgName);
- cl.AppendArg(kFifthArgName);
#if defined(OS_WIN)
CommandLine::StringType expected_first_arg(UTF8ToWide(kFirstArgName));
CommandLine::StringType expected_second_arg(UTF8ToWide(kSecondArgName));
CommandLine::StringType expected_third_arg(UTF8ToWide(kThirdArgName));
CommandLine::StringType expected_fourth_arg(UTF8ToWide(kFourthArgName));
- CommandLine::StringType expected_fifth_arg(UTF8ToWide(kFifthArgName));
#elif defined(OS_POSIX) || defined(OS_FUCHSIA)
CommandLine::StringType expected_first_arg(kFirstArgName);
CommandLine::StringType expected_second_arg(kSecondArgName);
CommandLine::StringType expected_third_arg(kThirdArgName);
CommandLine::StringType expected_fourth_arg(kFourthArgName);
- CommandLine::StringType expected_fifth_arg(kFifthArgName);
#endif
#if defined(OS_WIN)
@@ -243,21 +239,8 @@
.append(expected_third_arg)
.append(QUOTE_ON_WIN)
.append(FILE_PATH_LITERAL(" "))
- .append(expected_fourth_arg)
- .append(FILE_PATH_LITERAL(" "));
-
- CommandLine::StringType expected_str_no_quote_placeholders(expected_str);
- expected_str_no_quote_placeholders.append(expected_fifth_arg);
- EXPECT_EQ(expected_str_no_quote_placeholders, cl.GetArgumentsString());
-
-#if defined(OS_WIN)
- CommandLine::StringType expected_str_quote_placeholders(expected_str);
- expected_str_quote_placeholders.append(QUOTE_ON_WIN)
- .append(expected_fifth_arg)
- .append(QUOTE_ON_WIN);
- EXPECT_EQ(expected_str_quote_placeholders,
- cl.GetArgumentsStringWithPlaceholders());
-#endif
+ .append(expected_fourth_arg);
+ EXPECT_EQ(expected_str, cl.GetArgumentsString());
}
// Test methods for appending switches to a command line.
@@ -329,6 +312,16 @@
EXPECT_EQ(FILE_PATH_LITERAL("--arg2"), cl_argv[5]);
}
+#if defined(OS_WIN)
+TEST(CommandLineTest, GetCommandLineStringForShell) {
+ CommandLine cl = CommandLine::FromString(
+ FILE_PATH_LITERAL("program --switch /switch2 --"));
+ EXPECT_EQ(
+ cl.GetCommandLineStringForShell(),
+ FILE_PATH_LITERAL("program --switch /switch2 -- --single-argument %1"));
+}
+#endif // defined(OS_WIN)
+
// Tests that when AppendArguments is called that the program is set correctly
// on the target CommandLine object and the switches from the source
// CommandLine are added to the target.
@@ -373,12 +366,6 @@
// Check that quotes are added to command line string paths containing spaces.
CommandLine::StringType cmd_string(cl_program_path.GetCommandLineString());
EXPECT_EQ(L"\"Program Path\"", cmd_string);
-
- // Check the optional quoting of placeholders in programs.
- CommandLine cl_quote_placeholder(FilePath(L"%1"));
- EXPECT_EQ(L"%1", cl_quote_placeholder.GetCommandLineString());
- EXPECT_EQ(L"\"%1\"",
- cl_quote_placeholder.GetCommandLineStringWithPlaceholders());
}
#endif
@@ -577,4 +564,26 @@
EXPECT_EQ("two", cl.GetSwitchValueASCII("foo"));
}
+#if defined(OS_WIN)
+TEST(CommandLineTest, ParseAsSingleArgument) {
+ CommandLine cl = CommandLine::FromString(
+ FILE_PATH_LITERAL("program --switch_before arg_before "
+ "--single-argument arg with spaces \"and quotes\" \""));
+
+ EXPECT_FALSE(cl.GetCommandLineString().empty());
+ EXPECT_EQ(FilePath(FILE_PATH_LITERAL("program")), cl.GetProgram());
+ EXPECT_TRUE(cl.HasSwitch("switch_before"));
+ EXPECT_EQ(cl.GetArgs(), CommandLine::StringVector({FILE_PATH_LITERAL(
+ "arg with spaces \"and quotes\" \"")}));
+
+ CommandLine cl_without_arg =
+ CommandLine::FromString(FILE_PATH_LITERAL("program --single-argument "));
+
+ EXPECT_FALSE(cl_without_arg.GetCommandLineString().empty());
+ EXPECT_EQ(FilePath(FILE_PATH_LITERAL("program")),
+ cl_without_arg.GetProgram());
+ EXPECT_TRUE(cl_without_arg.GetArgs().empty());
+}
+#endif // defined(OS_WIN)
+
} // namespace base
