Avoid unnecessary byte-swapping in blink's SerializedScriptValue.

third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp

Index: third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
index fade5395cbe6467da81cfce877e42040d235579f..e52039d12801369b8f98071192802c2ca3fdfb84 100644
--- a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
@@ -35,6 +35,7 @@
 #include "bindings/core/v8/DOMWrapperWorld.h"
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/ScriptState.h"
+#include "bindings/core/v8/SerializationTag.h"
 #include "bindings/core/v8/SerializedScriptValueFactory.h"
 #include "bindings/core/v8/Transferables.h"
 #include "bindings/core/v8/V8ArrayBuffer.h"
@@ -89,22 +90,108 @@ PassRefPtr<SerializedScriptValue> SerializedScriptValue::create(
   return adoptRef(new SerializedScriptValue(data));
 }
 
+// Versions 16 and below (prior to April 2017) used ntohs() to byte-flip SSV

[jsbell, 2017/04/07 23:40:34]

nit: "byte flip" or "byte swap"? "swap" seems more common

[pwnall, 2017/04/08 01:09:08]

Done.
After reading "byte-swap", my brain instantly agreed that it is the name used by
pretty much every old doc I've read. Thank you!

+// data when converting it to the wire format. This was a historical accient.
+//
+// As IndexedDB stores SSVs to disk indefinitely, we still need to keep around
+// the code needed to deserialize the old format.
+inline static bool isByteSwappedWiredData(const char* data, size_t length) {
+  // TODO(pwnall): Bail early if we're on big-endian hardware. Chromium doesn't
+  // currently support big-endian hardware, and there's no header exposing
+  // endianness yet.

[jsbell, 2017/04/07 23:40:34]

ARCH_CPU_LITTLE_ENDIAN seems used in several places?

[pwnall, 2017/04/08 01:09:07]

Sadly, it seems like we're not allowed to use it in Blink?

** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
  third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
      Illegal include: "build/build_config.h"
    Because of "-build" from third_party's include_rules.

+
+  // The first SSV version without byte-flipping has two envelopes (Blink, V8),
+  // each of which is at least 2 bytes long.
+  if (length < 4)
+    return true;
+
+  if (static_cast<uint8_t>(data[0]) == VersionTag) {

[jsbell, 2017/04/07 23:40:34]

Maybe precede this with a list of the different cases we are detecting:

// v0 (byte-fliped):       [ d, t, ...] where t is a tag byte, d is first data
byte
// v1 - 16 (byte-swapped):  [ v, 0xFF, ... ] where v is the version
// v17+ (non-byte-swapped): [ 0xFF, v, ... ] where v is the version

[pwnall, 2017/04/08 01:09:07]

Done.

+    // The only case where byte-flipped data can have 0xFF in byte zero is
+    // serialization version 0. This can only happen if byte one is a tag
+    // (supported in version 0) that takes in extra data, and the first byte of
+    // extra data is 0xFF. There are 13 such tags, listed below. These tags
+    //  cannot be used as version numbers in the Blink-side SSV envelope.

[jsbell, 2017/04/07 23:40:34]

nit: remove extra leading space

[pwnall, 2017/04/08 01:09:07]

Done.

+    //
+    //  35 - 0x23 - # - ImageDataTag
+    //  64 - 0x40 - @ - SparseArrayTag
+    //  68 - 0x44 - D - DateTag
+    //  73 - 0x49 - I - Int32Tag
+    //  78 - 0x4E - N - NumberTag
+    //  82 - 0x52 - R - RegExpTag
+    //  83 - 0x53 - S - StringTag
+    //  85 - 0x55 - U - Uint32Tag
+    //  91 - 0x5B - [ - ArrayTag
+    //  98 - 0x62 - b - BlobTag
+    // 102 - 0x66 - f - FileTag
+    // 108 - 0x6C - l - FileListTag
+    // 123 - 0x7B - { - ObjectTag
+    //
+    // Why we care about version 0:
+    //
+    // IndexedDB stores values using the SSV format. Currently, IndexedDB does
+    // not do any sort of migration, so a value written with a SSV version will
+    // be stored with that version until it is removed via an update or delete.
+    //
+    // IndexedDB was shipped in Chrome 11, which was released on April 27, 2011.
+    // SSV version 1 was added in WebKit r91698, which was shipped in Chrome 14,
+    // which was released on September 16, 2011.
+
+    // Fast path until the Blink-side SSV envelope reaches version 35.
+    if (SerializedScriptValue::wireFormatVersion < 35) {
+      if (static_cast<uint8_t>(data[1]) < 35)
+        return false;
+
+      // TODO(pwnall): Add UMA metric here.
+      return true;
+    }
+
+    // Slower path that would kick in after version 35, assuming we don't remove

[jbroman, 2017/04/11 15:01:47]

This is dead code. Is it actually useful to have it here before version 35?
Maybe we should just assert that it is.

[pwnall, 2017/04/11 19:10:55]

I felt bad leaving a static_assert that'd force a future maintainer to write the
code below at some point in the future. This is completely up to you... if you
prefer, I'm happy to replace the code with a static_assert :)

+    // support for SSV version 0 by then.
+
+    static uint8_t version0Tags[] = {35, 64, 68, 73,  78,  82, 83,

[jbroman, 2017/04/11 15:01:47]

const

[pwnall, 2017/04/11 19:10:55]

Done.

+                                     85, 91, 98, 102, 108, 123};
+
+    uint8_t maybeVersion0Tag = static_cast<uint8_t>(data[1]);
+    for (size_t i = 0; i < 13; ++i) {

[jsbell, 2017/04/07 23:40:34]

ARRAY_SIZE(version0Tags)

(Also, could binary search. Probably overkill.)

[pwnall, 2017/04/08 01:09:08]

Done.
I'm afraid that binary search would be slightly worse for this small set,
because it doesn't work with branch predictors and cache prefetchers.

Although computer architecture might be quite different by 2028 :), I doubt
we'll witness the kind of miraculous RAM improvement that would make the
concerns above irrelevant.

Fun aside, TBH I expect that we'll kill version 0 support by the time this code
path actually makes it into the binary.

[jbroman, 2017/04/11 15:01:47]

This loop is equivalent to:

return std::count(std::begin(version0Tags), std::end(version0Tags), data[1]);

or

return std::find(std::begin(version0Tags), std::end(version0Tags), data[1]) !=
std::end(version0Tags);

[pwnall, 2017/04/11 19:10:55]

Done.

+      if (maybeVersion0Tag == version0Tags[i])
+        return true;
+    }
+    return false;
+  }
+
+  if (static_cast<uint8_t>(data[1]) == VersionTag) {
+    // The last SSV format that used byte-flipping was version 16. The version
+    // number is stored (before byte-flipping) after a serialization tag, which
+    // is 0xFF.
+    return static_cast<uint8_t>(data[0]) != VersionTag;
+  }
+
+  // If VersionTag isn't in any of the first two bytes, this is SSV version 0,
+  // which was byte-flipped.
+  return true;
+}
+
 PassRefPtr<SerializedScriptValue> SerializedScriptValue::create(
     const char* data,
     size_t length) {
   if (!data)
     return create();
 
-  // Decode wire data from big endian to host byte order.
   DCHECK(!(length % sizeof(UChar)));
-  size_t stringLength = length / sizeof(UChar);
-  StringBuffer<UChar> buffer(stringLength);
   const UChar* src = reinterpret_cast<const UChar*>(data);
-  UChar* dst = buffer.characters();
-  for (size_t i = 0; i < stringLength; i++)
-    dst[i] = ntohs(src[i]);
+  size_t stringLength = length / sizeof(UChar);
+
+  if (isByteSwappedWiredData(data, length)) {
+    // Decode wire data from big endian to host byte order.
+    StringBuffer<UChar> buffer(stringLength);
+    UChar* dst = buffer.characters();
+    for (size_t i = 0; i < stringLength; ++i)
+      dst[i] = ntohs(src[i]);
+
+    return adoptRef(new SerializedScriptValue(String::adopt(buffer)));
+  }
 
-  return adoptRef(new SerializedScriptValue(String::adopt(buffer)));
+  return adoptRef(new SerializedScriptValue(String(src, stringLength)));
 }
 
 SerializedScriptValue::SerializedScriptValue()
@@ -134,8 +221,7 @@ SerializedScriptValue::~SerializedScriptValue() {
 }
 
 PassRefPtr<SerializedScriptValue> SerializedScriptValue::nullValue() {
-  // UChar rather than uint8_t here to get host endian behavior.
-  static const UChar kNullData[] = {0xff09, 0x3000};
+  static const uint8_t kNullData[] = {0xff, 17, 0xff, 13, '0', 0x00};

[jsbell, 2017/04/07 23:40:34]

Maybe document what this is, while we're here?

Is it VersionTag, blink version, VersionTag, v8 version, NullTag, padding? Can
we use some of those consts here?

[pwnall, 2017/04/08 01:09:07]

Done.

kNull is not exported from V8, so I couldn't use it. After a bit of thinking, I
realized that we probably don't want to use V8's constants directly anyway, and
I documented why that's the case.

   return create(reinterpret_cast<const char*>(kNullData), sizeof(kNullData));
 }
 
@@ -152,22 +238,17 @@ String SerializedScriptValue::toWireString() const {
   return wireString;
 }
 
-// Convert serialized string to big endian wire data.
 void SerializedScriptValue::toWireBytes(Vector<char>& result) const {
   DCHECK(result.isEmpty());
 
-  size_t wireSizeBytes = (m_dataBufferSize + 1) & ~1;
-  result.resize(wireSizeBytes);
-
-  const UChar* src = reinterpret_cast<UChar*>(m_dataBuffer.get());
-  UChar* dst = reinterpret_cast<UChar*>(result.data());
-  for (size_t i = 0; i < m_dataBufferSize / 2; i++)
-    dst[i] = htons(src[i]);
+  size_t resultSize = (m_dataBufferSize + 1) & ~1;

[jsbell, 2017/04/07 23:40:34]

How feasible is follow-on work to not expose the results via String and thus
require padding to an even size?

[pwnall, 2017/04/08 01:09:08]

Seems fairly feasible to me. There aren't a lot of calls to toWireString(). I'd
like to do the follow-up, mostly because I'm hoping it opens up the way to
exposing the SSV buffer and avoiding a copy altogether. 

+  result.resize(resultSize);
+  memcpy(result.data(), m_dataBuffer.get(), m_dataBufferSize);
 
-  // This is equivalent to swapping the byte order of the two bytes (x, 0),
-  // depending on endianness.
-  if (m_dataBufferSize % 2)
-    dst[wireSizeBytes / 2 - 1] = m_dataBuffer[m_dataBufferSize - 1] << 8;
+  if (resultSize > m_dataBufferSize) {
+    DCHECK_EQ(resultSize, m_dataBufferSize + 1);
+    result[m_dataBufferSize] = 0;
+  }
 }
 
 static void accumulateArrayBuffersForAllWorlds(