Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2446593002:
[wasm] Fix memory leak in wasm-module-runner.cc (Closed)
Description[wasm] Fix memory leak in wasm-module-runner.cc
BUG=chromium:658057
R=titzer@chromium.org
Committed: https://crrev.com/1fe7a91751ceb7fd037ad2539feb320797cda96e
Cr-Commit-Position: refs/heads/master@{#40586}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add a todo #Messages
Total messages: 17 (10 generated)
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2446593002/diff/1/test/common/wasm/wasm-modul... File test/common/wasm/wasm-module-runner.cc (right): https://codereview.chromium.org/2446593002/diff/1/test/common/wasm/wasm-modul... test/common/wasm/wasm-module-runner.cc:84: std::unique_ptr<const WasmModule> module(DecodeWasmModuleForTesting( This isn't quite right. This C++ object should be cleaned up by the garbage collector, since the instance now holds a special Managed<WasmModule> reference to it. Adding this unique pointer reclaims the C++ object after reaching the end of the method, but the instance still holds a pointer to it. If this is not getting cleaned up because the GC is not running finalizers, then we should find a way to clean it up by explicitly, perhaps by manually deleting the Managed<WasmModule> in InstantiateModuleForTesting.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2446593002/diff/1/test/common/wasm/wasm-modul... File test/common/wasm/wasm-module-runner.cc (right): https://codereview.chromium.org/2446593002/diff/1/test/common/wasm/wasm-modul... test/common/wasm/wasm-module-runner.cc:84: std::unique_ptr<const WasmModule> module(DecodeWasmModuleForTesting( On 2016/10/24 at 13:34:40, titzer wrote: > This isn't quite right. > > This C++ object should be cleaned up by the garbage collector, since the instance now holds a special Managed<WasmModule> reference to it. Adding this unique pointer reclaims the C++ object after reaching the end of the method, but the instance still holds a pointer to it. > > If this is not getting cleaned up because the GC is not running finalizers, then we should find a way to clean it up by explicitly, perhaps by manually deleting the Managed<WasmModule> in InstantiateModuleForTesting. This is not the WasmModule that is given to the instance in InstantiateModuleForTesting, so the garbage collector does not deallocate this WasmModule. I added a TODO in InstantiateModuleForTesting that this module should be given to the instance instead of decoding the module bytes again to make a new WasmModule.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/24 14:04:29, ahaas wrote: > https://codereview.chromium.org/2446593002/diff/1/test/common/wasm/wasm-modul... > File test/common/wasm/wasm-module-runner.cc (right): > > https://codereview.chromium.org/2446593002/diff/1/test/common/wasm/wasm-modul... > test/common/wasm/wasm-module-runner.cc:84: std::unique_ptr<const WasmModule> > module(DecodeWasmModuleForTesting( > On 2016/10/24 at 13:34:40, titzer wrote: > > This isn't quite right. > > > > This C++ object should be cleaned up by the garbage collector, since the > instance now holds a special Managed<WasmModule> reference to it. Adding this > unique pointer reclaims the C++ object after reaching the end of the method, but > the instance still holds a pointer to it. > > > > If this is not getting cleaned up because the GC is not running finalizers, > then we should find a way to clean it up by explicitly, perhaps by manually > deleting the Managed<WasmModule> in InstantiateModuleForTesting. > > This is not the WasmModule that is given to the instance in > InstantiateModuleForTesting, so the garbage collector does not deallocate this > WasmModule. I added a TODO in InstantiateModuleForTesting that this module > should be given to the instance instead of decoding the module bytes again to > make a new WasmModule. lgtm
The CQ bit was checked by ahaas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Fix memory leak in wasm-module-runner.cc BUG=chromium:658057 R=titzer@chromium.org ========== to ========== [wasm] Fix memory leak in wasm-module-runner.cc BUG=chromium:658057 R=titzer@chromium.org Committed: https://crrev.com/1fe7a91751ceb7fd037ad2539feb320797cda96e Cr-Commit-Position: refs/heads/master@{#40586} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1fe7a91751ceb7fd037ad2539feb320797cda96e Cr-Commit-Position: refs/heads/master@{#40586} |
