Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2867233002:
[wasm] Do not unregister an ArrayBuffer if it is already external (Closed)
Description[wasm] Do not unregister an ArrayBuffer if it is already external
- Currently if GrowMemory is called with pages = 0, an attempt is made to
unregister the ArrayBuffer even if it is external. Cleanup so all Detaching
of ArrayBuffer is centralized to one method, and can only be called fromJS.
- Gate creating WeakHandles to the memory on the buffer having guard pages
enabled. Currently creating a WeakHandle is gated only on if the buffer
is_external true. If a buffer is marked is_external = true to begin with,
the WeakHandle is created and the Finalizer is run causing the program to
crash.
BUG=chromium:717647
Review-Url: https://codereview.chromium.org/2867233002
Cr-Commit-Position: refs/heads/master@{#45238}
Committed: https://chromium.googlesource.com/v8/v8/+/e2fc979e0e9b81b37144e3371a05163324102dd0
Patch Set 1 #Patch Set 2 : Fix #Patch Set 3 : Format #Patch Set 4 : Fix constructor #Patch Set 5 : Fix bot #
Total comments: 4
Patch Set 6 : Andreas's review #Patch Set 7 : Rebase #
Total comments: 4
Patch Set 8 : Clemens's review #Patch Set 9 : Eric's review #
Messages
Total messages: 50 (38 generated)
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: CQ cannot get SignCLA result. Please try later.
The CQ bit was checked by gdeepti@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...
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_win_compile_dbg on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_compile_dbg/builds/3...)
Description was changed from ========== [wasm] Do not unregister an ArrayBuffer if it is already external BUG=chromium:717647 R=eholk@chromium.org, bradnelson@chromium.org ========== to ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching an ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently a WeakHandle is allocated only on if the buffer is_external = true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ==========
gdeepti@chromium.org changed reviewers: + ahaas@chromium.org - bradnelson@chromium.org
The CQ bit was checked by gdeepti@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...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by gdeepti@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...
Description was changed from ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching an ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently a WeakHandle is allocated only on if the buffer is_external = true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ========== to ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching an ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external = true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with nits https://codereview.chromium.org/2867233002/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2867233002/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2235: uint32_t pages) { could you just pass in a boolean flag here? I think this would make the purpose of this parameter more obvious. https://codereview.chromium.org/2867233002/diff/80001/test/cctest/wasm/test-r... File test/cctest/wasm/test-run-wasm-module.cc (right): https://codereview.chromium.org/2867233002/diff/80001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm-module.cc:1140: static const int kPageSize = 0x10000; use WasmModule::kPageSize instead.
The CQ bit was checked by gdeepti@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/2867233002/diff/80001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2867233002/diff/80001/src/wasm/wasm-module.cc... src/wasm/wasm-module.cc:2235: uint32_t pages) { On 2017/05/10 08:33:48, ahaas wrote: > could you just pass in a boolean flag here? I think this would make the purpose > of this parameter more obvious. Done. https://codereview.chromium.org/2867233002/diff/80001/test/cctest/wasm/test-r... File test/cctest/wasm/test-run-wasm-module.cc (right): https://codereview.chromium.org/2867233002/diff/80001/test/cctest/wasm/test-r... test/cctest/wasm/test-run-wasm-module.cc:1140: static const int kPageSize = 0x10000; On 2017/05/10 08:33:48, ahaas wrote: > use WasmModule::kPageSize instead. Done.
clemensh@chromium.org changed reviewers: + clemensh@chromium.org
Sorry for nitpicking, but can you please format the commit message according to the style guide (https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... That makes it much easier to read over CLs e.g. on the waterfall. Thanks! https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:743: bool free_memory = delta_size != 0 ? true : false; Why not just (delta_size != 0)?
Description was changed from ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching an ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external = true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ========== to ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching an ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external = true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ==========
Edited the message in the browser so missed formatting, thanks for pointing it out! https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc File src/wasm/wasm-js.cc (right): https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc#ne... src/wasm/wasm-js.cc:743: bool free_memory = delta_size != 0 ? true : false; On 2017/05/10 16:00:10, Clemens Hammacher wrote: > Why not just (delta_size != 0)? Done.
On 2017/05/10 at 16:14:28, gdeepti wrote: > Edited the message in the browser so missed formatting, thanks for pointing it out! > > https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc > File src/wasm/wasm-js.cc (right): > > https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc#ne... > src/wasm/wasm-js.cc:743: bool free_memory = delta_size != 0 ? true : false; > On 2017/05/10 16:00:10, Clemens Hammacher wrote: > > Why not just (delta_size != 0)? > > Done. Thanks! There is still a newline missing though ;) From the style guide: "A short subject and a blank line after the subject are crucial."
Description was changed from ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching an ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external = true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ========== to ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching of ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ==========
Description was changed from ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching of ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ========== to ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching of ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ==========
On 2017/05/10 16:15:53, Clemens Hammacher wrote: > On 2017/05/10 at 16:14:28, gdeepti wrote: > > Edited the message in the browser so missed formatting, thanks for pointing it > out! > > > > https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc > > File src/wasm/wasm-js.cc (right): > > > > > https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-js.cc#ne... > > src/wasm/wasm-js.cc:743: bool free_memory = delta_size != 0 ? true : false; > > On 2017/05/10 16:00:10, Clemens Hammacher wrote: > > > Why not just (delta_size != 0)? > > > > Done. > > Thanks! > There is still a newline missing though ;) > > From the style guide: "A short subject and a blank line after the subject are > crucial." Thanks again, done. :)
The CQ bit was checked by gdeepti@chromium.org
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/2867233002/#ps140001 (title: "Clemens's review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with nit https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:858: if (is_external && enable_guard_regions) { nit: I think just `if (enable_guard_regions)` is better here.
The CQ bit was unchecked by gdeepti@chromium.org
The CQ bit was checked by gdeepti@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/2867233002/diff/120001/src/wasm/wasm-module.cc File src/wasm/wasm-module.cc (right): https://codereview.chromium.org/2867233002/diff/120001/src/wasm/wasm-module.c... src/wasm/wasm-module.cc:858: if (is_external && enable_guard_regions) { On 2017/05/10 16:23:15, Eric Holk wrote: > nit: I think just `if (enable_guard_regions)` is better here. Done.
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 gdeepti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eholk@chromium.org, ahaas@chromium.org Link to the patchset: https://codereview.chromium.org/2867233002/#ps160001 (title: "Eric's review")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494436129166650,
"parent_rev": "eab268e5a6ea99e0d83da09d6bc2819639fde276", "commit_rev":
"e2fc979e0e9b81b37144e3371a05163324102dd0"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching of ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 ========== to ========== [wasm] Do not unregister an ArrayBuffer if it is already external - Currently if GrowMemory is called with pages = 0, an attempt is made to unregister the ArrayBuffer even if it is external. Cleanup so all Detaching of ArrayBuffer is centralized to one method, and can only be called fromJS. - Gate creating WeakHandles to the memory on the buffer having guard pages enabled. Currently creating a WeakHandle is gated only on if the buffer is_external true. If a buffer is marked is_external = true to begin with, the WeakHandle is created and the Finalizer is run causing the program to crash. BUG=chromium:717647 Review-Url: https://codereview.chromium.org/2867233002 Cr-Commit-Position: refs/heads/master@{#45238} Committed: https://chromium.googlesource.com/v8/v8/+/e2fc979e0e9b81b37144e3371a051633241... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/v8/v8/+/e2fc979e0e9b81b37144e3371a051633241... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
