[wasm] Fix test-run-wasm-module tests in debug mode.

[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes for moving Compilation/Instantiation off the module object (https://codereview.chromium.org/2320723005). The problem here is that SetupIsolateForWasm tries to add the same property to a module_object multiple times and hits a DCHECK when this property is found on a lookup.
 - Fixed to use the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org

Committed: https://crrev.com/dd3b14b7b47693806b62b25a102c8d912cd90004
Cr-Commit-Position: refs/heads/master@{#39484}

[gdeepti, 2016-09-16 06:42:15]

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

[commit-bot: I haz the power, 2016-09-16 06:42:18]

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

[gdeepti, 2016-09-16 06:43:31]

Description was changed from

==========
[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes
for moving Compilation/Instantiation off the module object
(https://codereview.chromium.org/2320723005). The problem here is that
SetupIsolateForWasm tries to add the same property to a module_object multiple
times and hits a DCHECK when this property is found on a lookup.  - Fixed to use
the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The
install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes
for moving Compilation/Instantiation off the module object
(https://codereview.chromium.org/2320723005). The problem here is that
SetupIsolateForWasm tries to add the same property to a module_object multiple
times and hits a DCHECK when this property is found on a lookup.   
 - Fixed to use the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The
install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org
==========

[ahaas, 2016-09-16 07:01:32]

I like this change, thanks. lgtm

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-module.cc (right):

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
Could you put this check to SetupIsolateForWasmModule, so that it's code is
executed not more than once?

[gdeepti, 2016-09-16 07:01:54]

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

[commit-bot: I haz the power, 2016-09-16 07:01:56]

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 07:07:45]

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

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

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gdeepti, 2016-09-16 07:07:48]

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-module.cc (right):

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
On 2016/09/16 07:01:32, ahaas wrote:
> Could you put this check to SetupIsolateForWasmModule, so that it's code is
> executed not more than once?

Thought of doing that initially, but the problem is that
SetupIsolateForWasmModule should be called only once per isolate, and the method
can be called for different isolates (For example the serialization test below).
This case is unique because we use the generic CcTest::InitIsolateOnce() instead
of a new isolate and repeatedly invoke TestModule.

[Mircea Trofin, 2016-09-16 07:13:01]

Not sure I understand what this fixes - could you link the failing test?

https://codereview.chromium.org/2342263002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-module.cc (right):

https://codereview.chromium.org/2342263002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
This will only setup the first isolate.

https://codereview.chromium.org/2342263002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:241:
testing::SetupIsolateForWasmModule(isolate);
On line 240, we reassign isolate to the v8_isolate created on line 234. Since
isolate_setup_for_module == true, we won't initialize this new one. I'm not sure
we're fixing anything this way.

[gdeepti, 2016-09-16 07:37:21]

There is no link to a failing case as this does not fail on the bots. I'm
assuming that is because there is some flag that bypasses DCHECKS. This only
happens on a debug build - I tried x64, as this is not architecture specific I'm
assuming other architectures will behave the same. 

out/x64.debug/cctest test-run-wasm-module will result in - 

# Fatal error in ../src/objects.cc, line 5709
# Check failed: !it.IsFound().

Ran into this while trying to debug a separate change to the same file.

https://codereview.chromium.org/2342263002/diff/40001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-module.cc (right):

https://codereview.chromium.org/2342263002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
On 2016/09/16 07:13:01, Mircea Trofin wrote:
> This will only setup the first isolate.

My understanding is that the isolate being returned by CcTest::InitIsolateOnce()
is the only one being used for all the tests except the serializer test, which
sets up its own isolate when a new isolate is used.

https://codereview.chromium.org/2342263002/diff/40001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:241:
testing::SetupIsolateForWasmModule(isolate);
On 2016/09/16 07:13:00, Mircea Trofin wrote:
> On line 240, we reassign isolate to the v8_isolate created on line 234. Since
> isolate_setup_for_module == true, we won't initialize this new one. I'm not
sure
> we're fixing anything this way.

Can you elaborate what you mean by we won't initialize this new one? Doesn't
calling the SetupIsolateForWasmModule bypass the isolate_setup_for_module ==
true check?  Calling this method more than once for the same isolate is the case
I am attempting to fix.

[Mircea Trofin, 2016-09-16 18:05:41]

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-module.cc (right):

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
On 2016/09/16 07:07:48, gdeepti wrote:
> On 2016/09/16 07:01:32, ahaas wrote:
> > Could you put this check to SetupIsolateForWasmModule, so that it's code is
> > executed not more than once?
> 
> Thought of doing that initially, but the problem is that
> SetupIsolateForWasmModule should be called only once per isolate, and the
method
> can be called for different isolates (For example the serialization test
below).
> This case is unique because we use the generic CcTest::InitIsolateOnce()
instead
> of a new isolate and repeatedly invoke TestModule.

(as chatted offline) You can fish out from the isolate if it's been setup, thus
avoiding a global variable. 

At that point, I'd rename "SetupIsolateForWasmModule" to
"EnsureIsolateIsWasmReady" or something like that.

[gdeepti, 2016-09-16 18:11:56]

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

[commit-bot: I haz the power, 2016-09-16 18:12:03]

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 18:26:33]

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

[commit-bot: I haz the power, 2016-09-16 18:26:34]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gdeepti, 2016-09-16 23:49:23]

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

[commit-bot: I haz the power, 2016-09-16 23:49: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, 2016-09-17 00:04:15]

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

[commit-bot: I haz the power, 2016-09-17 00:04:16]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)
  v8_linux64_gyp_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng_trigg...)

[gdeepti, 2016-09-17 00:26:12]

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

[commit-bot: I haz the power, 2016-09-17 00:26:16]

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

[gdeepti, 2016-09-17 00:31:02]

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
File test/cctest/wasm/test-run-wasm-module.cc (right):

https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
On 2016/09/16 18:05:40, Mircea Trofin wrote:
> On 2016/09/16 07:07:48, gdeepti wrote:
> > On 2016/09/16 07:01:32, ahaas wrote:
> > > Could you put this check to SetupIsolateForWasmModule, so that it's code
is
> > > executed not more than once?
> > 
> > Thought of doing that initially, but the problem is that
> > SetupIsolateForWasmModule should be called only once per isolate, and the
> method
> > can be called for different isolates (For example the serialization test
> below).
> > This case is unique because we use the generic CcTest::InitIsolateOnce()
> instead
> > of a new isolate and repeatedly invoke TestModule.
> 
> (as chatted offline) You can fish out from the isolate if it's been setup,
thus
> avoiding a global variable. 
> 
> At that point, I'd rename "SetupIsolateForWasmModule" to
> "EnsureIsolateIsWasmReady" or something like that.

Removed global variable, moved checking to InstallWasmModuleSymbol instead of
cctest file. Renamed Install functions with a suffix of IfNeeded as suggested by
mtrofin@ offline.

[Mircea Trofin, 2016-09-17 00:33:40]

On 2016/09/17 00:31:02, gdeepti wrote:
>
https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
> File test/cctest/wasm/test-run-wasm-module.cc (right):
> 
>
https://codereview.chromium.org/2342263002/diff/20001/test/cctest/wasm/test-r...
> test/cctest/wasm/test-run-wasm-module.cc:32: if (!isolate_setup_for_module) {
> On 2016/09/16 18:05:40, Mircea Trofin wrote:
> > On 2016/09/16 07:07:48, gdeepti wrote:
> > > On 2016/09/16 07:01:32, ahaas wrote:
> > > > Could you put this check to SetupIsolateForWasmModule, so that it's code
> is
> > > > executed not more than once?
> > > 
> > > Thought of doing that initially, but the problem is that
> > > SetupIsolateForWasmModule should be called only once per isolate, and the
> > method
> > > can be called for different isolates (For example the serialization test
> > below).
> > > This case is unique because we use the generic CcTest::InitIsolateOnce()
> > instead
> > > of a new isolate and repeatedly invoke TestModule.
> > 
> > (as chatted offline) You can fish out from the isolate if it's been setup,
> thus
> > avoiding a global variable. 
> > 
> > At that point, I'd rename "SetupIsolateForWasmModule" to
> > "EnsureIsolateIsWasmReady" or something like that.
> 
> Removed global variable, moved checking to InstallWasmModuleSymbol instead of
> cctest file. Renamed Install functions with a suffix of IfNeeded as suggested
by
> mtrofin@ offline.

LGTM

[commit-bot: I haz the power, 2016-09-17 00:54:48]

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

[commit-bot: I haz the power, 2016-09-17 00:54:50]

Dry run: This issue passed the CQ dry run.

[gdeepti, 2016-09-17 01:26:39]

The CQ bit was checked by gdeepti@chromium.org

[gdeepti, 2016-09-17 01:26:40]

The patchset sent to the CQ was uploaded after l-g-t-m from ahaas@chromium.org
Link to the patchset: https://codereview.chromium.org/2342263002/#ps100001
(title: "Rename Install functions")

[commit-bot: I haz the power, 2016-09-17 01:26:49]

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-17 01:30:16]

Description was changed from

==========
[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes
for moving Compilation/Instantiation off the module object
(https://codereview.chromium.org/2320723005). The problem here is that
SetupIsolateForWasm tries to add the same property to a module_object multiple
times and hits a DCHECK when this property is found on a lookup.   
 - Fixed to use the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The
install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes
for moving Compilation/Instantiation off the module object
(https://codereview.chromium.org/2320723005). The problem here is that
SetupIsolateForWasm tries to add the same property to a module_object multiple
times and hits a DCHECK when this property is found on a lookup.   
 - Fixed to use the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The
install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org
==========

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

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-17 01:31:12]

Description was changed from

==========
[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes
for moving Compilation/Instantiation off the module object
(https://codereview.chromium.org/2320723005). The problem here is that
SetupIsolateForWasm tries to add the same property to a module_object multiple
times and hits a DCHECK when this property is found on a lookup.   
 - Fixed to use the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The
install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org
==========

to

==========
[wasm] Fix test-run-wasm-module tests in debug mode.

test-run-wasm-module cctests broken in debug since recent refactoring changes
for moving Compilation/Instantiation off the module object
(https://codereview.chromium.org/2320723005). The problem here is that
SetupIsolateForWasm tries to add the same property to a module_object multiple
times and hits a DCHECK when this property is found on a lookup.
 - Fixed to use the setup method only once when CcTest::InitIsolateOnce is used.
 - Move setup method to test as this is only used for cctests/fuzzers. The
install method should take care of this in the regular JS pipeline.

 R=mtrofin@chromium.org, ahaas@chromium.org

Committed: https://crrev.com/dd3b14b7b47693806b62b25a102c8d912cd90004
Cr-Commit-Position: refs/heads/master@{#39484}
==========

[commit-bot: I haz the power, 2016-09-17 01:31:13]

Patchset 6 (id:??) landed as
https://crrev.com/dd3b14b7b47693806b62b25a102c8d912cd90004
Cr-Commit-Position: refs/heads/master@{#39484}