[wasm] Block compile/instantiate of large array buffers

[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=693945
R=bradnelson@chromium.org,esprehn@chromium.org,jochen@chromium.org

Review-Url: https://codereview.chromium.org/2702953002
Cr-Commit-Position: refs/heads/master@{#451811}
Committed: https://chromium.googlesource.com/chromium/src/+/50e18a2d44ebf8da875090bf9f0a4645d6741d23

[Mircea Trofin, 2017-02-18 18:17:10]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-18 18:17:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mircea Trofin, 2017-02-18 18:20:03]

Description was changed from

==========
[wasm] Block compile/instantiate of large array buffers

BUG=
==========

to

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=
==========

[Mircea Trofin, 2017-02-18 18:20:03]

mtrofin@chromium.org changed reviewers:
+ bradnelson@chromium.org

[commit-bot: I haz the power, 2017-02-18 18:28:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-18 18:28:01]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bradnelson, 2017-02-18 22:09:08]

Description was changed from

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=
==========

to

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=693945
==========

[bradnelson, 2017-02-18 22:16:22]

lgtm

I take it the red is from V8 being behind?

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js:1: // Copyright 2015
the V8 project authors. All rights reserved.
2017 + Chromium / Blink to match where this is landing.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js:342: var
threwException = true;
Indent 2 per JS style guide.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js:356: throw new
MjsUnitAssertionError("Did not trap, expected: " + kTrapMsgs[trap]);
>80

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-test.html (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-test.html:12: <script>
Could we put this script in a separate js file?

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-tests.js (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-tests.js:23:
assert_true(new WebAssembly.Module(buffs.small) instanceof WebAssembly.Module);
>80

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-module-builder.js:1: // Copyright
2016 the V8 project authors. All rights reserved.
I see this is lifted from v8.
Don't imagine we have the plumbing to share with that copy?

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:327: if (asPromise)
Maybe comment that we allow any size for promise i.e. async cases?

[bradn, 2017-02-18 22:24:12]

Description was changed from

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=693945
==========

to

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=693945

R=bradnelson@chromium.org,esprehn@chromium.org,jochen@chromium.org
==========

[bradn, 2017-02-18 22:24:12]

bradnelson@google.com changed reviewers:
+ esprehn@chromium.org, jochen@chromium.org

[bradn, 2017-02-18 22:24:58]

bradnelson@google.com changed reviewers:
+ bradnelson@google.com

[bradn, 2017-02-18 22:24:59]

+jochen for advice on if there's a better way to share v8 code needed to
generate the module with LayoutTests?

+esprehn for OWNERS

[Mircea Trofin, 2017-02-19 00:18:33]

Incorporated feedback. 

Bots will be red until autoroller picks up my v8 side change.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js:1: // Copyright 2015
the V8 project authors. All rights reserved.
On 2017/02/18 22:16:22, bradnelson wrote:
> 2017 + Chromium / Blink to match where this is landing.

Done.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js:342: var
threwException = true;
On 2017/02/18 22:16:22, bradnelson wrote:
> Indent 2 per JS style guide.

Done.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-constants.js:356: throw new
MjsUnitAssertionError("Did not trap, expected: " + kTrapMsgs[trap]);
On 2017/02/18 22:16:22, bradnelson wrote:
> >80

Done.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-test.html (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-test.html:12: <script>
On 2017/02/18 22:16:22, bradnelson wrote:
> Could we put this script in a separate js file?

As a weak argument, I've seen this pattern elsewhere in LayoutTests, however, if
the preferred pattern is to move all
js in .js files, I can move these guys in the wasm-limits-tests.js file.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-tests.js (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-limits-tests.js:23:
assert_true(new WebAssembly.Module(buffs.small) instanceof WebAssembly.Module);
On 2017/02/18 22:16:22, bradnelson wrote:
> >80

Done.

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/wasm/wasm-module-builder.js (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/wasm/wasm-module-builder.js:1: // Copyright
2016 the V8 project authors. All rights reserved.
On 2017/02/18 22:16:22, bradnelson wrote:
> I see this is lifted from v8.
> Don't imagine we have the plumbing to share with that copy?

We had a similar problem with the wasm structured cloning tests in 
LayoutTests. See https://codereview.chromium.org/2255673003/. Long story 
short, I chatted offline with dpranke and eisinger, and the recommendation
at the time was to follow this pattern, because LayoutTests can't reference out
of their directory. Very happy to help changing that, but 
would prefer separating the issues, unless we already have a 
mechanism in place, and given the tight schedule (we want to make 57 with 
this change).

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/2702953002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:327: if (asPromise)
On 2017/02/18 22:16:22, bradnelson wrote:
> Maybe comment that we allow any size for promise i.e. async cases?

Done.

[jochen (gone - plz use gerrit), 2017-02-20 09:29:17]

lgtm

[Mircea Trofin, 2017-02-21 16:27:10]

The CQ bit was checked by mtrofin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 16:27:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 18:07:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 18:07:13]

Dry run: This issue passed the CQ dry run.

[Mircea Trofin, 2017-02-21 18:28:10]

The CQ bit was checked by mtrofin@chromium.org

[Mircea Trofin, 2017-02-21 18:28:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
bradnelson@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/2702953002/#ps60001
(title: "Updated after V8 side landed")

[commit-bot: I haz the power, 2017-02-21 18:28:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 19:07:58]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487701690732080,
"parent_rev": "5ad0be710a2027c2a3971d3aa09a2c7fc720d9f6", "commit_rev":
"50e18a2d44ebf8da875090bf9f0a4645d6741d23"}

[commit-bot: I haz the power, 2017-02-21 19:08:34]

Description was changed from

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=693945

R=bradnelson@chromium.org,esprehn@chromium.org,jochen@chromium.org
==========

to

==========
[wasm] Block compile/instantiate of large array buffers

To discourage development patterns leading to bad user experience, we 
block wasm compilation/instantiation through the sync APIs
for modules larger than a certain size.

Promise-based APIs still succeed; also, sync APIs succeed in worker 
contexts.

BUG=693945

R=bradnelson@chromium.org,esprehn@chromium.org,jochen@chromium.org

Review-Url: https://codereview.chromium.org/2702953002
Cr-Commit-Position: refs/heads/master@{#451811}
Committed:
https://chromium.googlesource.com/chromium/src/+/50e18a2d44ebf8da875090bf9f0a...
==========

[commit-bot: I haz the power, 2017-02-21 19:08:35]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/50e18a2d44ebf8da875090bf9f0a...

[Eden Wang, 2017-02-23 08:50:02]

nedenwang@tencent.com changed reviewers:
+ nedenwang@tencent.com

[Eden Wang, 2017-02-23 08:50:03]

https://codereview.chromium.org/2702953002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/2702953002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:146: const size_t
kWasmWireBytesLimit = 1 << 12;
Hi mtrodfin,
    Why choose this value? 
    In most cases, subsequent logic relies on the completion of compiling for
wasm, Is there any plan to reduce wasm compile time?