Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deps: cherry-pick ca0f9573 from V8 upstream #11940

Closed
wants to merge 1 commit into from
Closed

deps: cherry-pick ca0f9573 from V8 upstream #11940

wants to merge 1 commit into from

Conversation

ofrobots
Copy link
Contributor

Original commit message:

Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
…8::SharedArrayBuffer::New.

This API does not allow reporting failure, but we should crash rather than have
the caller get an ArrayBuffer that isn't properly set up.

BUG=chromium:681843

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

deps:v8

@ofrobots
deps: cherry-pick ca0f9573 from V8 upstream
4db4d0d 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

  Review-Url: https://codereview.chromium.org/2641953002
  Cr-Commit-Position: refs/heads/master@{nodejs#42511}
@ofrobots ofrobots added lts-watch-v4.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 20, 2017
@nodejs-github-bot nodejs-github-bot added v7.x v8 engine Issues and PRs related to the V8 dependency. labels Mar 20, 2017
@ofrobots
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/6938/
V8-CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/615/

/cc @nodejs/v8

bnoordhuis
bnoordhuis approved these changes Mar 20, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let me just note that ValueDeserializer::ReadJSArrayBuffer() and WebAssemblyMemory() also don't check the return value.

We don't use either in core at the moment but there is an open pull request that introduces ValueDeserializer bindings.

@italoacasas
Copy link
Contributor

Landed in staging

@ofrobots ofrobots deleted the arraybuffer-new-oom branch March 23, 2017 01:00
@MylesBorins
Copy link
Contributor

FWIW this landed without appropriate meta data

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@ofrobots @MylesBorins
deps: cherry-pick ca0f9573 from V8 upstream
d7d0dba 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

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

PR-URL: #11940
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Apr 18, 2017 •
edited
Loading

Landed on v6.x in d7d0dba
Landed in v4.x in 929fd6c

4.x required some manual care... please review

c v4.xi: https://ci.nodejs.org/job/node-test-commit-v8-linux/656/
ci v6.x: https://ci.nodejs.org/job/node-test-commit-v8-linux/655/

edit 2: CI GREEN!

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
@ofrobots @MylesBorins
deps: cherry-pick ca0f9573 from V8 upstream
929fd6c 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

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

PR-URL: #11940
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
bnoordhuis referenced this pull request Apr 18, 2017
@ofrobots @italoacasas
deps: cherry-pick ca0f9573 from V8 upstream
ca31986 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

  Review-Url: https://codereview.chromium.org/2641953002
  Cr-Commit-Position: refs/heads/master@{#42511}
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@ofrobots @MylesBorins
deps: cherry-pick ca0f9573 from V8 upstream
ab3fdf5 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

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

PR-URL: #11940
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
@ofrobots @MylesBorins
deps: cherry-pick ca0f9573 from V8 upstream
8cdddcd 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

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

PR-URL: #11940
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
This was referenced Apr 19, 2017
Merged
Merged
gibfahn pushed a commit to ibmruntimes/node that referenced this pull request May 3, 2017
@ofrobots @gibfahn
deps: cherry-pick ca0f9573 from V8 upstream
f3e8bea 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

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

PR-URL: nodejs/node#11940
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@ofrobots @andrew749
deps: cherry-pick ca0f9573 from V8 upstream
677035e 
Original commit message:
  Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v…
  …8::SharedArrayBuffer::New.

  This API does not allow reporting failure, but we should crash rather than have
  the caller get an ArrayBuffer that isn't properly set up.

  BUG=chromium:681843

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

PR-URL: nodejs/node#11940
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ofrobots @italoacasas @MylesBorins @bnoordhuis @jasnell @addaleax @nodejs-github-bot