Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(701)

Issue 2748473004: [wasm] Transferrable modules (Closed)

Created:
3 years, 9 months ago by Mircea Trofin
Modified:
3 years, 9 months ago
Reviewers:
bradnelson, jbroman
CC:
esprehn, Michael Hablich, Paweł Hajdan Jr., v8-merges_googlegroups.com, v8-reviews_googlegroups.com
Target Ref:
refs/heads/master
Project:
v8
Visibility:
Public.

Description

[wasm] Transferrable modules We want to restrict structured cloning in Chrome to: - postMessage senders and receivers that are co-located in the same process - indexedDB (just https). For context, on the Chrome side, we will achieve the postMessage part by using a mechanism similar to transferrables: the SerializedScriptValue will have a list of wasm modules, separate from the serialized data stream; and this list won't be copied cross process boundaries. The IDB part is achieved by explicitly opting in reading/writing to the serialization stream. To block attack vectors in IPC cases, the default for deserialization will be to expect data in the wasm transfers list. This change is the V8 side necessary to enabling this design. We introduce TransferrableModule, an opaque datatype exposed to the embedder. Internally, TransferrableModules are just serialized data, because we don't have a better mechanism, at the moment, for de-contextualizing/re-contextualizing wasm modules (wrt Isolate and Context). The chrome defaults will be implemented in the serialization/deserialization delegates on that side. For the v8 side of things, in the absence of a serialization delegate, the V8 serializer will write to serialization stream. In the absence of a deserialization delegate, the deserializer won't work. This asymmetry is intentional - it communicates to the embedder the need to make a policy decision, otherwise wasm serialization/deserialization won't work "out of the box". BUG=v8:6079 Review-Url: https://codereview.chromium.org/2748473004 Cr-Commit-Position: refs/heads/master@{#43955} Committed: https://chromium.googlesource.com/v8/v8/+/99743ad460ea5b9795ba9d70a074e75d7362a3d1

Patch Set 1 : [wasm] Transferrable Modules #

Patch Set 2 : test for transferrable modules #

Patch Set 3 : test for transferrable modules #

Patch Set 4 : rebase #

Patch Set 5 : Transferrable modules #

Total comments: 13

Patch Set 6 : <rebase> #

Patch Set 7 : Feedback #

Patch Set 8 : fixups (AddObjectWithID) #

Patch Set 9 : compile error #

Total comments: 4

Patch Set 10 : deserializer asks delegate #

Patch Set 11 : test patch #

Total comments: 4

Patch Set 12 : error cases #

Total comments: 4

Patch Set 13 : feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+464 lines, -24 lines) Patch
M include/v8.h View 1 2 3 4 5 6 7 8 9 6 chunks +51 lines, -0 lines 0 comments Download
M src/api.cc View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +48 lines, -0 lines 0 comments Download
M src/value-serializer.h View 1 2 3 4 5 6 7 8 9 5 chunks +7 lines, -0 lines 0 comments Download
M src/value-serializer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +49 lines, -7 lines 0 comments Download
M test/cctest/wasm/test-run-wasm-module.cc View 1 2 3 4 5 6 7 8 9 1 chunk +37 lines, -0 lines 0 comments Download
M test/unittests/value-serializer-unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 7 chunks +272 lines, -17 lines 0 comments Download

Messages

Total messages: 68 (49 generated)
Mircea Trofin
ptal - v8 side of structured cloning controls
3 years, 9 months ago (2017-03-12 16:14:57 UTC) #18
jbroman
How settled is the issue of how wasm structured clone works (both from an impl ...
3 years, 9 months ago (2017-03-13 18:53:38 UTC) #19
Mircea Trofin
https://codereview.chromium.org/2748473004/diff/120001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1718 include/v8.h:1718: virtual Maybe<uint32_t> GetWasmModuleTransferId( On 2017/03/13 18:53:37, jbroman wrote: > ...
3 years, 9 months ago (2017-03-15 18:02:29 UTC) #22
Mircea Trofin
On 2017/03/15 18:02:29, Mircea Trofin wrote: > https://codereview.chromium.org/2748473004/diff/120001/include/v8.h > File include/v8.h (right): > > https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1718 ...
3 years, 9 months ago (2017-03-15 18:04:24 UTC) #23
Mircea Trofin
https://codereview.chromium.org/2748473004/diff/120001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/2748473004/diff/120001/include/v8.h#newcode1831 include/v8.h:1831: uint32_t id); On 2017/03/15 18:02:28, Mircea Trofin wrote: > ...
3 years, 9 months ago (2017-03-16 20:17:27 UTC) #32
jbroman
lgtm once a couple comments are resolved, and once you deal with v8_linux_blink_rel (possibly just ...
3 years, 9 months ago (2017-03-17 14:44:37 UTC) #35
Mircea Trofin
ptal - as agreed offline, went back to the delgate-based deserialization https://codereview.chromium.org/2748473004/diff/200001/src/value-serializer.h File src/value-serializer.h (right): ...
3 years, 9 months ago (2017-03-17 16:55:32 UTC) #40
jbroman
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc#newcode809 src/value-serializer.cc:809: Maybe<uint32_t> transfer_id = delegate_->GetWasmModuleTransferId( Do these calls into the ...
3 years, 9 months ago (2017-03-17 17:08:32 UTC) #41
Mircea Trofin
https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc#newcode809 src/value-serializer.cc:809: Maybe<uint32_t> transfer_id = delegate_->GetWasmModuleTransferId( On 2017/03/17 17:08:32, jbroman wrote: ...
3 years, 9 months ago (2017-03-17 17:16:11 UTC) #42
jbroman
On 2017/03/17 at 17:16:11, mtrofin wrote: > https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc > File src/value-serializer.cc (right): > > https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc#newcode809 ...
3 years, 9 months ago (2017-03-17 17:28:24 UTC) #45
Mircea Trofin
On 2017/03/17 17:28:24, jbroman wrote: > On 2017/03/17 at 17:16:11, mtrofin wrote: > > https://codereview.chromium.org/2748473004/diff/240001/src/value-serializer.cc ...
3 years, 9 months ago (2017-03-17 18:35:41 UTC) #48
jbroman
lgtm with comments https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.cc#newcode1627 src/value-serializer.cc:1627: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_, JSObject); This should be in ...
3 years, 9 months ago (2017-03-17 19:08:49 UTC) #51
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2748473004/280001
3 years, 9 months ago (2017-03-17 22:16:41 UTC) #58
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the ...
3 years, 9 months ago (2017-03-17 22:16:43 UTC) #60
Mircea Trofin
https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2748473004/diff/260001/src/value-serializer.cc#newcode1627 src/value-serializer.cc:1627: RETURN_EXCEPTION_IF_SCHEDULED_EXCEPTION(isolate_, JSObject); On 2017/03/17 19:08:48, jbroman wrote: > This ...
3 years, 9 months ago (2017-03-17 22:16:48 UTC) #61
bradnelson
lgtm
3 years, 9 months ago (2017-03-20 18:23:51 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2748473004/280001
3 years, 9 months ago (2017-03-20 18:23:58 UTC) #64
commit-bot: I haz the power
Committed patchset #13 (id:280001) as https://chromium.googlesource.com/v8/v8/+/99743ad460ea5b9795ba9d70a074e75d7362a3d1
3 years, 9 months ago (2017-03-20 19:03:33 UTC) #67
Michael Achenbach
3 years, 9 months ago (2017-03-21 13:54:00 UTC) #68
Message was sent while issue was closed.
A revert of this CL (patchset #13 id:280001) has been created in
https://codereview.chromium.org/2762163002/ by machenbach@chromium.org.

The reason for reverting is: Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/bui...

See https://github.com/v8/v8/wiki/Blink-layout-tests.

Powered by Google App Engine
This is Rietveld 408576698