Accurate transfer of SerializedScriptValue allocation costs.

Accurate transfer of SerializedScriptValue allocation costs.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another as part
of a postMessage()

The handoff 'protocol' provided there fell short in that it could
fail to subtract transferable (array buffer contents) costs in
the source context, or end up doing it twice if the postMessage()
failed. Bookkeeping confusion resulted.

Rework the mechanism by instead having ArrayBufferContents keep
track of its external allocation cost registration status, so as
to prevent double discounting. Along with that, it is both safe
and accurate to unregister all allocation costs prior to
transfer. Should the value successfully be posted to its target
context, cost will be registered there. And if not, the value will
be destructed (..but without discounting allocation cost yet again.)

R=jbroman,haraken
BUG=700353
Review-Url: https://codereview.chromium.org/2741793003
Cr-Commit-Position: refs/heads/master@{#456800}
Committed: https://chromium.googlesource.com/chromium/src/+/e75f1381dccdeef00b30479ad55432f60ee8db7d

[sof, 2017-03-10 14:53:25]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 14:53:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-03-10 15:20:58]

sigbjornf@opera.com changed reviewers:
+ haraken@chromium.org, jbroman@chromium.org

[sof, 2017-03-10 15:20:59]

please take a look.

TSan is an invaluable tool, I wish I was equally circumspect about the issues it
uncovers. Doing the transfer in two stages on the sending/source side seems
unavoidable.

[sof, 2017-03-10 15:21:18]

Description was changed from

==========
Split up SerializedScriptValue cost transfer into two stages.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another.

As that transfer is in general between threads, extra care is needed
to make that handoff of cost attribution race free, which r456009
failed to handle correctly. Address cross-thread transfer by splitting
up the transferring on the 'source' side into two: first marking the
SerializedScriptValue as being transferred and if that transfer went
ahead as wanted, finalize and discount the allocation cost from the
source context. Non-interference between the two participating threads
is ensured as a result.

In order to implement this safely, we also make the sampling of
ThreadSafeRefCounted<> ref counts atomic.

R=
BUG=700353
==========

to

==========
Split up SerializedScriptValue cost handoff into two stages.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another.

As that transfer is in general between threads, extra care is needed
to make that handoff of cost attribution race free, which r456009
failed to handle correctly. Address cross-thread transfer by splitting
up the transferring on the 'source' side into two: first marking the
SerializedScriptValue as being transferred and if that transfer went
ahead as wanted, finalize and discount the allocation cost from the
source context. Non-interference between the two participating threads
is ensured as a result.

In order to implement this safely, we also make the sampling of
ThreadSafeRefCounted<> ref counts atomic.

R=
BUG=700353
==========

[commit-bot: I haz the power, 2017-03-10 16:47:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 16:47:55]

Dry run: This issue passed the CQ dry run.

[jbroman, 2017-03-10 20:08:05]

https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:475: if
(hasOneRef())
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?)

[sof, 2017-03-10 20:50:26]

On 2017/03/10 20:08:05, jbroman wrote:
>
https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> (right):
> 
>
https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:475: if
> (hasOneRef())
> 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.
> 

The postMessage() implementation will have to do a ref() on the value to keep it
alive. That we then get to construct a MessageEvent (and register allocation
cost by doing so), and subsequently schedule a post message task, run that task
and GC it before the postMessage() returns..I don't quite see how could come
about.

> 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?
> 

A return value (from postMessage()) or overriding ref() are two ways, will have
a look. I wouldn't mind dropping the use of hasOneRef().

> (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?)

I don't see it; once owned by MessageEvent, the SerializedScriptValue will have
registered the allocation with the target context & subtraction will be done
relative to it.

[jbroman, 2017-03-10 21:05:45]

On 2017/03/10 at 20:50:26, sigbjornf wrote:
> On 2017/03/10 20:08:05, jbroman wrote:
> >
https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> > (right):
> > 
> >
https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:475: if
> > (hasOneRef())
> > 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.
> > 
> 
> The postMessage() implementation will have to do a ref() on the value to keep
it alive. That we then get to construct a MessageEvent (and register allocation
cost by doing so), and subsequently schedule a post message task, run that task
and GC it before the postMessage() returns..I don't quite see how could come
about.

Not in the case where we're sending it out of process. Then we do this path:

blink::DOMWindow::postMessage constructs MessageEvent
blink::RemoteDOMWindow::schedulePostMessage
blink::RemoteFrameClientImpl::forwardPostMessage wraps the MessageEvent in a
WebDOMMessageEvent
content::RenderFrameProxy::forwardPostMessage copies the data out of the
WebDOMMessageEvent into FrameMsg_PostMessage_Params

Once DOMWindow::postMessage returns, the MessageEvent* has no remaining
references on the stack, and content::RenderFrameProxy::forwardPostMessage never
took a reference (rather, it copied the contents into an IPC message). There is
no task posted (aside from the IPC message, but that doesn't claim a reference
to either the SerializedScriptValue or MessageEvent), so the
SerializedScriptValue reference can be dropped the first time Oilpan sees fit to
collect the MessageEvent.

> > 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?
> > 
> 
> A return value (from postMessage()) or overriding ref() are two ways, will
have a look. I wouldn't mind dropping the use of hasOneRef().
> 
> > (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?)
> 
> I don't see it; once owned by MessageEvent, the SerializedScriptValue will
have registered the allocation with the target context & subtraction will be
done relative to it.

Ah, I do think you are correct about this one.

[sof, 2017-03-11 07:55:25]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 07:55:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-03-11 07:58:53]

On 2017/03/10 21:05:45, jbroman wrote:
> On 2017/03/10 at 20:50:26, sigbjornf wrote:
> > On 2017/03/10 20:08:05, jbroman wrote:
> > >
>
https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/2741793003/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:475:
if
> > > (hasOneRef())
> > > 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.
> > > 
> > 
> > The postMessage() implementation will have to do a ref() on the value to
keep
> it alive. That we then get to construct a MessageEvent (and register
allocation
> cost by doing so), and subsequently schedule a post message task, run that
task
> and GC it before the postMessage() returns..I don't quite see how could come
> about.
> 
> Not in the case where we're sending it out of process. Then we do this path:
> 
> blink::DOMWindow::postMessage constructs MessageEvent
> blink::RemoteDOMWindow::schedulePostMessage
> blink::RemoteFrameClientImpl::forwardPostMessage wraps the MessageEvent in a
> WebDOMMessageEvent
> content::RenderFrameProxy::forwardPostMessage copies the data out of the
> WebDOMMessageEvent into FrameMsg_PostMessage_Params
> 
> Once DOMWindow::postMessage returns, the MessageEvent* has no remaining
> references on the stack, and content::RenderFrameProxy::forwardPostMessage
never
> took a reference (rather, it copied the contents into an IPC message). There
is
> no task posted (aside from the IPC message, but that doesn't claim a reference
> to either the SerializedScriptValue or MessageEvent), so the
> SerializedScriptValue reference can be dropped the first time Oilpan sees fit
to
> collect the MessageEvent.
> 

Thanks, there is a weakness there should the MessageEvent end up not being
stack-reachable from the initiating postMessage().

I've made the postMessage() implementations return a bool to indicate status
instead.

[sof, 2017-03-11 08:06:39]

Description was changed from

==========
Split up SerializedScriptValue cost handoff into two stages.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another.

As that transfer is in general between threads, extra care is needed
to make that handoff of cost attribution race free, which r456009
failed to handle correctly. Address cross-thread transfer by splitting
up the transferring on the 'source' side into two: first marking the
SerializedScriptValue as being transferred and if that transfer went
ahead as wanted, finalize and discount the allocation cost from the
source context. Non-interference between the two participating threads
is ensured as a result.

In order to implement this safely, we also make the sampling of
ThreadSafeRefCounted<> ref counts atomic.

R=
BUG=700353
==========

to

==========
Split up SerializedScriptValue cost handoff into two stages.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another.

As that transfer is in general between threads, extra care is needed
to make that handoff of cost attribution race free, which r456009
failed to handle correctly. Address cross-thread transfer by splitting
up the transferring on the 'source' side into two: first marking the
SerializedScriptValue as being transferred and if that transfer went
ahead as wanted, finalize and discount the external allocation cost from the
source context. Non-interfering access to the shared SerializedScriptValue
is accomplished as a result.

R=
BUG=700353
==========

[sof, 2017-03-11 08:54:17]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 08:54:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 11:16:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 11:16:57]

Dry run: This issue passed the CQ dry run.

[sof, 2017-03-11 15:11:12]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 15:11:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 16:24:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 16:24:42]

Dry run: This issue passed the CQ dry run.

[sof, 2017-03-13 13:19:19]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-13 13:19:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-03-13 13:29:00]

Thinking this through over the weekend, I realized that it is needlessly general
to track postMessage() success/failure to transfer -- we can just observe if the
register*() has been called upon completion of said postMessage(). Iff it has,
then we need to discount cost in our current context. I don't think there are
thread raciness to contend with when doing so.

Tracking postMessage() success/failure is flimsy for other reasons also. Anyway,
updated CL to do the above instead.

[jbroman, 2017-03-13 15:01:14]

https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:469: void
SerializedScriptValue::finalizeTransferringContext() {
Why do we believe registerMemoryAllocatedWithCurrentScriptContext will be called
before finalizeTransferringContext? In the case that we're sending to a worker,
we don't create the MessageEvent until we arrive on the worker thread, which may
not be (and in fact, probably is not) before the sending thread runs
finalizeTransferringContext.

[commit-bot: I haz the power, 2017-03-13 15:07:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-13 15:07:47]

Dry run: This issue passed the CQ dry run.

[sof, 2017-03-13 15:10:49]

https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:469: void
SerializedScriptValue::finalizeTransferringContext() {
On 2017/03/13 15:01:14, jbroman wrote:
> Why do we believe registerMemoryAllocatedWithCurrentScriptContext will be
called
> before finalizeTransferringContext? In the case that we're sending to a
worker,
> we don't create the MessageEvent until we arrive on the worker thread, which
may
> not be (and in fact, probably is not) before the sending thread runs
> finalizeTransferringContext.

I don't see how to accommodate that, so we will have to accept a cross-context
overestimate.

[jbroman, 2017-03-13 15:45:52]

On 2017/03/13 at 15:10:49, sigbjornf wrote:
>
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):
> 
>
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:469: void
SerializedScriptValue::finalizeTransferringContext() {
> On 2017/03/13 15:01:14, jbroman wrote:
> > Why do we believe registerMemoryAllocatedWithCurrentScriptContext will be
called
> > before finalizeTransferringContext? In the case that we're sending to a
worker,
> > we don't create the MessageEvent until we arrive on the worker thread, which
may
> > not be (and in fact, probably is not) before the sending thread runs
> > finalizeTransferringContext.
> 
> I don't see how to accommodate that, so we will have to accept a cross-context
overestimate.

This is how I'd imagine it ought to work (mild pseudo-code). I'm not sure how
SharedArrayBuffer should be accounted (maybe the V8 SAB folk have thought about
it), but setting that aside:

Both SerializedScriptValue and ArrayBufferContents::DataHolder have a boolean,
|m_externalMemoryRegistered|, indicating whether their memory is registered with
the current context (name doesn't matter -- we could reuse the existing name).

They both have methods:

void T::registerExternalMemory() {
  // if SerializedScriptValue, delegate to ArrayBufferContents as well
  if (m_externalMemoryRegistered)
    return;
  isolate->AdjustAmountOfExternalAllocatedMemory(externalMemoryUsage);
  m_externalMemoryRegistered = true;
}

void T::unregisterExternalMemory() {
  // if SerializedScriptValue, delegate to ArrayBufferContents as well
  if (!m_externalMemoryRegistered)
    return;
  isolate->AdjustAmountOfExternalAllocatedMemory(-externalMemoryUsage);
  m_externalMemoryRegistered = false;
}

They each invoke unregisterExternalMemory in their destructor, ensuring that any
adjustment they make is eventually cancelled. When we intend to transfer to
another thread, we do:

message->unregisterExternalMemory();
target->postMessage(std::move(message), ...);

If the message is discarded, then the memory has already been unregistered, and
since that fact is stored in the boolean, the destructor won't do so again.
If the message is registered to another thread, then it will happen only after
the unregistration has already happened -- and that registration will be
balanced by the destructor.

[sof, 2017-03-13 16:04:03]

On 2017/03/13 15:45:52, jbroman wrote:
> On 2017/03/13 at 15:10:49, sigbjornf wrote:
> >
>
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:469:
void
> SerializedScriptValue::finalizeTransferringContext() {
> > On 2017/03/13 15:01:14, jbroman wrote:
> > > Why do we believe registerMemoryAllocatedWithCurrentScriptContext will be
> called
> > > before finalizeTransferringContext? In the case that we're sending to a
> worker,
> > > we don't create the MessageEvent until we arrive on the worker thread,
which
> may
> > > not be (and in fact, probably is not) before the sending thread runs
> > > finalizeTransferringContext.
> > 
> > I don't see how to accommodate that, so we will have to accept a
cross-context
> overestimate.
> 
> This is how I'd imagine it ought to work (mild pseudo-code). I'm not sure how
> SharedArrayBuffer should be accounted (maybe the V8 SAB folk have thought
about
> it), but setting that aside:
> 
> Both SerializedScriptValue and ArrayBufferContents::DataHolder have a boolean,
> |m_externalMemoryRegistered|, indicating whether their memory is registered
with
> the current context (name doesn't matter -- we could reuse the existing name).
> 
> They both have methods:
> 
> void T::registerExternalMemory() {
>   // if SerializedScriptValue, delegate to ArrayBufferContents as well
>   if (m_externalMemoryRegistered)
>     return;
>   isolate->AdjustAmountOfExternalAllocatedMemory(externalMemoryUsage);
>   m_externalMemoryRegistered = true;
> }
> 
> void T::unregisterExternalMemory() {
>   // if SerializedScriptValue, delegate to ArrayBufferContents as well
>   if (!m_externalMemoryRegistered)
>     return;
>   isolate->AdjustAmountOfExternalAllocatedMemory(-externalMemoryUsage);
>   m_externalMemoryRegistered = false;
> }
> 
> They each invoke unregisterExternalMemory in their destructor, ensuring that
any
> adjustment they make is eventually cancelled. When we intend to transfer to
> another thread, we do:
> 
> message->unregisterExternalMemory();
> target->postMessage(std::move(message), ...);
> 
> If the message is discarded, then the memory has already been unregistered,
and
> since that fact is stored in the boolean, the destructor won't do so again.
> If the message is registered to another thread, then it will happen only after
> the unregistration has already happened -- and that registration will be
> balanced by the destructor.

Won't this cause under-reporting if the message is discarded, but the
ArrayBuffer(s) are still referred to on the source side?

[jbroman, 2017-03-13 18:09:56]

On 2017/03/13 at 16:04:03, sigbjornf wrote:
> On 2017/03/13 15:45:52, jbroman wrote:
> > On 2017/03/13 at 15:10:49, sigbjornf wrote:
> > >
> >
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> > (right):
> > > 
> > >
> >
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:469:
void
> > SerializedScriptValue::finalizeTransferringContext() {
> > > On 2017/03/13 15:01:14, jbroman wrote:
> > > > Why do we believe registerMemoryAllocatedWithCurrentScriptContext will
be
> > called
> > > > before finalizeTransferringContext? In the case that we're sending to a
> > worker,
> > > > we don't create the MessageEvent until we arrive on the worker thread,
which
> > may
> > > > not be (and in fact, probably is not) before the sending thread runs
> > > > finalizeTransferringContext.
> > > 
> > > I don't see how to accommodate that, so we will have to accept a
cross-context
> > overestimate.
> > 
> > This is how I'd imagine it ought to work (mild pseudo-code). I'm not sure
how
> > SharedArrayBuffer should be accounted (maybe the V8 SAB folk have thought
about
> > it), but setting that aside:
> > 
> > Both SerializedScriptValue and ArrayBufferContents::DataHolder have a
boolean,
> > |m_externalMemoryRegistered|, indicating whether their memory is registered
with
> > the current context (name doesn't matter -- we could reuse the existing
name).
> > 
> > They both have methods:
> > 
> > void T::registerExternalMemory() {
> >   // if SerializedScriptValue, delegate to ArrayBufferContents as well
> >   if (m_externalMemoryRegistered)
> >     return;
> >   isolate->AdjustAmountOfExternalAllocatedMemory(externalMemoryUsage);
> >   m_externalMemoryRegistered = true;
> > }
> > 
> > void T::unregisterExternalMemory() {
> >   // if SerializedScriptValue, delegate to ArrayBufferContents as well
> >   if (!m_externalMemoryRegistered)
> >     return;
> >   isolate->AdjustAmountOfExternalAllocatedMemory(-externalMemoryUsage);
> >   m_externalMemoryRegistered = false;
> > }
> > 
> > They each invoke unregisterExternalMemory in their destructor, ensuring that
any
> > adjustment they make is eventually cancelled. When we intend to transfer to
> > another thread, we do:
> > 
> > message->unregisterExternalMemory();
> > target->postMessage(std::move(message), ...);
> > 
> > If the message is discarded, then the memory has already been unregistered,
and
> > since that fact is stored in the boolean, the destructor won't do so again.
> > If the message is registered to another thread, then it will happen only
after
> > the unregistration has already happened -- and that registration will be
> > balanced by the destructor.
> 
> Won't this cause under-reporting if the message is discarded, but the
ArrayBuffer(s) are still referred to on the source side?

Except for SharedArrayBuffer, the source array buffers (and their views) are
neutered if they are transferred. Not sure if there are other cases where the
source can have other references to the same contents (such things would seem to
already be a threading bug, because such access is unsychronized?).

[sof, 2017-03-14 09:05:51]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 09:06:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 09:16:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 09:16:54]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sof, 2017-03-14 09:50:14]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 09:50:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 11:14:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 11:14:16]

Dry run: This issue passed the CQ dry run.

[sof, 2017-03-14 11:41:16]

On 2017/03/13 18:09:56, jbroman wrote:
> On 2017/03/13 at 16:04:03, sigbjornf wrote:
> > On 2017/03/13 15:45:52, jbroman wrote:
> > > On 2017/03/13 at 15:10:49, sigbjornf wrote:
> > > >
> > >
>
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> > > > File
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
> > > (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/2741793003/diff/80001/third_party/WebKit/Sour...
> > > >
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:469:
> void
> > > SerializedScriptValue::finalizeTransferringContext() {
> > > > On 2017/03/13 15:01:14, jbroman wrote:
> > > > > Why do we believe registerMemoryAllocatedWithCurrentScriptContext will
> be
> > > called
> > > > > before finalizeTransferringContext? In the case that we're sending to
a
> > > worker,
> > > > > we don't create the MessageEvent until we arrive on the worker thread,
> which
> > > may
> > > > > not be (and in fact, probably is not) before the sending thread runs
> > > > > finalizeTransferringContext.
> > > > 
> > > > I don't see how to accommodate that, so we will have to accept a
> cross-context
> > > overestimate.
> > > 
> > > This is how I'd imagine it ought to work (mild pseudo-code). I'm not sure
> how
> > > SharedArrayBuffer should be accounted (maybe the V8 SAB folk have thought
> about
> > > it), but setting that aside:
> > > 
> > > Both SerializedScriptValue and ArrayBufferContents::DataHolder have a
> boolean,
> > > |m_externalMemoryRegistered|, indicating whether their memory is
registered
> with
> > > the current context (name doesn't matter -- we could reuse the existing
> name).
> > > 
> > > They both have methods:
> > > 
> > > void T::registerExternalMemory() {
> > >   // if SerializedScriptValue, delegate to ArrayBufferContents as well
> > >   if (m_externalMemoryRegistered)
> > >     return;
> > >   isolate->AdjustAmountOfExternalAllocatedMemory(externalMemoryUsage);
> > >   m_externalMemoryRegistered = true;
> > > }
> > > 
> > > void T::unregisterExternalMemory() {
> > >   // if SerializedScriptValue, delegate to ArrayBufferContents as well
> > >   if (!m_externalMemoryRegistered)
> > >     return;
> > >   isolate->AdjustAmountOfExternalAllocatedMemory(-externalMemoryUsage);
> > >   m_externalMemoryRegistered = false;
> > > }
> > > 
> > > They each invoke unregisterExternalMemory in their destructor, ensuring
that
> any
> > > adjustment they make is eventually cancelled. When we intend to transfer
to
> > > another thread, we do:
> > > 
> > > message->unregisterExternalMemory();
> > > target->postMessage(std::move(message), ...);
> > > 
> > > If the message is discarded, then the memory has already been
unregistered,
> and
> > > since that fact is stored in the boolean, the destructor won't do so
again.
> > > If the message is registered to another thread, then it will happen only
> after
> > > the unregistration has already happened -- and that registration will be
> > > balanced by the destructor.
> > 
> > Won't this cause under-reporting if the message is discarded, but the
> ArrayBuffer(s) are still referred to on the source side?
> 
> Except for SharedArrayBuffer, the source array buffers (and their views) are
> neutered if they are transferred. Not sure if there are other cases where the
> source can have other references to the same contents (such things would seem
to
> already be a threading bug, because such access is unsychronized?).

Yes, good, neutering disassociates the contents from the buffer object, so
reporting will remain accurate even if the neutered object is held onto. Updated
to do so.

[jbroman, 2017-03-14 15:43:33]

lgtm

https://codereview.chromium.org/2741793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2741793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:144:
m_hasRegisteredExternalAllocation = !m_hasRegisteredExternalAllocation;
nit: sanity DCHECK that this is becoming true if diff > 0, and false if diff <
0?

[sof, 2017-03-14 16:51:27]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[sof, 2017-03-14 16:51:39]

thanks, hope that proves a stable base for cost transfer. 

One too many iterations required to get there, I should have thought matters
through more carefully.

https://codereview.chromium.org/2741793003/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):

https://codereview.chromium.org/2741793003/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:144:
m_hasRegisteredExternalAllocation = !m_hasRegisteredExternalAllocation;
On 2017/03/14 15:43:33, jbroman wrote:
> nit: sanity DCHECK that this is becoming true if diff > 0, and false if diff <
> 0?

Done.

[commit-bot: I haz the power, 2017-03-14 16:51:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-03-14 16:55:22]

LGTM

[jbroman, 2017-03-14 18:01:19]

Thanks for your patience. :)

Please update the CL description before landing.

[commit-bot: I haz the power, 2017-03-14 18:30:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 18:30:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2017-03-14 18:56:58]

Description was changed from

==========
Split up SerializedScriptValue cost handoff into two stages.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another.

As that transfer is in general between threads, extra care is needed
to make that handoff of cost attribution race free, which r456009
failed to handle correctly. Address cross-thread transfer by splitting
up the transferring on the 'source' side into two: first marking the
SerializedScriptValue as being transferred and if that transfer went
ahead as wanted, finalize and discount the external allocation cost from the
source context. Non-interfering access to the shared SerializedScriptValue
is accomplished as a result.

R=
BUG=700353
==========

to

==========
Accurate transfer of SerializedScriptValue allocation costs.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another as part
of a postMessage()

The handoff 'protocol' provided there fell short in that it could
fail to subtract transferable (array buffer contents) costs in
the source context, or end up doing it twice if the postMessage()
failed. Bookkeeping confusion resulted.

Rework the mechanism by instead having ArrayBufferContents keep
track of its external allocation cost registration status, so as
to prevent double discounting. Along with that, it is both safe
and accurate to unregister all allocation costs prior to
transfer. Should the value successfully be posted to its target
context, cost will be registered there. And if not, the value will
be destructed (..but without discounting allocation cost yet again.)

R=jbroman,haraken
BUG=700353
==========

[sof, 2017-03-14 19:01:10]

On 2017/03/14 18:01:19, jbroman wrote:
> Thanks for your patience. :)
> 
> Please update the CL description before landing.

Done.

The testcase here is an example that shows that aggregating external allocation
cost as part of GC tracing, wouldn't be sufficient -- the transfer of cost needs
to be done right away..in order to trigger that next GC. (I've wondered at times
if that would be a 'better' way to do external allocation costs.)

[sof, 2017-03-14 19:01:18]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-03-14 19:01:19]

The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2741793003/#ps140001
(title: "sanity check diff adjustments")

[commit-bot: I haz the power, 2017-03-14 19:02:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-03-14 19:52:40]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2017-03-14 19:52:52]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2017-03-14 19:53:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 20:04:51]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489521172843450,
"parent_rev": "d7de5323b1ddb18bce389091b8a0c4f327fd86d1", "commit_rev":
"e75f1381dccdeef00b30479ad55432f60ee8db7d"}

[commit-bot: I haz the power, 2017-03-14 20:05:35]

Description was changed from

==========
Accurate transfer of SerializedScriptValue allocation costs.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another as part
of a postMessage()

The handoff 'protocol' provided there fell short in that it could
fail to subtract transferable (array buffer contents) costs in
the source context, or end up doing it twice if the postMessage()
failed. Bookkeeping confusion resulted.

Rework the mechanism by instead having ArrayBufferContents keep
track of its external allocation cost registration status, so as
to prevent double discounting. Along with that, it is both safe
and accurate to unregister all allocation costs prior to
transfer. Should the value successfully be posted to its target
context, cost will be registered there. And if not, the value will
be destructed (..but without discounting allocation cost yet again.)

R=jbroman,haraken
BUG=700353
==========

to

==========
Accurate transfer of SerializedScriptValue allocation costs.

r456009 added transferring of allocation costs for a
SerializedScriptValue and any array buffers that it refers to,
transferring that cost from one v8 context to another as part
of a postMessage()

The handoff 'protocol' provided there fell short in that it could
fail to subtract transferable (array buffer contents) costs in
the source context, or end up doing it twice if the postMessage()
failed. Bookkeeping confusion resulted.

Rework the mechanism by instead having ArrayBufferContents keep
track of its external allocation cost registration status, so as
to prevent double discounting. Along with that, it is both safe
and accurate to unregister all allocation costs prior to
transfer. Should the value successfully be posted to its target
context, cost will be registered there. And if not, the value will
be destructed (..but without discounting allocation cost yet again.)

R=jbroman,haraken
BUG=700353

Review-Url: https://codereview.chromium.org/2741793003
Cr-Commit-Position: refs/heads/master@{#456800}
Committed:
https://chromium.googlesource.com/chromium/src/+/e75f1381dccdeef00b30479ad554...
==========

[commit-bot: I haz the power, 2017-03-14 20:05:36]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/e75f1381dccdeef00b30479ad554...