Accurate transfer of SerializedScriptValue allocation costs.

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 428 matching lines @@
 
       if (isNeuterable) {
         for (const auto& bufferHandle : bufferHandles)
           bufferHandle->Neuter();
       }
     }
   }
   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;
   }
 }
 
 void SerializedScriptValue::registerMemoryAllocatedWithCurrentScriptContext() {
   if (m_externallyAllocatedMemory)
     return;
 
   m_externallyAllocatedMemory = dataLengthInBytes();
   int64_t diff = static_cast<int64_t>(m_externallyAllocatedMemory);
   DCHECK_GE(diff, 0);
   v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(diff);
   if (m_adjustTransferableExternalAllocationOnContextTransfer) {
     DCHECK(m_arrayBufferContentsArray);
     for (size_t i = 0; i < m_arrayBufferContentsArray->size(); ++i) {
       WTF::ArrayBufferContents& buffer = m_arrayBufferContentsArray->at(i);
       buffer.adjustExternalAllocatedMemoryUponContextTransfer(
           WTF::ArrayBufferContents::Enter);
     }
     m_adjustTransferableExternalAllocationOnContextTransfer = false;
   }
 }
 
 }  // namespace blink