Avoid unnecessary byte-swapping in blink's SerializedScriptValue.

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

 /*
  * Copyright (C) 2010 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:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 17 matching lines @@
  * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #include "bindings/core/v8/SerializedScriptValue.h"
 
 #include <memory>
 #include "bindings/core/v8/DOMDataStore.h"
 #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"
 #include "bindings/core/v8/V8ImageBitmap.h"
 #include "bindings/core/v8/V8MessagePort.h"
 #include "bindings/core/v8/V8OffscreenCanvas.h"
 #include "bindings/core/v8/V8SharedArrayBuffer.h"
 #include "core/dom/DOMArrayBuffer.h"
 #include "core/dom/DOMSharedArrayBuffer.h"
 #include "core/dom/ExceptionCode.h"
@@ skipping 34 matching lines @@
 
 PassRefPtr<SerializedScriptValue> SerializedScriptValue::create() {
   return adoptRef(new SerializedScriptValue);
 }
 
 PassRefPtr<SerializedScriptValue> SerializedScriptValue::create(
     const String& data) {
   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)));
+  const UChar* src = reinterpret_cast<const UChar*>(data);
   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]);
 
-  return adoptRef(new SerializedScriptValue(String::adopt(buffer)));
+  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(src, stringLength)));
 }
 
 SerializedScriptValue::SerializedScriptValue()
     : m_hasRegisteredExternalAllocation(false),
       m_transferablesNeedExternalAllocationRegistration(false) {}
 
 SerializedScriptValue::SerializedScriptValue(const String& wireData)
     : m_hasRegisteredExternalAllocation(false),
       m_transferablesNeedExternalAllocationRegistration(false) {
   size_t byteLength = wireData.length() * 2;
   m_dataBuffer.reset(static_cast<uint8_t*>(WTF::Partitions::bufferMalloc(
       byteLength, "SerializedScriptValue buffer")));
   m_dataBufferSize = byteLength;
   wireData.copyTo(reinterpret_cast<UChar*>(m_dataBuffer.get()), 0,
                   wireData.length());
 }
 
 SerializedScriptValue::~SerializedScriptValue() {
   // If the allocated memory was not registered before, then this class is
   // likely used in a context other than Worker's onmessage environment and the
   // presence of current v8 context is not guaranteed. Avoid calling v8 then.
   if (m_hasRegisteredExternalAllocation) {
     ASSERT(v8::Isolate::GetCurrent());
     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
         -static_cast<int64_t>(dataLengthInBytes()));
   }
 }
 
 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));
 }
 
 String SerializedScriptValue::toWireString() const {
   // Add the padding '\0', but don't put it in |m_dataBuffer|.
   // This requires direct use of uninitialized strings, though.
   UChar* destination;
   size_t stringSizeBytes = (m_dataBufferSize + 1) & ~1;
   String wireString =
       String::createUninitialized(stringSizeBytes / 2, destination);
   memcpy(destination, m_dataBuffer.get(), m_dataBufferSize);
   if (stringSizeBytes > m_dataBufferSize)
     reinterpret_cast<char*>(destination)[stringSizeBytes - 1] = '\0';
   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);
+  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);
 
-  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]);
-
-  // 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(
     v8::Isolate* isolate,
     DOMArrayBuffer* object,
     Vector<v8::Local<v8::ArrayBuffer>, 4>& buffers) {
   Vector<RefPtr<DOMWrapperWorld>> worlds;
   DOMWrapperWorld::allWorldsInCurrentThread(worlds);
   for (const auto& world : worlds) {
     v8::Local<v8::Object> wrapper = world->domDataStore().get(object, isolate);
@@ skipping 280 matching lines @@
   // Only (re)register allocation cost for transferables if this
   // SerializedScriptValue has explicitly unregistered them before.
   if (m_arrayBufferContentsArray &&
       m_transferablesNeedExternalAllocationRegistration) {
     for (auto& buffer : *m_arrayBufferContentsArray)
       buffer.registerExternalAllocationWithCurrentContext();
   }
 }
 
 }  // namespace blink