FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

deps: backport 8d6a228 from the v8's upstream by indutny · Pull Request #3549 · nodejs/node · GitHub

/ node Public

deps: backport 8d6a228 from the v8's upstream - #3549

Closed
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/v8-1406133003
ConversationCommits (1)ChecksFiles changed
Closed

deps: backport 8d6a228 from the v8's upstream#3549
indutny wants to merge 1 commit into
nodejs:masterfrom
indutny:fix/v8-1406133003

Conversation

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member

Original commit message:

[heap] fix crash during the scavenge of ArrayBuffer
Scavenger should not attempt to visit ArrayBuffer's storage, it is a
user-supplied pointer that may have any alignment. Visiting it, may
result in a crash.

BUG=
R=jochen

Review URL: https://codereview.chromium.org/1406133003

Cr-Commit-Position: refs/heads/master@{#31611}

This is very critical ArrayBuffer fix, and we should backport it to v4 and make a release.

R= @bnoordhuis

cc @nodejs/v8

deps: backport 8d6a228 from the v8's upstream
e31d41f 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{nodejs#31611}
indutny added v8 engine Issues and PRs related to the V8 dependency. land-on-v4.x labels Oct 27, 2015

trevnorris commented Oct 27, 2015

Copy link
Copy Markdown
Contributor

LGTM. Didn't you already back port this?

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

@trevnorris this is a new one

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/629/

Fishrock123 commented Oct 27, 2015

Copy link
Copy Markdown
Contributor

Cc @nodejs/lts

jasnell commented Oct 27, 2015

Copy link
Copy Markdown
Member

Tagged... @Fishrock123 and @indutny , if possible, please use the lts-watch label. I'm working up a tool that'll help triage and cherry pick these kinds of commits in a more automated way and the label definitely helps with that 👍

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

I guess CI is looking good? cc @jasnell

jasnell commented Oct 27, 2015

Copy link
Copy Markdown
Member

@indutny @Fishrock123 ... I'll be working up a new v4.x release today and tomorrow. How about I look to do a release candidate by tomorrow afternoon? Then cut the actual v4.2.2 release on Thursday or Monday assuming everything looks good with the RC? Or is this a much higher priority than that?

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

Of course! Just be aware that this bug is already causing issues for one company, and they are waiting for the patch to deploy 4.x in their product. (Can't really name the company unless they will decide to comment on this themselves)

jasnell commented Oct 27, 2015

Copy link
Copy Markdown
Member

Ok, I'll work on getting the final few commits landed into v4.x-staging today and will get the v4.2.2 Request Proposal put together this evening. I'll do a release candidate build tomorrow and we can go from there. If all goes well, we can hopefully get v4.2.2 out by Thursday but if there are any regressions it could slip to Monday because we definitely do not want to do a new LTS release on a Friday.
/cc @mhdawson

indutny commented Oct 27, 2015

Copy link
Copy Markdown
Member Author

@jasnell of course. Thank you for this!

rvagg mentioned this pull request Oct 27, 2015
Closed

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

@indutny ... looking at cherry picking commits now. I can only pull over commits that have landed so please give me a ping when this one hits in master

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

@jasnell landed in 3223704

indutny closed this Oct 28, 2015
indutny deleted the fix/v8-1406133003 branch October 28, 2015 00:17
indutny added a commit that referenced this pull request Oct 28, 2015
deps: backport 8d6a228 from the v8's upstream
3223704 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{#31611}

PR-URL: #3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

Ok, I optimistically attempted to cherry pick it onto v4.x-staging and the commit definitely does not land cleanly. It'll need a bit of fixing up before it can land. @indutny, if you have the time I'd appreciate some help with that.

indutny added a commit to indutny/io.js that referenced this pull request Oct 28, 2015
deps: backport 8d6a228 from the v8's upstream
792ac6d 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{nodejs#31611}

PR-URL: nodejs#3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

@jasnell backported in 58e0127, please give it a try

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

Argh, actually 792ac6d sorry!

indutny added a commit that referenced this pull request Oct 28, 2015
deps: backport 8d6a228 from the v8's upstream
9061aa2 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{#31611}

PR-URL: #3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

jasnell commented Oct 28, 2015

Copy link
Copy Markdown
Member

Ok, 792ac6d Landed in v4.x-staging as 9061aa2

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

Thank you, James!

rvagg mentioned this pull request Oct 28, 2015
Closed
indutny added a commit that referenced this pull request Oct 28, 2015
deps: backport 8d6a228 from the v8's upstream
6a457bd 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{#31611}

PR-URL: #3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

rvagg commented Oct 28, 2015

Copy link
Copy Markdown
Member

I've had a go at backporting to v3.x @ 6a457bd for inclusion in v3.3.2, it seem to be still relevant for V8 4.4.

rvagg commented Oct 28, 2015

Copy link
Copy Markdown
Member

Without much success, perhaps it's not a problem in V8 4.4 but I'd love confirmation if you have time @indutny:

../deps/v8/src/heap/heap.cc:5046:19: error: no member named 'ContentType' in 'v8::internal::HeapObject'
  switch (target->ContentType()) {
          ~~~~~~  ^
1 error generated.

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

@rvagg is it from 3.3.2?

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

@rvagg this patch should not be backported to it.

rvagg commented Oct 28, 2015

Copy link
Copy Markdown
Member

thanks, backing it out and pushing 3.3.2 forward

indutny commented Oct 28, 2015

Copy link
Copy Markdown
Member Author

@rvagg I will look into it deeply tomorrow to see if there should be any kind of other fix for 3.3.2

bnoordhuis commented Oct 28, 2015

Copy link
Copy Markdown
Member

Belated LGTM.

rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
deps: backport 8d6a228 from the v8's upstream
8c0318c 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{nodejs#31611}

PR-URL: nodejs#3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
rvagg mentioned this pull request Oct 29, 2015
Merged
indutny added a commit that referenced this pull request Oct 29, 2015
deps: backport 8d6a228 from the v8's upstream
cadee67 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{#31611}

PR-URL: #3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell mentioned this pull request Oct 29, 2015
Closed
indutny mentioned this pull request Dec 12, 2015
Closed
indutny added a commit to indutny/io.js that referenced this pull request Dec 14, 2015
deps: backport 8d6a228 from the v8's upstream
65c6208 
Original commit message:

    [heap] fix crash during the scavenge of ArrayBuffer
    Scavenger should not attempt to visit ArrayBuffer's storage, it is a
    user-supplied pointer that may have any alignment. Visiting it, may
    result in a crash.

    BUG=
    R=jochen

    Review URL: https://codereview.chromium.org/1406133003

    Cr-Commit-Position: refs/heads/master@{nodejs#31611}

PR-URL: nodejs#3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

bnoordhuis

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

Back | FazBrowse Home | New Git URL