[url] Reserve output buffer for various URL parsing paths

url/url_canon_relative.cc

Index: url/url_canon_relative.cc
diff --git a/url/url_canon_relative.cc b/url/url_canon_relative.cc
index e34ea2fa2495dc6acbb301644dca4b04b3738762..73833f7e875a31de75684c1bd45f990682c6d8a4 100644
--- a/url/url_canon_relative.cc
+++ b/url/url_canon_relative.cc
@@ -4,6 +4,8 @@
 
 // Canonicalizer functions for working with and resolving relative URLs.
 
+#include <algorithm>
+
 #include "base/logging.h"
 #include "url/url_canon.h"
 #include "url/url_canon_internal.h"
@@ -264,7 +266,7 @@ int CopyBaseDriveSpecIfNecessary(const char* base_url,
 #endif  // WIN32
 
 // A subroutine of DoResolveRelativeURL, this resolves the URL knowning that
-// the input is a relative path or less (qyuery or ref).
+// the input is a relative path or less (query or ref).
 template<typename CHAR>
 bool DoResolveRelativePath(const char* base_url,
                            const Parsed& base_parsed,
@@ -280,7 +282,13 @@ bool DoResolveRelativePath(const char* base_url,
   // also know we have a path so can copy up to there.
   Component path, query, ref;
   ParsePathInternal(relative_url, relative_component, &path, &query, &ref);
-  // Canonical URLs always have a path, so we can use that offset.
+
+  // Canonical URLs always have a path, so we can use that offset. Reserve
+  // enough room for the base URL, the new path, and some extra bytes for
+  // possible escaped characters.
+  output->ReserveSizeIfNeeded(
+      base_parsed.path.begin +
+      std::max(path.end(), std::max(query.end(), ref.end())) + 32);

[brettw, 2017/01/18 21:12:27]

You copied "32" from the old code. But when we start using this for all URLs, I
think this might be unnecessarily large since Blink does so much more URL
resolving than GURL does. Typically we don't expand the URL much, if at all, and
adding 32 bytes to each one in Blink will be a lot of memory. I think we should
do 8 for each one in this change. If somebody wants to do the analysis we can
consider expanding from there.

[Charlie Harrison, 2017/01/18 21:30:38]

SGTM

   output->Append(base_url, base_parsed.path.begin);
 
   if (path.len > 0) {
@@ -394,6 +402,9 @@ bool DoResolveRelativeHost(const char* base_url,
   replacements.SetQuery(relative_url, relative_parsed.query);
   replacements.SetRef(relative_url, relative_parsed.ref);
 
+  // Length() will include the scheme size, because the underlying Parsed
+  // structure starts from |relative_component.begin|.
+  output->ReserveSizeIfNeeded(replacements.components().Length() + 32);

[brettw, 2017/01/18 21:12:27]

I don't follow this comment. In this case the input string will look like
"//gstatic.com/foo/bar.gif" and I think relative_component will normally be the
entire input string.

So I would expect to need to add
base_parsed.CountCharactersBefore(Parsed::USERNAME, false) to account for the
old scheme not being replaced.

[Charlie Harrison, 2017/01/18 21:30:38]

Just read through the code and I think you're right. For some reason when I
wrote this code I thought relative_component.begin was indexed off of the end of
the base URL.

   return ReplaceStandardURL(base_url, base_parsed, replacements,
                             query_converter, output, out_parsed);
 }