Beware of print-read inconsistency when serializing GURLs.

Beware of print-read inconsistency when serializing GURLs.
BUG=165622
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173583

[Tom Sepez, 2012-12-14 21:57:01]

This is a straightforward fix to the underlying bug until we can rewrite GURL
serialization on a member by member basis.

[jam, 2012-12-14 23:00:59]

I'm not that familiar with the internals of GURL, so redirecting to brettw who
wrote it

[brettw, 2012-12-15 00:21:00]

GURL stuff LGTM. In changing the serialization of GURL, it seems like we will
need a new baseline for the perf bots since we do IPC to the checked in version?
Maybe John knows more, but this certainly used to be the case.

Longer-term we should try to fix GURL to not emit something so confusing. Any
case where it doesn't reparse the same will likely cause other problems,
although it may be impractical to do in general.

[Tom Sepez, 2012-12-15 00:26:11]

Hey John, do you know if the perf bots will break if I land this?  If so, what
do I do?

[Tom Sepez, 2012-12-17 18:36:02]

John, Brett, let me know if you like this version better where I try to maintain
backwards compatibility.  Thanks.

[jam, 2012-12-17 18:45:20]

On 2012/12/15 00:26:11, Tom Sepez wrote:
> Hey John, do you know if the perf bots will break if I land this?  If so, what
> do I do?

yes they will, because IPC wont work with the old binaries. you probably want to
avoid this since updating the builds is a huge pain (especially spending a few
days updating perf expectations)

[jam, 2012-12-17 18:45:32]

On 2012/12/17 18:36:02, Tom Sepez wrote:
> John, Brett, let me know if you like this version better where I try to
maintain
> backwards compatibility.  Thanks.

I defer to Brett and you on this.

[brettw, 2012-12-17 18:53:23]

https://codereview.chromium.org/11576038/diff/12001/content/public/common/com...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/11576038/diff/12001/content/public/common/com...
content/public/common/common_param_traits.cc:75: *p = GURL();
If we're going to do this, why not just have Write() write an empty string if
the URL is invalid and skip the flag?

We're going to do another pass where the serialization is done "properly",
right? Getting an empty GURL out the other makes me a bit worried about causing
regressions.

[Tom Sepez, 2012-12-17 18:59:35]

> If we're going to do this, why not just have Write() write an empty
> string if the URL is invalid and skip the flag?

I was under the impression that we might need to pass an invalid URL for error
reporting purposes or the such.  Passing an invalid URL isn't evil in and of
itself; if it parses invalid, then it's fine to keep the spec around for
reporting purposes.

Its just that in the underlying bug, clever folks have found a way to make the
read perform a invalid -> valid transition.  Ideally we'd keep the spec and just
mark m_isvalid false, but its a private member and there's no method for doing
this.

[brettw, 2012-12-17 19:09:56]

That's why I was asking about doing a "better" patch. There is a GURL
constructor that takes the entire state of the object:

GURL(const char* canonical_spec, size_t canonical_spec_len, const
url_parse::Parsed& parsed, bool is_valid);

which is *maybe* what we need here. The reason I originally serialized as a
string was that I was concerned that an exploited renderer could send us a
totally insane parse (embedded nulls, slashes in the hostname, etc.) that might
confuse some system library that resolves hostnames, or whatever.

Thinking out loud, maybe the ideal serialization sends everything, but valid
URLs get reparsed, and invalid URLs we just store everything as-is (hopefully
being marked invalid will prevent most of the evil stuff I was vaguely worried
about).

[Tom Sepez, 2012-12-17 19:10:19]

Turns out PS#2 won't work since it would trip the NOTREACHED() at line 24 of
ipc_message_utils_impl.h.  So the best fix is to reparse the
possibly_invalid_spec(), and send GURL() when it mismatches validity.  I don't
see how to avoid that cost for now.

[Tom Sepez, 2012-12-17 19:30:29]

Yep. In the mean time, ps#3 stops the bleeding until we can design a better
patch down the road.

[brettw, 2012-12-17 20:25:00]

lgtm

https://codereview.chromium.org/11576038/diff/28001/content/public/common/com...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/11576038/diff/28001/content/public/common/com...
content/public/common/common_param_traits.cc:58: // type as GURL.
Can you file a bug for doing said improvements and link to it here?

https://codereview.chromium.org/11576038/diff/28001/content/public/common/com...
content/public/common/common_param_traits.cc:64: m->WriteString("");
Can you do WriteString(std::string()) instead?

[Tom Sepez, 2012-12-17 20:53:38]

https://codereview.chromium.org/11576038/diff/28001/content/public/common/com...
File content/public/common/common_param_traits.cc (right):

https://codereview.chromium.org/11576038/diff/28001/content/public/common/com...
content/public/common/common_param_traits.cc:58: // type as GURL.
On 2012/12/17 20:25:00, brettw wrote:
> Can you file a bug for doing said improvements and link to it here?

Done.

https://codereview.chromium.org/11576038/diff/28001/content/public/common/com...
content/public/common/common_param_traits.cc:64: m->WriteString("");
On 2012/12/17 20:25:00, brettw wrote:
> Can you do WriteString(std::string()) instead?

Done.

[commit-bot: I haz the power, 2012-12-17 21:00:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tsepez@chromium.org/11576038/25002

[commit-bot: I haz the power, 2012-12-18 00:01:52]

Change committed as 173583