New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 694657 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug



Sign in to add a comment

Remove support for experimental WebAssembly version 0xD

Project Member Reported by titzer@chromium.org, Feb 21 2017

Issue description

As per title
For graphics-related bugs, please copy/paste the contents of the about:gpu
page at the end of this report.

 

Comment 1 by titzer@chromium.org, Feb 21 2017

Labels: Merge-Request-57

Comment 2 by gov...@chromium.org, Feb 21 2017

Could you please add bug description and link to CL here?

Comment 3 by gov...@chromium.org, Feb 21 2017

Also pls apply appropriate OS labels. Thank you.

Comment 4 by titzer@chromium.org, Feb 22 2017

Labels: OS-All
In Chrome 57 and 58, V8 currently accepts both version 0xD (experimental) and 0x1 (official) binary formats for WebAssembly. The following CL removes the 0xD support:

CL is here: https://codereview.chromium.org/2709753003/
Project Member

Comment 6 by bugdroid1@chromium.org, Feb 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/5004748868a15be9ba47727ab15c278fcca4f2b1

commit 5004748868a15be9ba47727ab15c278fcca4f2b1
Author: machenbach <machenbach@chromium.org>
Date: Wed Feb 22 13:59:23 2017

Revert of [wasm] Remove support for experimental version 0xD. (patchset #3 id:40001 of https://codereview.chromium.org/2709753003/ )

Reason for revert:
Breaks layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/13730

Original issue's description:
> [wasm] Remove support for experimental version 0xD.
>
> R=mtrofin@chromium.org
> BUG=chromium:575167, chromium:694657 
>
> Review-Url: https://codereview.chromium.org/2709753003
> Cr-Commit-Position: refs/heads/master@{#43368}
> Committed: https://chromium.googlesource.com/v8/v8/+/c8329253ea345e06a923f7800f96f4ef59262997

TBR=ahaas@chromium.org,mtrofin@chromium.org,titzer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=chromium:575167, chromium:694657 

Review-Url: https://codereview.chromium.org/2706303004
Cr-Commit-Position: refs/heads/master@{#43373}

[modify] https://crrev.com/5004748868a15be9ba47727ab15c278fcca4f2b1/src/wasm/module-decoder.cc
[modify] https://crrev.com/5004748868a15be9ba47727ab15c278fcca4f2b1/src/wasm/module-decoder.h
[modify] https://crrev.com/5004748868a15be9ba47727ab15c278fcca4f2b1/test/mjsunit/wasm/jsapi-harness.js

Project Member

Comment 7 by bugdroid1@chromium.org, Feb 22 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/85df088e0310cbb08280e453afc371faa01d0042

commit 85df088e0310cbb08280e453afc371faa01d0042
Author: mtrofin <mtrofin@chromium.org>
Date: Wed Feb 22 19:44:19 2017

[wasm] bump version of binary incrementer.wasm to 0x1

This is in preparation of v8 supporting that version only. It currently
supports both 0x1 and 0xD.

BUG=chromium:575167
BUG= chromium:694657 

Review-Url: https://codereview.chromium.org/2710033002
Cr-Commit-Position: refs/heads/master@{#452165}

[modify] https://crrev.com/85df088e0310cbb08280e453afc371faa01d0042/third_party/WebKit/LayoutTests/http/tests/wasm/incrementer.wasm

Project Member

Comment 9 by bugdroid1@chromium.org, Feb 22 2017

Labels: merge-merged-5.7
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/e22438bbc6935b0d11cccf6193344c1000b4d6a2

commit e22438bbc6935b0d11cccf6193344c1000b4d6a2
Author: Brad Nelson <bradnelson@chromium.org>
Date: Wed Feb 22 21:24:07 2017

Merged: [wasm] Remove support for experimental version 0xD.

Revision: 6a09a41622d65d07fcb4436ddfaaaa6a9cad2467

BUG=chromium:575167, chromium:694657 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=mtrofin@chromium.org

Review-Url: https://codereview.chromium.org/2708373004 .
Cr-Commit-Position: refs/branch-heads/5.7@{#128}
Cr-Branched-From: 975e9a320b6eaf9f12280c35df98e013beb8f041-refs/heads/5.7.492@{#1}
Cr-Branched-From: 8d76f0e3465a84bbf0bceab114900fbe75844e1f-refs/heads/master@{#42426}

[modify] https://crrev.com/e22438bbc6935b0d11cccf6193344c1000b4d6a2/src/wasm/module-decoder.cc
[modify] https://crrev.com/e22438bbc6935b0d11cccf6193344c1000b4d6a2/src/wasm/wasm-module.h

Cc: hablich@chromium.org
Cl listed at #9 got merged to M57 without approval. Also this is reported as P3, can't this wait until M58?
Cc: bradnelson@chromium.org
Components: Blink>JavaScript>WebAssembly
Labels: -Pri-3 Pri-1
This merge is ok because it is only affecting WASM. The prio should be 1, because it is a planned prerequisite for the coordinated launch. I am wondering too why it was facilitated like that though.

I suppose you also might want to merge https://chromium.googlesource.com/chromium/src.git/+/85df088e0310cbb08280e453afc371faa01d0042?

Is this bug fixed btw?
Actually, you're right we need that one too, ok to merge?

Status: Fixed (was: Assigned)
Project Member

Comment 14 by bugdroid1@chromium.org, Feb 22 2017

Labels: merge-merged-2987
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9413c369cb4d9457015b9fe0401630eef3b0bad6

commit 9413c369cb4d9457015b9fe0401630eef3b0bad6
Author: Brad Nelson <bradnelson@chromium.org>
Date: Wed Feb 22 23:19:38 2017

[wasm] bump version of binary incrementer.wasm to 0x1

This is in preparation of v8 supporting that version only. It currently
supports both 0x1 and 0xD.

BUG=chromium:575167
BUG= chromium:694657 

Review-Url: https://codereview.chromium.org/2710033002
Cr-Commit-Position: refs/heads/master@{#452165}
(cherry picked from commit 85df088e0310cbb08280e453afc371faa01d0042)

Review-Url: https://codereview.chromium.org/2707263007 .
Cr-Commit-Position: refs/branch-heads/2987@{#648}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/9413c369cb4d9457015b9fe0401630eef3b0bad6/third_party/WebKit/LayoutTests/http/tests/wasm/incrementer.wasm

Project Member

Comment 15 by sheriffbot@chromium.org, Feb 23 2017

Labels: -Merge-Request-57 Hotlist-Merge-Review Merge-Review-57
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Per comment #9 and #14, changes are already merged to M57. 
If nothing is pending for M57, hablich@, could you please remove "Merge-Review-57" label. Thank you.
Labels: -Merge-Review-57

Sign in to add a comment