url::Origin::Create("chrome://version").GetURL() hits a DCHECK in GURL::InitializeFromCanonicalSpec() |
|||
Issue description
REPRO: Add the following 2 tests:
TEST(SchemeHostPortTest, DcheckInGurl125) {
url::AddStandardScheme("chrome", url::SCHEME_WITHOUT_PORT);
url::SchemeHostPort tuple("chrome", "", 0);
ExpectParsedUrlsEqual(GURL(tuple.Serialize()), tuple.GetURL());
}
TEST(SchemeHostPortTest, DcheckInGurl125) {
url::AddStandardScheme("chrome", url::SCHEME_WITHOUT_PORT);
url::SchemeHostPort tuple("chrome", "", 0);
ExpectParsedUrlsEqual(GURL(tuple.Serialize()), tuple.GetURL());
}
EXPECTED: No DCHECKs are hit when running the tests
ACTUAL: A DCHECK is hit here:
gurl.cc(125)] Check failed: test_url.is_valid_ == is_valid_ (0 vs. 1)
#2 logging::LogMessage::~LogMessage()
#3 GURL::InitializeFromCanonicalSpec()
#4 GURL::GURL()
#5 url::SchemeHostPort::GetURL()
#6 url::Origin::GetURL()
NOTES:
- This bug is a follow-up for issue 820070
- The call to url::AddStandardScheme("chrome", url::SCHEME_WITHOUT_PORT)
pretty much replicates what would have happened when running |unit_tests|.
The DCHECK doesn't happen without this AddStandardScheme call.
- The DCHECK in GURL::InitializeFromCanonicalSpec tries to verify that
GURL test_url(spec_, ...) will parse into the same GURL as |this|.
FWIW, right before the DCHECK failure |spec_| is "chrome:///"
,
Mar 8 2018
mkwst@, can you PTAL (since you are listed in //url/OWNERS)
,
Mar 8 2018
,
Mar 8 2018
Yep this seems like a bug. From reading through the code I think there are two solutions: 1. Replace "true" with an emptiness check for the host. 2. Do an emptiness check for the host in either IsValidInput or IsCanonicalHost in SchemeHostPort. Note that IsValidInput *does* do an emptiness check already for SCHEME_WITH_PORT. I don't see why we don't do so for SCHEME_WITHOUT_PORT.
,
Mar 8 2018
Oh sorry comment #4 is wrong and omits e.g. file URLs which have an optional authority component.
,
Mar 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/60f0c536591d5b64febafb5f3c5112ca33c691a4 commit 60f0c536591d5b64febafb5f3c5112ca33c691a4 Author: Charles Harrison <csharrison@chromium.org> Date: Mon Mar 12 17:30:55 2018 [url] For empty hosts, just trust GURL to make the invalid/valid decision The SchemeHostPort always considers tuples with SCHEME_WITHOUT_PORT to have empty hosts without being invalid (to cater to cases like file schemes which can have hosts or not). This led to a mismatch in SchemeHostPost::GetURL where we always considered URLs with empty hosts to be valid GURLs, but this is not an assumption that holds true. This CL just punts the valid decision in this case to GURL for re-parsing, since it should be a rare occurance. Bug: 820194 Change-Id: I3f4441d8c71a9b9500c71dbe98f0769f42ad13b5 Reviewed-on: https://chromium-review.googlesource.com/956424 Reviewed-by: Mike West <mkwst@chromium.org> Commit-Queue: Charlie Harrison <csharrison@chromium.org> Cr-Commit-Position: refs/heads/master@{#542527} [modify] https://crrev.com/60f0c536591d5b64febafb5f3c5112ca33c691a4/url/scheme_host_port.cc [modify] https://crrev.com/60f0c536591d5b64febafb5f3c5112ca33c691a4/url/scheme_host_port_unittest.cc
,
Mar 12 2018
Should be fixed. |
|||
►
Sign in to add a comment |
|||
Comment 1 by lukasza@chromium.org
, Mar 8 2018Maybe the problem is here: GURL SchemeHostPort::GetURL() const { url::Parsed parsed; std::string serialized = SerializeInternal(&parsed); if (IsInvalid()) return GURL(std::move(serialized), parsed, false); // If the serialized string is passed to GURL for parsing, it will append an // empty path "/". Add that here. Note: per RFC 6454 we cannot do this for // normal Origin serialization. DCHECK(!parsed.path.is_valid()); parsed.path = Component(serialized.length(), 1); serialized.append("/"); return GURL(std::move(serialized), parsed, true); <- MAYBE ALWAYS PASSING |true| AS |is_valid| ARG IS WRONG? }