Fix some leaks and failures under valgrind in JS extensions unit tests.

Fix some leaks and failures under valgrind in JS extensions unit tests.

This change:
- Adds running the GC until the v8 heap size reaches steady state during
  ModuleSystemTest::TearDown. This is necessary to ensure that
  gin::Wrappable objects are freed, as finalizers for JS objects aren't
  run during isolate shut down.
- Removes a racy check that failed under valgrind.

BUG=389016, 406487
Committed: https://crrev.com/32a6fc7d37cf7c6e921779664bc02bbc353add57
Cr-Commit-Position: refs/heads/master@{#294779}

[benwells, 2014-08-26 03:51:22]

I'm wondering if kalman would be a better reviewer for this. Hasn't he been
reviewing your changes to the js test system?

[Sam McNally, 2014-08-26 04:16:08]

sammc@chromium.org changed reviewers:
+ kalman@chromium.org
- benwells@chromium.org

[Sam McNally, 2014-08-26 04:16:08]

+kalman
-benwells

[not at google - send to devlin, 2014-08-27 19:28:58]

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/module_s...
File extensions/renderer/module_system_test.cc (right):

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/module_s...
extensions/renderer/module_system_test.cc:224: env_.reset();
env_ is created in the constructor, why do you need to destroy it here?

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/serial_service.js (right):

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/serial_service.js:26: });
Hm, why does the destruction of |serviceRouter| (via the context being
destroyed) not cause it to automatically close? All of its references should
have gone. I.e. is it not enough to just call the RunGCForTesting method and not
change any code in here?

[Sam McNally, 2014-08-28 07:06:51]

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/module_s...
File extensions/renderer/module_system_test.cc (right):

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/module_s...
extensions/renderer/module_system_test.cc:224: env_.reset();
On 2014/08/27 19:28:58, kalman wrote:
> env_ is created in the constructor, why do you need to destroy it here?

We need to remove the references from either ModuleSystem or ModuleRegistry to
any modules that use gin to wrap a C++ object before running the GC. It appears
that v8 doesn't run finalizers during isolate destruction, so we have to remove
all references and run the GC before the isolate is destroyed.

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/resource...
File extensions/renderer/resources/serial_service.js (right):

https://codereview.chromium.org/502893002/diff/1/extensions/renderer/resource...
extensions/renderer/resources/serial_service.js:26: });
On 2014/08/27 19:28:58, kalman wrote:
> Hm, why does the destruction of |serviceRouter| (via the context being
> destroyed) not cause it to automatically close? All of its references should
> have gone. I.e. is it not enough to just call the RunGCForTesting method and
not
> change any code in here?

It appears "kFullGarbageCollection" doesn't mean what I thought it meant. The
old version does work as expected if the GC is run enough.

[not at google - send to devlin, 2014-08-28 15:42:04]

lgtm, is the CL description still accurate?

https://codereview.chromium.org/502893002/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system_test.cc (right):

https://codereview.chromium.org/502893002/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system_test.cc:212: env_(CreateEnvironment()),
As a nit, I wonder if env_ should be initialised in a SetUp method to mirror it
being destroyed in TearDown?

https://codereview.chromium.org/502893002/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system_test.cc:235: }
Yikes :(

[Sam McNally, 2014-08-29 01:13:21]

https://codereview.chromium.org/502893002/diff/20001/extensions/renderer/modu...
File extensions/renderer/module_system_test.cc (right):

https://codereview.chromium.org/502893002/diff/20001/extensions/renderer/modu...
extensions/renderer/module_system_test.cc:212: env_(CreateEnvironment()),
On 2014/08/28 15:42:04, kalman wrote:
> As a nit, I wonder if env_ should be initialised in a SetUp method to mirror
it
> being destroyed in TearDown?

Done.

[Sam McNally, 2014-08-29 01:14:16]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-08-29 01:15:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sammc@chromium.org/502893002/40001

[commit-bot: I haz the power, 2014-08-29 02:13:33]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-29 02:22:35]

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

[commit-bot: I haz the power, 2014-08-29 02:22:35]

Try jobs failed on following builders:
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sam McNally, 2014-09-15 02:46:38]

The CQ bit was checked by sammc@chromium.org

[commit-bot: I haz the power, 2014-09-15 02:47:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/502893002/40001

[commit-bot: I haz the power, 2014-09-15 02:49:39]

Committed patchset #3 (id:40001) as 1b8eec14ebd25ed263d0bb71889a2ce10d07527a

[commit-bot: I haz the power, 2014-09-15 02:52:36]

Patchset 3 (id:??) landed as
https://crrev.com/32a6fc7d37cf7c6e921779664bc02bbc353add57
Cr-Commit-Position: refs/heads/master@{#294779}