binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized before its use.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069
Committed: https://crrev.com/1851a7c26bc46902c7f62de0bcabf774911596c0
Cr-Commit-Position: refs/heads/master@{#422396}

[Yuki, 2016-09-14 13:31:42]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 13:32:10]

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

[Yuki, 2016-09-14 13:35:21]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org

[Yuki, 2016-09-14 13:35:22]

Could you review this CL?

I don't see any problem about where we should adjust.

Fixes two possible defeats, but I'm not sure if this really fixes the flakiness
or not.

[haraken, 2016-09-14 14:43:46]

On 2016/09/14 13:35:22, Yuki wrote:
> Could you review this CL?
> 
> I don't see any problem about where we should adjust.
> 
> Fixes two possible defeats, but I'm not sure if this really fixes the
flakiness
> or not.

Ah, the problem is that |totalSize| is a per-process variable, right? I think it
needs to be a thread-local variable.

[commit-bot: I haz the power, 2016-09-14 14:44:16]

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

[commit-bot: I haz the power, 2016-09-14 14:44:17]

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

[Yuki, 2016-09-15 13:13:18]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 13:13:33]

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

[Yuki, 2016-09-15 13:13:49]

Could you take another look?

[haraken, 2016-09-15 13:57:07]

LGTM

[Yuki, 2016-09-15 14:24:08]

The CQ bit was unchecked by yukishiino@chromium.org

[Yuki, 2016-09-15 14:24:28]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-09-15 14:24:46]

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

[peria, 2016-09-15 14:40:44]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2016-09-15 14:40:44]

I agree that this CL does (2) in the description, but I think it does not do
(1).

I also agree that this CL can fix the target issue, i.e. Cr does not fail with
the CHECK(), but I wonder this CL just hides wrong behaviors. (and it does not
count in the memory usage of non-main threads.)

IIUC, the issue can happen in following situation;
 1. ABC::aAOEAM(+8) in thread B, but be ignored with null s_aAOEAM.
 2. V8initialize() in thread A
 3. ABC::aAOEAM(-8) in thread B

Can't we catch up how this situation happen with doing (1) in this CL's
description?

[Yuki, 2016-09-15 14:42:46]

The CQ bit was unchecked by yukishiino@chromium.org

[Yuki, 2016-09-15 14:55:36]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 14:55:52]

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

[Yuki, 2016-09-15 14:56:10]

Description was changed from

==========
binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069
==========

to

==========
binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized before its use.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069
==========

[Yuki, 2016-09-15 14:57:13]

I was lazy.  Updated.

[commit-bot: I haz the power, 2016-09-15 16:44:13]

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

[commit-bot: I haz the power, 2016-09-15 16:44:14]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yuki, 2016-09-16 13:25:18]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-16 13:25:38]

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, 2016-09-16 15:07:30]

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

[commit-bot: I haz the power, 2016-09-16 15:07:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[peria, 2016-09-20 07:35:52]

I'm sorry for the late reply.
although some tests still fail, code itself LGTM.

[Yuki, 2016-09-21 12:40:41]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 12:40:54]

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, 2016-09-21 14:27:38]

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

[commit-bot: I haz the power, 2016-09-21 14:27:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Yuki, 2016-09-26 12:16:25]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 12:16:35]

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, 2016-09-26 13:24:08]

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

[commit-bot: I haz the power, 2016-09-26 13:24:09]

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

[Yuki, 2016-09-28 10:26:01]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 10:26:13]

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, 2016-09-28 11:34:38]

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

[commit-bot: I haz the power, 2016-09-28 11:34:39]

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

[Yuki, 2016-09-28 13:28:57]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 13:29:14]

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

[Yuki, 2016-09-28 13:33:09]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 13:33:18]

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, 2016-09-28 14:22:32]

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

[commit-bot: I haz the power, 2016-09-28 14:22:33]

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

[Yuki, 2016-09-29 04:51:31]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 04:51:53]

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, 2016-09-29 05:58:54]

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

[commit-bot: I haz the power, 2016-09-29 05:58:55]

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

[Yuki, 2016-09-29 06:52:09]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 06:52:26]

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, 2016-09-29 08:00:24]

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

[commit-bot: I haz the power, 2016-09-29 08:00:25]

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

[Yuki, 2016-09-29 09:42:13]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 09:42:28]

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, 2016-09-29 09:55:55]

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

[commit-bot: I haz the power, 2016-09-29 09:55:56]

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

[Yuki, 2016-09-30 05:22:25]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 05:22:40]

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, 2016-09-30 05:35:23]

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

[commit-bot: I haz the power, 2016-09-30 05:35:24]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[Yuki, 2016-09-30 05:53:12]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 05:53:29]

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, 2016-09-30 06:55:49]

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

[commit-bot: I haz the power, 2016-09-30 06:55:50]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-09-30 08:39:28]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 08:39:40]

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, 2016-09-30 11:01:58]

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

[commit-bot: I haz the power, 2016-09-30 11:01:59]

Dry run: This issue passed the CQ dry run.

[Yuki, 2016-09-30 13:58:25]

There seems a case that an ArrayBuffer created in a thread is passed over to
another thread, so we have to count the total size in a process.

I've updated the CL and the approach changed again from per-thread-counting to
process-wide-counting.

Could you take another look?

[haraken, 2016-09-30 14:12:51]

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

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);

Sorry, what is this DCHECK checking?

[Yuki, 2016-09-30 14:16:11]

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

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
On 2016/09/30 14:12:51, haraken wrote:
> 
> Sorry, what is this DCHECK checking?

I was afraid of the following case.

1) Use ArrayBufferContents (allocate).
2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the first time
through V8Initializer).
3) Use ArrayBufferContents (deallocate).

i.e. I wanted to make sure that ABC::initialize() was called before the first
use of ABC.

[haraken, 2016-09-30 14:21:40]

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

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
On 2016/09/30 14:16:11, Yuki wrote:
> On 2016/09/30 14:12:51, haraken wrote:
> > 
> > Sorry, what is this DCHECK checking?
> 
> I was afraid of the following case.
> 
> 1) Use ArrayBufferContents (allocate).
> 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the first time
> through V8Initializer).
> 3) Use ArrayBufferContents (deallocate).
> 
> i.e. I wanted to make sure that ABC::initialize() was called before the first
> use of ABC.

Isn't the DCHECK at line 112 enough to detect the case?

[Yuki, 2016-09-30 14:27:26]

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

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
On 2016/09/30 14:21:39, haraken wrote:
> On 2016/09/30 14:16:11, Yuki wrote:
> > On 2016/09/30 14:12:51, haraken wrote:
> > > 
> > > Sorry, what is this DCHECK checking?
> > 
> > I was afraid of the following case.
> > 
> > 1) Use ArrayBufferContents (allocate).
> > 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the first
time
> > through V8Initializer).
> > 3) Use ArrayBufferContents (deallocate).
> > 
> > i.e. I wanted to make sure that ABC::initialize() was called before the
first
> > use of ABC.
> 
> Isn't the DCHECK at line 112 enough to detect the case?

Not enough.  s_adjust... is initialized to defaultAdjust... by default, and
tests are using this default implementation.

The problem here is that most of tests do not call ABC::initialize(), and it's
not easy to make all the tests call ACB::initialize(), but we'd like to
guarantee that, if the adjust function is going to be changed, it must be only
once and before any use.

[haraken, 2016-10-03 01:52:11]

On 2016/09/30 14:27:26, Yuki wrote:
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):
> 
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
> s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
> On 2016/09/30 14:21:39, haraken wrote:
> > On 2016/09/30 14:16:11, Yuki wrote:
> > > On 2016/09/30 14:12:51, haraken wrote:
> > > > 
> > > > Sorry, what is this DCHECK checking?
> > > 
> > > I was afraid of the following case.
> > > 
> > > 1) Use ArrayBufferContents (allocate).
> > > 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the first
> time
> > > through V8Initializer).
> > > 3) Use ArrayBufferContents (deallocate).
> > > 
> > > i.e. I wanted to make sure that ABC::initialize() was called before the
> first
> > > use of ABC.
> > 
> > Isn't the DCHECK at line 112 enough to detect the case?
> 
> Not enough.  s_adjust... is initialized to defaultAdjust... by default, and
> tests are using this default implementation.
> 
> The problem here is that most of tests do not call ABC::initialize(), and it's
> not easy to make all the tests call ACB::initialize(),

Why is it so hard?

Tests that need to use V8 need to call V8Initializer::initializeMainThread. And
V8Initializer::initializeMainThread calls ABC::initialize().

[Yuki, 2016-10-03 05:09:41]

On 2016/10/03 01:52:11, haraken wrote:
> On 2016/09/30 14:27:26, Yuki wrote:
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h
(right):
> > 
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
> > s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
> > On 2016/09/30 14:21:39, haraken wrote:
> > > On 2016/09/30 14:16:11, Yuki wrote:
> > > > On 2016/09/30 14:12:51, haraken wrote:
> > > > > 
> > > > > Sorry, what is this DCHECK checking?
> > > > 
> > > > I was afraid of the following case.
> > > > 
> > > > 1) Use ArrayBufferContents (allocate).
> > > > 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the first
> > time
> > > > through V8Initializer).
> > > > 3) Use ArrayBufferContents (deallocate).
> > > > 
> > > > i.e. I wanted to make sure that ABC::initialize() was called before the
> > first
> > > > use of ABC.
> > > 
> > > Isn't the DCHECK at line 112 enough to detect the case?
> > 
> > Not enough.  s_adjust... is initialized to defaultAdjust... by default, and
> > tests are using this default implementation.
> > 
> > The problem here is that most of tests do not call ABC::initialize(), and
it's
> > not easy to make all the tests call ACB::initialize(),
> 
> Why is it so hard?
> 
> Tests that need to use V8 need to call V8Initializer::initializeMainThread.
And
> V8Initializer::initializeMainThread calls ABC::initialize().

Tests under platform/ don't depend on V8Initializer.  Tests under WTF/, too.

[haraken, 2016-10-03 05:13:09]

On 2016/10/03 05:09:41, Yuki wrote:
> On 2016/10/03 01:52:11, haraken wrote:
> > On 2016/09/30 14:27:26, Yuki wrote:
> > >
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > > File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h
> (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > > third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
> > > s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
> > > On 2016/09/30 14:21:39, haraken wrote:
> > > > On 2016/09/30 14:16:11, Yuki wrote:
> > > > > On 2016/09/30 14:12:51, haraken wrote:
> > > > > > 
> > > > > > Sorry, what is this DCHECK checking?
> > > > > 
> > > > > I was afraid of the following case.
> > > > > 
> > > > > 1) Use ArrayBufferContents (allocate).
> > > > > 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the
first
> > > time
> > > > > through V8Initializer).
> > > > > 3) Use ArrayBufferContents (deallocate).
> > > > > 
> > > > > i.e. I wanted to make sure that ABC::initialize() was called before
the
> > > first
> > > > > use of ABC.
> > > > 
> > > > Isn't the DCHECK at line 112 enough to detect the case?
> > > 
> > > Not enough.  s_adjust... is initialized to defaultAdjust... by default,
and
> > > tests are using this default implementation.
> > > 
> > > The problem here is that most of tests do not call ABC::initialize(), and
> it's
> > > not easy to make all the tests call ACB::initialize(),
> > 
> > Why is it so hard?
> > 
> > Tests that need to use V8 need to call V8Initializer::initializeMainThread.
> And
> > V8Initializer::initializeMainThread calls ABC::initialize().
> 
> Tests under platform/ don't depend on V8Initializer.  Tests under WTF/, too.

Are they using ABC? It sounds a bit strange that they don't initialize
V8Initializer but use ABC.

[peria, 2016-10-03 05:43:49]

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

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:112:
DCHECK(s_adjustAmountOfExternalAllocatedMemoryFunction);
s_adjustAOEAMF is initialized with a non null function pointer, so I think this
DCHECK can be removed.

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:113: #if
ENABLE(ASSERT)
If you work for all tests which use ABC without ABC::initialize(), this
ENABLE(ASSERT) may be replaced with a DCHECK(false) in defaultAAOEAMF(),
and I think it better to catch up how the unexpected first call happens.

[Yuki, 2016-10-03 05:49:56]

On 2016/10/03 05:13:09, haraken wrote:
> On 2016/10/03 05:09:41, Yuki wrote:
> > On 2016/10/03 01:52:11, haraken wrote:
> > > On 2016/09/30 14:27:26, Yuki wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > > > File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h
> > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > > > third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
> > > > s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
> > > > On 2016/09/30 14:21:39, haraken wrote:
> > > > > On 2016/09/30 14:16:11, Yuki wrote:
> > > > > > On 2016/09/30 14:12:51, haraken wrote:
> > > > > > > 
> > > > > > > Sorry, what is this DCHECK checking?
> > > > > > 
> > > > > > I was afraid of the following case.
> > > > > > 
> > > > > > 1) Use ArrayBufferContents (allocate).
> > > > > > 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the
> first
> > > > time
> > > > > > through V8Initializer).
> > > > > > 3) Use ArrayBufferContents (deallocate).
> > > > > > 
> > > > > > i.e. I wanted to make sure that ABC::initialize() was called before
> the
> > > > first
> > > > > > use of ABC.
> > > > > 
> > > > > Isn't the DCHECK at line 112 enough to detect the case?
> > > > 
> > > > Not enough.  s_adjust... is initialized to defaultAdjust... by default,
> and
> > > > tests are using this default implementation.
> > > > 
> > > > The problem here is that most of tests do not call ABC::initialize(),
and
> > it's
> > > > not easy to make all the tests call ACB::initialize(),
> > > 
> > > Why is it so hard?
> > > 
> > > Tests that need to use V8 need to call
V8Initializer::initializeMainThread.
> > And
> > > V8Initializer::initializeMainThread calls ABC::initialize().
> > 
> > Tests under platform/ don't depend on V8Initializer.  Tests under WTF/, too.
> 
> Are they using ABC? It sounds a bit strange that they don't initialize
> V8Initializer but use ABC.

Yes, platform/graphics/ is using.  It's not strange.  platform/ cannot depend on
bindings/, so platform/ never initialize V8Initializer.  platform/graphics/ is
using ArrayBuffer as underlying buffer.  No strange here.

[Yuki, 2016-10-03 05:53:26]

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

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:112:
DCHECK(s_adjustAmountOfExternalAllocatedMemoryFunction);
On 2016/10/03 05:43:49, peria wrote:
> s_adjustAOEAMF is initialized with a non null function pointer, so I think
this
> DCHECK can be removed.

You can do ABC::initialize(nullptr), so we cannot remove this.
We can move the check to ABC::initialize, but it's safe to have the CHECK here
because this always works even if we introduced ABC::reset(func), etc.

https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:113: #if
ENABLE(ASSERT)
On 2016/10/03 05:43:49, peria wrote:
> If you work for all tests which use ABC without ABC::initialize(), this
> ENABLE(ASSERT) may be replaced with a DCHECK(false) in defaultAAOEAMF(),
> and I think it better to catch up how the unexpected first call happens.

That's true, but I don't want to do that.  ;)

[haraken, 2016-10-03 06:04:34]

On 2016/10/03 05:49:56, Yuki wrote:
> On 2016/10/03 05:13:09, haraken wrote:
> > On 2016/10/03 05:09:41, Yuki wrote:
> > > On 2016/10/03 01:52:11, haraken wrote:
> > > > On 2016/09/30 14:27:26, Yuki wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > > > > File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h
> > > (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> > > > > third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:119:
> > > > > s_lastUsedAdjustAmountOfExternalAllocatedMemoryFunction);
> > > > > On 2016/09/30 14:21:39, haraken wrote:
> > > > > > On 2016/09/30 14:16:11, Yuki wrote:
> > > > > > > On 2016/09/30 14:12:51, haraken wrote:
> > > > > > > > 
> > > > > > > > Sorry, what is this DCHECK checking?
> > > > > > > 
> > > > > > > I was afraid of the following case.
> > > > > > > 
> > > > > > > 1) Use ArrayBufferContents (allocate).
> > > > > > > 2) Change s_adjustAmountOfExternalAllocatedMemoryFunction (for the
> > first
> > > > > time
> > > > > > > through V8Initializer).
> > > > > > > 3) Use ArrayBufferContents (deallocate).
> > > > > > > 
> > > > > > > i.e. I wanted to make sure that ABC::initialize() was called
before
> > the
> > > > > first
> > > > > > > use of ABC.
> > > > > > 
> > > > > > Isn't the DCHECK at line 112 enough to detect the case?
> > > > > 
> > > > > Not enough.  s_adjust... is initialized to defaultAdjust... by
default,
> > and
> > > > > tests are using this default implementation.
> > > > > 
> > > > > The problem here is that most of tests do not call ABC::initialize(),
> and
> > > it's
> > > > > not easy to make all the tests call ACB::initialize(),
> > > > 
> > > > Why is it so hard?
> > > > 
> > > > Tests that need to use V8 need to call
> V8Initializer::initializeMainThread.
> > > And
> > > > V8Initializer::initializeMainThread calls ABC::initialize().
> > > 
> > > Tests under platform/ don't depend on V8Initializer.  Tests under WTF/,
too.
> > 
> > Are they using ABC? It sounds a bit strange that they don't initialize
> > V8Initializer but use ABC.
> 
> Yes, platform/graphics/ is using.  It's not strange.  platform/ cannot depend
on
> bindings/, so platform/ never initialize V8Initializer.  platform/graphics/ is
> using ArrayBuffer as underlying buffer.  No strange here.

Makes sense.

However, the current DCHECKs look a bit too complicated. I'd prefer simply
removing the DCHECK (because it's already guaranteed that WebKit::initialize is
called at the very beginning of the renderer execution) or replace the DCHECK
with DCHECK(WTF::isInitialized()) or something.

In long term, a right solution would be to move core-independent binding things
to platform/bindings/ and make platform/bindings/ register the
AdjustExternalMemory function.

[peria, 2016-10-03 06:15:08]

On 2016/10/03 05:53:26, Yuki wrote:
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h (right):
> 
>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:112:
> DCHECK(s_adjustAmountOfExternalAllocatedMemoryFunction);
> On 2016/10/03 05:43:49, peria wrote:
> > s_adjustAOEAMF is initialized with a non null function pointer, so I think
> this
> > DCHECK can be removed.
> 
> You can do ABC::initialize(nullptr), so we cannot remove this.
> We can move the check to ABC::initialize, but it's safe to have the CHECK here
> because this always works even if we introduced ABC::reset(func), etc.
got it. thanks.


>
https://codereview.chromium.org/2343643003/diff/280001/third_party/WebKit/Sou...
> third_party/WebKit/Source/wtf/typed_arrays/ArrayBufferContents.h:113: #if
> ENABLE(ASSERT)
> On 2016/10/03 05:43:49, peria wrote:
> > If you work for all tests which use ABC without ABC::initialize(), this
> > ENABLE(ASSERT) may be replaced with a DCHECK(false) in defaultAAOEAMF(),
> > and I think it better to catch up how the unexpected first call happens.
> 
> That's true, but I don't want to do that.  ;)
+1


non-owner lgtm

[Yuki, 2016-10-03 07:34:13]

The CQ bit was checked by yukishiino@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-03 07:34:27]

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

[Yuki, 2016-10-03 07:37:37]

On 2016/10/03 06:04:34, haraken wrote:
> > Yes, platform/graphics/ is using.  It's not strange.  platform/ cannot
depend
> on
> > bindings/, so platform/ never initialize V8Initializer.  platform/graphics/
is
> > using ArrayBuffer as underlying buffer.  No strange here.
> 
> Makes sense.
> 
> However, the current DCHECKs look a bit too complicated. I'd prefer simply
> removing the DCHECK (because it's already guaranteed that WebKit::initialize
is
> called at the very beginning of the renderer execution) or replace the DCHECK
> with DCHECK(WTF::isInitialized()) or something.
> 
> In long term, a right solution would be to move core-independent binding
things
> to platform/bindings/ and make platform/bindings/ register the
> AdjustExternalMemory function.

Factored the check out to checkIfAAOEAMIsConsistent().

I'm on the fence to remove the DCHECKs.  If everything is so trivial, we
wouldn't have had this issue at all.

[haraken, 2016-10-03 07:46:24]

On 2016/10/03 07:37:37, Yuki wrote:
> On 2016/10/03 06:04:34, haraken wrote:
> > > Yes, platform/graphics/ is using.  It's not strange.  platform/ cannot
> depend
> > on
> > > bindings/, so platform/ never initialize V8Initializer. 
platform/graphics/
> is
> > > using ArrayBuffer as underlying buffer.  No strange here.
> > 
> > Makes sense.
> > 
> > However, the current DCHECKs look a bit too complicated. I'd prefer simply
> > removing the DCHECK (because it's already guaranteed that WebKit::initialize
> is
> > called at the very beginning of the renderer execution) or replace the
DCHECK
> > with DCHECK(WTF::isInitialized()) or something.
> > 
> > In long term, a right solution would be to move core-independent binding
> things
> > to platform/bindings/ and make platform/bindings/ register the
> > AdjustExternalMemory function.
> 
> Factored the check out to checkIfAAOEAMIsConsistent().
> 
> I'm on the fence to remove the DCHECKs.  If everything is so trivial, we
> wouldn't have had this issue at all.

LGTM

[Yuki, 2016-10-03 07:59:47]

The CQ bit was unchecked by yukishiino@chromium.org

[Yuki, 2016-10-03 07:59:58]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-10-03 07:59:59]

The patchset sent to the CQ was uploaded after l-g-t-m from peria@chromium.org
Link to the patchset: https://codereview.chromium.org/2343643003/#ps320001
(title: "Factor it out to checkIfAAOEAMIsConsistent.")

[commit-bot: I haz the power, 2016-10-03 08:00:09]

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, 2016-10-03 08:46:40]

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

[commit-bot: I haz the power, 2016-10-03 08:46:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-10-03 08:47:41]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-10-03 08:47:50]

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, 2016-10-03 09:45:25]

Description was changed from

==========
binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized before its use.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069
==========

to

==========
binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized before its use.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069
==========

[commit-bot: I haz the power, 2016-10-03 09:45:28]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-10-03 09:47:25]

Description was changed from

==========
binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized before its use.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069
==========

to

==========
binding: Makes assertion of adjustAmountOfExternalAllocatedMemory more exact.

1) Makes sure that ABC::s_adjustAoEAM is initialized before its use.
2) Makes the assertion in adjustAoEAM thread-safe.

BUG=638069

Committed: https://crrev.com/1851a7c26bc46902c7f62de0bcabf774911596c0
Cr-Commit-Position: refs/heads/master@{#422396}
==========

[commit-bot: I haz the power, 2016-10-03 09:47:26]

Patchset 17 (id:??) landed as
https://crrev.com/1851a7c26bc46902c7f62de0bcabf774911596c0
Cr-Commit-Position: refs/heads/master@{#422396}