blob hacking webcore style

Source/bindings/v8/SerializedScriptValue.cpp

Index: Source/bindings/v8/SerializedScriptValue.cpp
diff --git a/Source/bindings/v8/SerializedScriptValue.cpp b/Source/bindings/v8/SerializedScriptValue.cpp
index 8f9d608ca6af9dff052d5fc461b28a319da75e2e..6f2608879eb365926114357d672d2039a27984c3 100644
--- a/Source/bindings/v8/SerializedScriptValue.cpp
+++ b/Source/bindings/v8/SerializedScriptValue.cpp
@@ -241,7 +241,8 @@ static bool shouldCheckForCycles(int depth)
 
 // Increment this for each incompatible change to the wire format.
 // Version 2: Added StringUCharTag for UChar v8 strings.
-static const uint32_t wireFormatVersion = 2;
+// Version 3: Use blob uuids instead of urls.
+static const uint32_t wireFormatVersion = 3;

[jsbell, 2013/09/26 18:00:36]

As an aside, we have a copy of this value living over in Chromium-land to detect
version skew from when we moved the IDB back-end.

We'll need to bump it there before this lands or, more correctly, expose this in
the Blink API somehow and consume it there.

[michaeln, 2013/09/30 20:50:24]

i'll sync up with the changes to fix that (thnx for those changes)

 
 static const int maxDepth = 20000;
 
@@ -399,10 +400,10 @@ public:
         doWriteNumber(number);
     }
 
-    void writeBlob(const String& url, const String& type, unsigned long long size)
+    void writeBlob(const String& uuid, const String& type, unsigned long long size)
     {
         append(BlobTag);
-        doWriteWebCoreString(url);
+        doWriteWebCoreString(uuid);
         doWriteWebCoreString(type);
         doWriteUint64(size);
     }
@@ -415,12 +416,10 @@ public:
         doWriteWebCoreString(url);
     }
 
-    void writeFile(const String& path, const String& url, const String& type)
+    void writeFile(const File& file)
     {
         append(FileTag);
-        doWriteWebCoreString(path);
-        doWriteWebCoreString(url);
-        doWriteWebCoreString(type);
+        doWriteFile(file);
     }
 
     void writeFileList(const FileList& fileList)
@@ -428,11 +427,8 @@ public:
         append(FileListTag);
         uint32_t length = fileList.length();
         doWriteUint32(length);
-        for (unsigned i = 0; i < length; ++i) {
-            doWriteWebCoreString(fileList.item(i)->path());
-            doWriteWebCoreString(fileList.item(i)->url().string());
-            doWriteWebCoreString(fileList.item(i)->type());
-        }
+        for (unsigned i = 0; i < length; ++i)
+            doWriteFile(*fileList.item(i));
     }
 
     void writeArrayBuffer(const ArrayBuffer& arrayBuffer)
@@ -567,6 +563,13 @@ public:
     v8::Isolate* getIsolate() { return m_isolate; }
 
 private:
+    void doWriteFile(const File& file)
+    {
+        doWriteWebCoreString(file.path());
+        doWriteWebCoreString(file.uuid());
+        doWriteWebCoreString(file.type());
+    }
+
     void doWriteArrayBuffer(const ArrayBuffer& arrayBuffer)
     {
         uint32_t byteLength = arrayBuffer.byteLength();
@@ -706,14 +709,14 @@ public:
         JSFailure
     };
 
-    Serializer(Writer& writer, MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, Vector<String>& blobURLs, v8::TryCatch& tryCatch, v8::Isolate* isolate)
+    Serializer(Writer& writer, MessagePortArray* messagePorts, ArrayBufferArray* arrayBuffers, BlobDataHandleMap& blobDataHandles, v8::TryCatch& tryCatch, v8::Isolate* isolate)
         : m_writer(writer)
         , m_tryCatch(tryCatch)
         , m_depth(0)
         , m_execDepth(0)
         , m_status(Success)
         , m_nextObjectReference(0)
-        , m_blobURLs(blobURLs)
+        , m_blobDataHandles(blobDataHandles)
         , m_isolate(isolate)
     {
         ASSERT(!tryCatch.HasCaught());
@@ -1081,8 +1084,8 @@ private:
         Blob* blob = V8Blob::toNative(value.As<v8::Object>());
         if (!blob)
             return;
-        m_writer.writeBlob(blob->url().string(), blob->type(), blob->size());
-        m_blobURLs.append(blob->url().string());
+        m_writer.writeBlob(blob->uuid(), blob->type(), blob->size());
+        m_blobDataHandles.add(blob->uuid(), blob->blobDataHandle());
     }
 
     StateBase* writeDOMFileSystem(v8::Handle<v8::Value> value, StateBase* next)
@@ -1101,8 +1104,8 @@ private:
         File* file = V8File::toNative(value.As<v8::Object>());
         if (!file)
             return;
-        m_writer.writeFile(file->path(), file->url().string(), file->type());
-        m_blobURLs.append(file->url().string());
+        m_writer.writeFile(*file);
+        m_blobDataHandles.add(file->uuid(), file->blobDataHandle());
     }
 
     void writeFileList(v8::Handle<v8::Value> value)
@@ -1113,7 +1116,7 @@ private:
         m_writer.writeFileList(*fileList);
         unsigned length = fileList->length();
         for (unsigned i = 0; i < length; ++i)
-            m_blobURLs.append(fileList->item(i)->url().string());
+            m_blobDataHandles.add(fileList->item(i)->uuid(), fileList->item(i)->blobDataHandle());
     }
 
     void writeImageData(v8::Handle<v8::Value> value)
@@ -1232,7 +1235,7 @@ private:
     ObjectPool m_transferredMessagePorts;
     ObjectPool m_transferredArrayBuffers;
     uint32_t m_nextObjectReference;
-    Vector<String>& m_blobURLs;
+    BlobDataHandleMap& m_blobDataHandles;
     v8::Isolate* m_isolate;
 };
 
@@ -1342,12 +1345,13 @@ public:
 // restoring information about saved objects of composite types.
 class Reader {
 public:
-    Reader(const uint8_t* buffer, int length, v8::Isolate* isolate)
+    Reader(const uint8_t* buffer, int length, v8::Isolate* isolate,  const BlobDataHandleMap& blobDataHandles)
         : m_buffer(buffer)
         , m_length(length)
         , m_position(0)
         , m_version(0)
         , m_isolate(isolate)
+        , m_blobDataHandles(blobDataHandles)
     {
         ASSERT(!(reinterpret_cast<size_t>(buffer) & 1));
         ASSERT(length >= 0);
@@ -1857,17 +1861,19 @@ private:
 
     bool readBlob(v8::Handle<v8::Value>* value)
     {
-        String url;
+        if (m_version < 3)
+            return false;

[kinuko, 2013/09/30 11:31:53]

so it's assuming we should always get the same version of message unless it's
from wired, correct?

[michaeln, 2013/09/30 20:50:24]

deserialization will fail if we get an older format that contains an old style
blob url

+        String uuid;
         String type;
         uint64_t size;
-        if (!readWebCoreString(&url))
+        if (!readWebCoreString(&uuid))
             return false;
         if (!readWebCoreString(&type))
             return false;
         if (!doReadUint64(&size))
             return false;
-        PassRefPtr<Blob> blob = Blob::create(KURL(ParsedURLString, url), type, size);
-        *value = toV8(blob, v8::Handle<v8::Object>(), m_isolate);
+        RefPtr<Blob> blob = Blob::create(getOrCreateBlobDataHandle(uuid, type, size));
+        *value = toV8(blob.release(), v8::Handle<v8::Object>(), m_isolate);
         return true;
     }
 
@@ -1889,42 +1895,43 @@ private:
 
     bool readFile(v8::Handle<v8::Value>* value)
     {
-        String path;
-        String url;
-        String type;
-        if (!readWebCoreString(&path))
-            return false;
-        if (!readWebCoreString(&url))
-            return false;
-        if (!readWebCoreString(&type))
+        RefPtr<File> file = doReadFileHelper();
+        if (!file)
             return false;
-        PassRefPtr<File> file = File::create(path, KURL(ParsedURLString, url), type);
-        *value = toV8(file, v8::Handle<v8::Object>(), m_isolate);
+        *value = toV8(file.release(), v8::Handle<v8::Object>(), m_isolate);
         return true;
     }
 
     bool readFileList(v8::Handle<v8::Value>* value)
     {
+        if (m_version < 3)
+            return false;
         uint32_t length;
         if (!doReadUint32(&length))
             return false;
-        PassRefPtr<FileList> fileList = FileList::create();
-        for (unsigned i = 0; i < length; ++i) {
-            String path;
-            String urlString;
-            String type;
-            if (!readWebCoreString(&path))
-                return false;
-            if (!readWebCoreString(&urlString))
-                return false;
-            if (!readWebCoreString(&type))
-                return false;
-            fileList->append(File::create(path, KURL(ParsedURLString, urlString), type));
-        }
-        *value = toV8(fileList, v8::Handle<v8::Object>(), m_isolate);
+        RefPtr<FileList> fileList = FileList::create();
+        for (unsigned i = 0; i < length; ++i)
+            fileList->append(doReadFileHelper());
+        *value = toV8(fileList.release(), v8::Handle<v8::Object>(), m_isolate);
         return true;
     }
 
+    PassRefPtr<File> doReadFileHelper()
+    {
+        if (m_version < 3)
+            return 0;
+        String path;
+        String uuid;
+        String type;
+        if (!readWebCoreString(&path)) 
+            return 0;
+        if (!readWebCoreString(&uuid))
+            return 0;
+        if (!readWebCoreString(&type))
+            return 0;
+        return File::create(path, getOrCreateBlobDataHandle(uuid, type));
+    }
+
     template<class T>
     bool doReadUintHelper(T* value)
     {
@@ -1961,11 +1968,32 @@ private:
         return true;
     }
 
+    PassRefPtr<BlobDataHandle> getOrCreateBlobDataHandle(const String& uuid, const String& type, long long size = -1)
+    {
+        // The containing ssv may have a BDH for this uuid if this ssv is just being
+        // passed from main to worker thread (for example). We use those values when creating
+        // the new blob instead of cons'ing up a new BDH.
+        //
+        // FIXME: Maybe we should require that it work that way where the ssv must have a BDH for any

[kinuko, 2013/09/30 11:31:53]

Could we include the bug number to refer this FIXME in the comment? (not fully
sure if we do this too in blink code though)

[michaeln, 2013/09/30 20:50:24]

Do you think it's a good idea to do whats described here?

+        // blobs it comes across during deserialization. Would require callers to explicitly populate
+        // the collection of BDH's for blobs to work, which would encourage lifetimes to be considered
+        // when passing ssv's around cross process. At present, we get 'lucky' in some cases because
+        // the blob in the src process happens to still exists at the time the dest process is deserializing.

[kinuko, 2013/09/30 11:31:53]

nit: exists -> exist

[michaeln, 2013/09/30 20:50:24]

done (or will do on next upload)

+        // For example in sharedWorker.postMesssage(...).
+        BlobDataHandleMap::const_iterator it = m_blobDataHandles.find(uuid);
+        if (it != m_blobDataHandles.end()) {
+            // make assertions about type and size?
+            return it->value;
+        }
+        return BlobDataHandle::create(uuid, type, size);
+    }
+
     const uint8_t* m_buffer;
     const unsigned m_length;
     unsigned m_position;
     uint32_t m_version;
     v8::Isolate* m_isolate;
+    const BlobDataHandleMap& m_blobDataHandles;
 };
 
 
@@ -2434,7 +2462,7 @@ SerializedScriptValue::SerializedScriptValue(v8::Handle<v8::Value> value, Messag
     Serializer::Status status;
     {
         v8::TryCatch tryCatch;
-        Serializer serializer(writer, messagePorts, arrayBuffers, m_blobURLs, tryCatch, isolate);
+        Serializer serializer(writer, messagePorts, arrayBuffers, m_blobDataHandles, tryCatch, isolate);
         status = serializer.serialize(value);
         if (status == Serializer::JSException) {
             didThrow = true;
@@ -2498,7 +2526,7 @@ v8::Handle<v8::Value> SerializedScriptValue::deserialize(v8::Isolate* isolate, M
     // storage. Instead, it should use SharedBuffer or Vector<uint8_t>. The
     // information stored in m_data isn't even encoded in UTF-16. Instead,
     // unicode characters are encoded as UTF-8 with two code units per UChar.
-    Reader reader(reinterpret_cast<const uint8_t*>(m_data.impl()->characters16()), 2 * m_data.length(), isolate);
+    Reader reader(reinterpret_cast<const uint8_t*>(m_data.impl()->characters16()), 2 * m_data.length(), isolate, m_blobDataHandles);
     Deserializer deserializer(reader, messagePorts, m_arrayBufferContentsArray.get());
 
     // deserialize() can run arbitrary script (e.g., setters), which could result in |this| being destroyed.