Accurate transfer of SerializedScriptValue allocation costs.

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 d9d92bc33daabb6ea1bc86a20eb8bcd5a5367da3..0635aa33f591c749035f7e6c3e549ac75f003028 100644
--- a/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
@@ -446,28 +446,40 @@ SerializedScriptValue::transferArrayBufferContents(
   return contents;
 }
 
-void SerializedScriptValue::unregisterMemoryAllocatedByCurrentScriptContext() {
-  // If the caller is the only one holding a reference then this serialized
-  // value hasn't transferred ownership & no unregistration of allocation
-  // costs wanted.
-  if (hasOneRef() || m_adjustTransferableExternalAllocationOnContextTransfer)
-    return;
+void SerializedScriptValue::prepareForTransferringContext() {
+  // Unlikely that this is non-zero, but discount any already registered
+  // external allocation for this SerializedScriptValue payload.
   if (m_externallyAllocatedMemory) {
     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(
         -static_cast<int64_t>(m_externallyAllocatedMemory));
     m_externallyAllocatedMemory = 0;
   }
+
+  DCHECK(!m_adjustTransferableExternalAllocationOnContextTransfer);
+  // Mark value as needing re-registration of external allocation
+  // costs in its target context, as handled by
+  // |registerMemoryAllocatedWithCurrentScriptContext()|.
+  //
   // TODO: if other transferables start accounting for their external
   // allocations with V8, extend this with corresponding cases.
+  if (m_arrayBufferContentsArray)
+    m_adjustTransferableExternalAllocationOnContextTransfer = true;
+}
+
+void SerializedScriptValue::finalizeTransferringContext() {
+  // If the caller is the only one holding a reference then this serialized
+  // value hasn't transferred ownership & no unregistration of allocation
+  // costs wanted. Re-registering m_externallyAllocatedMemory with the
+  // current context isn't done, redundant work as this unused value
+  // will be finalized shortly hereafter.
+  if (hasOneRef())

[jbroman, 2017/03/10 20:08:04]

I'm not thrilled by using |hasOneRef| to detect this, because its correctness is
super subtle.

Firstly, it's brittle to code that might change who holds refs to this.

Secondly, the ref you're expecting to see here is the one held by MessageEvent.
But that object isn't strongly held alive at this point (nor are there any
pointers on the stack) in the case of RemoteDOMWindow. So really, you end up
relying on that object not being finalized before here (i.e. no conservative GC
in that interval), so that the ref isn't released yet.

That may be true today (I can't spot any places where Oilpan could GC in that
path at the moment), but I'd rather not rely on that. Could we instead have the
consumer send an explicit signal that it will take the data to another context?

(Aside: come to think of it, this looks like we might subtract the
ArrayBufferContents from the isolate externally allocated memory twice: once
here, and once when we destroy it, in the case of RemoteDOMWindow. Is that
accurate?)

+    return;
+
   if (m_arrayBufferContentsArray) {
     for (auto& buffer : *m_arrayBufferContentsArray) {
       buffer.adjustExternalAllocatedMemoryUponContextTransfer(
           WTF::ArrayBufferContents::Leave);
     }
-    // Mark value as needing re-registration of external allocation
-    // costs in its target context, as handled by
-    // |registerMemoryAllocatedWithCurrentScriptContext()|.
-    m_adjustTransferableExternalAllocationOnContextTransfer = true;
   }
 }