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

Issue 720302 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

ArrayBuffers allocated by wasm should use the api-provided allocator

Project Member Reported by mlippautz@chromium.org, May 10 2017

Issue description

For guard pages wasm currently allocates ArrayBuffers on the side. This violates the contract that V8 is using the API-provided allocator for all ArrayBuffer allocations.

Currently this already creates issues as we need to have different paths for allocation/free within V8. It will completely blow up once Blink gets hold onto those buffers and tries to free them.

 

Comment 2 by eholk@chromium.org, May 11 2017

Doing this will require adding some functionality to the array buffer allocator. The document linked below provides some more detail:

https://docs.google.com/a/chromium.org/document/d/1ZR-5x24a8AngopOVuVEn3fXG69LYmzQVA2LnmqlAZQI/edit?usp=sharing
Project Member

Comment 3 by bugdroid1@chromium.org, May 19 2017

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

commit 3603fb05a650d7915f8facb649bf25e9b9d3decb
Author: Eric Holk <eholk@chromium.org>
Date: Fri May 19 21:54:50 2017

[wasm] Use ArrayBuffer::Allocator API for guard regions

The WebAssembly code now uses these new APIs to allocate memory with guard
regions. Guarded array buffers are no longer always external, which eliminates
a lot of special cases around WebAssembly memory.

Bug:  chromium:720302 
Change-Id: I355b74ac30a05a18c8b363bd256d57458742849f
Reviewed-on: https://chromium-review.googlesource.com/505715
Reviewed-by: Deepti Gandluri <gdeepti@chromium.org>
Reviewed-by: Brad Nelson <bradnelson@chromium.org>
Commit-Queue: Eric Holk <eholk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45436}
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/src/objects-inl.h
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/src/objects.cc
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/src/objects.h
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/src/wasm/wasm-module.cc
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/src/wasm/wasm-module.h
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/src/wasm/wasm-objects.cc
[modify] https://crrev.com/3603fb05a650d7915f8facb649bf25e9b9d3decb/test/cctest/wasm/test-run-wasm-module.cc

Comment 4 by eholk@chromium.org, May 19 2017

The V8 part of this should be fixed now. We still need to implement the new allocator API in Chromium.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 8 2017

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

commit 8f39e07d80f689eda04c176e9cb33dce1471f7d1
Author: Eric Holk <eholk@chromium.org>
Date: Thu Jun 08 02:51:13 2017

Add allocation information to ArrayBuffer::Contents

Array buffers can now have an allocation that is larger than the actual
buffer, such as when WebAssembly guard regions are enabled. Embedders
need to know the actual allocation start and length when externalizing
a buffer so they can deallocate it properly.

Bug:  chromium:720302 , v8:5277
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: Ifc184fdd59d77af01c07a64d2c0229ca859a01b0
Reviewed-on: https://chromium-review.googlesource.com/523271
Commit-Queue: Eric Holk <eholk@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#45777}
[modify] https://crrev.com/8f39e07d80f689eda04c176e9cb33dce1471f7d1/include/v8.h
[modify] https://crrev.com/8f39e07d80f689eda04c176e9cb33dce1471f7d1/src/api.cc
[modify] https://crrev.com/8f39e07d80f689eda04c176e9cb33dce1471f7d1/test/cctest/test-api.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Jun 23 2017

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

commit 714060db89bbb6dc78229cd0b582bd51df8f1bd4
Author: Eric Holk <eholk@chromium.org>
Date: Fri Jun 23 18:13:44 2017

Implement updated array buffer allocator in gin

Bug:  chromium:720302 
Change-Id: I127daca6ee9954774b8ff23382ba38cb23da7318
Reviewed-on: https://chromium-review.googlesource.com/543670
Commit-Queue: Eric Holk <eholk@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481959}
[modify] https://crrev.com/714060db89bbb6dc78229cd0b582bd51df8f1bd4/gin/BUILD.gn
[modify] https://crrev.com/714060db89bbb6dc78229cd0b582bd51df8f1bd4/gin/array_buffer.cc
[modify] https://crrev.com/714060db89bbb6dc78229cd0b582bd51df8f1bd4/gin/array_buffer.h
[add] https://crrev.com/714060db89bbb6dc78229cd0b582bd51df8f1bd4/gin/array_buffer_unittest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 23 2017

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

commit dea76b7b72e53291607621c905a52c38f8eef279
Author: Ted Choc <tedchoc@chromium.org>
Date: Fri Jun 23 18:46:11 2017

Revert "Implement updated array buffer allocator in gin"

This reverts commit 714060db89bbb6dc78229cd0b582bd51df8f1bd4.

Reason for revert: This broke the cronet builds:
FAILED: lib_net_unittests__library.so lib_net_unittests__library.so.TOC ...
../../gin/array_buffer.cc:40: error: undefined reference to 'base::AllocPages(void*, unsigned long, unsigned long, base::PageAccessibilityConfiguration)'
../../gin/array_buffer.cc:55: error: undefined reference to 'base::FreePages(void*, unsigned long)'
../../gin/array_buffer.cc:67: error: undefined reference to 'base::SetSystemPagesInaccessible(void*, unsigned long)'
../../gin/array_buffer.cc:70: error: undefined reference to 'base::SetSystemPagesAccessible(void*, unsigned long)'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

https://uberchromegw.corp.google.com/i/chromium.android/builders/Android%20Cronet%20Marshmallow%2064bit%20Builder/builds/10832

Original change's description:
> Implement updated array buffer allocator in gin
> 
> Bug:  chromium:720302 
> Change-Id: I127daca6ee9954774b8ff23382ba38cb23da7318
> Reviewed-on: https://chromium-review.googlesource.com/543670
> Commit-Queue: Eric Holk <eholk@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481959}

TBR=eholk@chromium.org,jochen@chromium.org

Change-Id: Id51402c6a8e87ef8af6eb3d0f8c8332f52eeefdb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:720302 
Reviewed-on: https://chromium-review.googlesource.com/546676
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Commit-Queue: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481970}
[modify] https://crrev.com/dea76b7b72e53291607621c905a52c38f8eef279/gin/BUILD.gn
[modify] https://crrev.com/dea76b7b72e53291607621c905a52c38f8eef279/gin/array_buffer.cc
[modify] https://crrev.com/dea76b7b72e53291607621c905a52c38f8eef279/gin/array_buffer.h
[delete] https://crrev.com/4da73c092149777b32b73e9b324d907ae2bc990e/gin/array_buffer_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 23 2017

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

commit 9038baea6db2fbda8b0638dfded93e3d01e4130e
Author: Xida Chen <xidachen@chromium.org>
Date: Fri Jun 23 19:08:00 2017

Revert "Implement updated array buffer allocator in gin"

This reverts commit 714060db89bbb6dc78229cd0b582bd51df8f1bd4.

Reason for revert: <INSERT REASONING HERE>
Causing compile error here:
https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20Builder%20%28dbg%29/builds/5272

Original change's description:
> Implement updated array buffer allocator in gin
> 
> Bug:  chromium:720302 
> Change-Id: I127daca6ee9954774b8ff23382ba38cb23da7318
> Reviewed-on: https://chromium-review.googlesource.com/543670
> Commit-Queue: Eric Holk <eholk@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481959}

TBR=eholk@chromium.org,jochen@chromium.org

Change-Id: I135e796978521c6db250b53f9bcf4099641abeb4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  chromium:720302 
Reviewed-on: https://chromium-review.googlesource.com/546118
Reviewed-by: Xida Chen <xidachen@chromium.org>
Commit-Queue: Xida Chen <xidachen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481982}

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 17 2017

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

commit b2fa95d4ff42aa262456175532d52266749e5668
Author: Eric Holk <eholk@chromium.org>
Date: Mon Jul 17 18:51:36 2017

Reland "Implement updated array buffer allocator in gin"

This is a reland of 714060db89bbb6dc78229cd0b582bd51df8f1bd4
Original change's description:
> Implement updated array buffer allocator in gin
> 
> Bug:  chromium:720302 
> Change-Id: I127daca6ee9954774b8ff23382ba38cb23da7318
> Reviewed-on: https://chromium-review.googlesource.com/543670
> Commit-Queue: Eric Holk <eholk@chromium.org>
> Reviewed-by: Jochen Eisinger <jochen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#481959}

Bug:  chromium:720302 
Change-Id: I880773c83d51c8ec0d3d55dfa8913e6b3a16ddec
Reviewed-on: https://chromium-review.googlesource.com/570978
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Commit-Queue: Eric Holk <eholk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487179}
[modify] https://crrev.com/b2fa95d4ff42aa262456175532d52266749e5668/gin/BUILD.gn
[modify] https://crrev.com/b2fa95d4ff42aa262456175532d52266749e5668/gin/array_buffer.cc
[modify] https://crrev.com/b2fa95d4ff42aa262456175532d52266749e5668/gin/array_buffer.h
[add] https://crrev.com/b2fa95d4ff42aa262456175532d52266749e5668/gin/array_buffer_unittest.cc

Comment 11 by eholk@chromium.org, Jul 17 2017

Status: Fixed (was: Assigned)

Sign in to add a comment