Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2724053002:
[wasm] Better error message for exceeding module size limits. (Closed)
|
Created:
3 years, 9 months ago by titzer Modified:
3 years, 9 months ago Reviewers:
Mircea Trofin CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[wasm] Better error message for exceeding module size limits.
R=mtrofin@chromium.org
BUG=chromium:695388
Review-Url: https://codereview.chromium.org/2724053002
Cr-Commit-Position: refs/heads/master@{#43524}
Committed: https://chromium.googlesource.com/v8/v8/+/19f24d6ef5ac652a444664d75fd1c52cd96cff2a
Patch Set 1 #Patch Set 2 : [wasm] Better error message for exceeding module size limits. #Messages
Total messages: 18 (9 generated)
The CQ bit was checked by titzer@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 titzer@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...
PTAL
On 2017/03/01 17:07:22, titzer wrote: Trouble is, the message may not be true - the embedder's reasons are unknown here. Moreover, we'll want to use the same mechanism for wasm equivalent of CSP, case in which the message will be wrong. If we want to provide friendlier messages, perhaps we need to design the feature to allow the embedder to provide the error message?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/01 17:15:34, Mircea Trofin wrote: > On 2017/03/01 17:07:22, titzer wrote: > > Trouble is, the message may not be true - the embedder's reasons are unknown > here. Moreover, we'll want to use the same mechanism for wasm equivalent of CSP, > case in which the message will be wrong. > > If we want to provide friendlier messages, perhaps we need to design the feature > to allow the embedder to provide the error message? This is why I would have preferred that this be a less general mechanism.
On 2017/03/01 17:49:28, titzer wrote: > On 2017/03/01 17:15:34, Mircea Trofin wrote: > > On 2017/03/01 17:07:22, titzer wrote: > > > > Trouble is, the message may not be true - the embedder's reasons are unknown > > here. Moreover, we'll want to use the same mechanism for wasm equivalent of > CSP, > > case in which the message will be wrong. > > > > If we want to provide friendlier messages, perhaps we need to design the > feature > > to allow the embedder to provide the error message? > > This is why I would have preferred that this be a less general mechanism. The downside of that would be with wasm CSPs in the equation, as we'd end up with very similar checks in similar places, as well as more entrypoints the embedder would have to worry about. Granted, we don't have yet the CSP stuff in, and the limits had to be rushed out the door, too. How about this: we take the user-friendly change, but also revisit the design when we get wasm CSPs in, one way or the other (meaning, either we go for different entrypoints, or we go for message handshake) How's that sound?
On 2017/03/01 17:56:46, Mircea Trofin wrote: > On 2017/03/01 17:49:28, titzer wrote: > > On 2017/03/01 17:15:34, Mircea Trofin wrote: > > > On 2017/03/01 17:07:22, titzer wrote: > > > > > > Trouble is, the message may not be true - the embedder's reasons are unknown > > > here. Moreover, we'll want to use the same mechanism for wasm equivalent of > > CSP, > > > case in which the message will be wrong. > > > > > > If we want to provide friendlier messages, perhaps we need to design the > > feature > > > to allow the embedder to provide the error message? > > > > This is why I would have preferred that this be a less general mechanism. > > The downside of that would be with wasm CSPs in the equation, as we'd end up > with very similar checks in similar places, as well as more entrypoints the > embedder would have to worry about. > > Granted, we don't have yet the CSP stuff in, and the limits had to be rushed out > the door, too. > > How about this: we take the user-friendly change, but also revisit the design > when we get wasm CSPs in, one way or the other (meaning, either we go for > different entrypoints, or we go for message handshake) > > How's that sound? SGTM, would have proposed that myself :)
On 2017/03/01 17:57:52, titzer wrote: > On 2017/03/01 17:56:46, Mircea Trofin wrote: > > On 2017/03/01 17:49:28, titzer wrote: > > > On 2017/03/01 17:15:34, Mircea Trofin wrote: > > > > On 2017/03/01 17:07:22, titzer wrote: > > > > > > > > Trouble is, the message may not be true - the embedder's reasons are > unknown > > > > here. Moreover, we'll want to use the same mechanism for wasm equivalent > of > > > CSP, > > > > case in which the message will be wrong. > > > > > > > > If we want to provide friendlier messages, perhaps we need to design the > > > feature > > > > to allow the embedder to provide the error message? > > > > > > This is why I would have preferred that this be a less general mechanism. > > > > The downside of that would be with wasm CSPs in the equation, as we'd end up > > with very similar checks in similar places, as well as more entrypoints the > > embedder would have to worry about. > > > > Granted, we don't have yet the CSP stuff in, and the limits had to be rushed > out > > the door, too. > > > > How about this: we take the user-friendly change, but also revisit the design > > when we get wasm CSPs in, one way or the other (meaning, either we go for > > different entrypoints, or we go for message handshake) > > > > How's that sound? > > SGTM, would have proposed that myself :) LGTM opened Issue 6027
The CQ bit was checked by titzer@chromium.org
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": 20001, "attempt_start_ts": 1488393680720810,
"parent_rev": "a5af7fe9ee388a636675f4a6872b1d34fa7d1a7a", "commit_rev":
"19f24d6ef5ac652a444664d75fd1c52cd96cff2a"}
Message was sent while issue was closed.
Description was changed from ========== [wasm] Better error message for exceeding module size limits. R=mtrofin@chromium.org BUG=chromium:695388 ========== to ========== [wasm] Better error message for exceeding module size limits. R=mtrofin@chromium.org BUG=chromium:695388 Review-Url: https://codereview.chromium.org/2724053002 Cr-Commit-Position: refs/heads/master@{#43524} Committed: https://chromium.googlesource.com/v8/v8/+/19f24d6ef5ac652a444664d75fd1c52cd96... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/19f24d6ef5ac652a444664d75fd1c52cd96... |
