[KURL] Avoid re-hashing input if is already canonicalized

third_party/WebKit/Source/platform/weborigin/KURL.cpp

 /*
  * Copyright (C) 2004, 2007, 2008, 2011, 2012 Apple Inc. All rights reserved.
  * Copyright (C) 2012 Research In Motion Limited. All rights reserved.
  * Copyright (C) 2008, 2009, 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
@@ skipping 11 matching lines @@
  * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
  * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
  * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "platform/weborigin/KURL.h"
 
 #include "platform/weborigin/KnownPorts.h"
 #include "url/url_util.h"
+#include "wtf/CheckedNumeric.h"
 #include "wtf/PtrUtil.h"
 #include "wtf/StdLibExtras.h"
 #include "wtf/text/CString.h"
 #include "wtf/text/StringHash.h"
 #include "wtf/text/StringUTF8Adaptor.h"
 #include "wtf/text/TextEncoding.h"
 #include <algorithm>
+#include <limits>
 #ifndef NDEBUG
 #include <stdio.h>
 #endif
 
 namespace blink {
 
 static const int maximumValidPortNumber = 0xFFFE;
 static const int invalidPortNumber = 0xFFFF;
 
 static void assertProtocolIsGood(const char* protocol) {
@@ skipping 661 matching lines @@
   if (url.is8Bit())
     return url::FindAndCompareScheme(asURLChar8Subtle(url), url.length(),
                                      protocol, 0);
   return url::FindAndCompareScheme(url.characters16(), url.length(), protocol,
                                    0);
 }
 
 void KURL::init(const KURL& base,
                 const String& relative,
                 const WTF::TextEncoding* queryEncoding) {
-  if (!relative.isNull() && relative.is8Bit()) {
-    StringUTF8Adaptor relativeUTF8(relative);
-    init(base, relativeUTF8.data(), relativeUTF8.length(), queryEncoding);
-  } else
-    init(base, relative.characters16(), relative.length(), queryEncoding);
-  initProtocolIsInHTTPFamily();
-  initInnerURL();
-}
-
-template <typename CHAR>
-void KURL::init(const KURL& base,
-                const CHAR* relative,
-                int relativeLength,
-                const WTF::TextEncoding* queryEncoding) {
   // As a performance optimization, we do not use the charset converter
   // if encoding is UTF-8 or other Unicode encodings. Note that this is
   // per HTML5 2.5.3 (resolving URL). The URL canonicalizer will be more
   // efficient with no charset converter object because it can do UTF-8
   // internally with no extra copies.
 
+  StringUTF8Adaptor baseUTF8(base.getString());
+
   // We feel free to make the charset converter object every time since it's
   // just a wrapper around a reference.
   KURLCharsetConverter charsetConverterObject(queryEncoding);
   KURLCharsetConverter* charsetConverter =
       (!queryEncoding || isUnicodeEncoding(queryEncoding))
           ? 0
           : &charsetConverterObject;
 
-  StringUTF8Adaptor baseUTF8(base.getString());
+  // Clamp to int max to avoid overflow.
+  url::RawCanonOutputT<char> output;
+  if (!relative.isNull() && relative.is8Bit()) {
+    StringUTF8Adaptor relativeUTF8(relative);
+    CheckedNumeric<int> relativeLength(

[esprehn, 2016/11/04 22:55:59]

I think you want to use clampTo() in here instead of CheckedNumeric?

[Charlie Harrison, 2016/11/05 02:23:13]

Aha, thanks. Done.

+        CheckedNumeric<size_t>(relativeUTF8.length()));
+    m_isValid = url::ResolveRelative(
+        baseUTF8.data(), baseUTF8.length(), base.m_parsed, relativeUTF8.data(),
+        relativeLength.ValueOrDefault(std::numeric_limits<int>::max()),
+        charsetConverter, &output, &m_parsed);
+  } else {
+    CheckedNumeric<int> relativeLength(

[esprehn, 2016/11/04 22:55:59]

ditto

[Charlie Harrison, 2016/11/05 02:23:13]

Done.

+        CheckedNumeric<unsigned>(relative.length()));
+    m_isValid = url::ResolveRelative(
+        baseUTF8.data(), baseUTF8.length(), base.m_parsed,
+        relative.characters16(),
+        relativeLength.ValueOrDefault(std::numeric_limits<int>::max()),
+        charsetConverter, &output, &m_parsed);
+  }
 
-  url::RawCanonOutputT<char> output;
-  m_isValid = url::ResolveRelative(baseUTF8.data(), baseUTF8.length(),
-                                   base.m_parsed, relative, relativeLength,
-                                   charsetConverter, &output, &m_parsed);
+  // AtomicString::fromUTF8 will re-hash the raw output and check the
+  // AtomicStringTable (addWithTranslator) for the string. This can be very
+  // expensive for large URLs. However, since many URLs are generated from
+  // existing AtomicStrings (which already have their hashes computed), this
+  // fast path is used if the input string is already canonicalized.
+  //
+  // Because this optimization does not apply to non-AtomicStrings, explicitly
+  // check that the input is Atomic before moving forward with it. This also
+  // gets around the problem that marking non-Atomic input as Atomic will render
+  // the string thread unsafe.

[esprehn, 2016/11/04 22:55:59]

fwiw I don't think the thread safety is an issue, since KURL itself is not
thread safe since it uses AtomicString.

[Charlie Harrison, 2016/11/05 02:23:13]

The KURL is not thread safe, but the String passed as input could be (and needs
to be in some cases when the html parser is off-thread).

If the String is not Atomic, the code below would do something like `m_string =
AtomicString(relative)`, which would mark the underlying impl as atomic, making
the input no longer thread safe.

I tried to edit the comment to be a bit clearer here.

+  if (!relative.isNull() && relative.impl()->isAtomic() &&
+      StringView(output.data(), static_cast<unsigned>(output.length())) ==
+          relative) {
+    m_string = relative;
+  } else {
+    m_string = AtomicString::fromUTF8(output.data(), output.length());
+  }
 
-  // See FIXME in KURLPrivate in the header. If canonicalization has not
-  // changed the string, we can avoid an extra allocation by using assignment.
-  m_string = AtomicString::fromUTF8(output.data(), output.length());
+  initProtocolIsInHTTPFamily();
+  initInnerURL();
 }
 
 void KURL::initInnerURL() {
   if (!m_isValid) {
     m_innerURL.reset();
     return;
   }
   if (url::Parsed* innerParsed = m_parsed.inner_parsed())
     m_innerURL = wrapUnique(new KURL(
         ParsedURLString,
@@ skipping 100 matching lines @@
   m_parsed = newParsed;
   m_string = AtomicString::fromUTF8(output.data(), output.length());
 }
 
 bool KURL::isSafeToSendToAnotherThread() const {
   return m_string.isSafeToSendToAnotherThread() &&
          (!m_innerURL || m_innerURL->isSafeToSendToAnotherThread());
 }
 
 }  // namespace blink