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: backport 8d6a228 from the v8's upstream #3549

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

indutny
Copy link
Member

@indutny indutny commented Oct 27, 2015

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

@indutny
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 indutny added v8 engine Issues and PRs related to the V8 dependency. land-on-v4.x labels Oct 27, 2015
@trevnorris
Copy link
Contributor

LGTM. Didn't you already back port this?

@indutny
Copy link
Member Author

indutny commented Oct 27, 2015

@trevnorris this is a new one

@indutny
Copy link
Member Author

indutny commented Oct 27, 2015

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

@Fishrock123
Copy link
Contributor

Cc @nodejs/lts

@jasnell
Copy link
Member

jasnell commented Oct 27, 2015

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
Copy link
Member Author

indutny commented Oct 27, 2015

I guess CI is looking good? cc @jasnell

@jasnell
Copy link
Member

jasnell commented Oct 27, 2015

@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
Copy link
Member Author

indutny commented Oct 27, 2015

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
Copy link
Member

jasnell commented Oct 27, 2015

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
Copy link
Member Author

indutny commented Oct 27, 2015

@jasnell of course. Thank you for this!

@rvagg rvagg mentioned this pull request Oct 27, 2015
Closed
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

@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
Copy link
Member Author

indutny commented Oct 28, 2015

@jasnell landed in 3223704

@indutny indutny closed this Oct 28, 2015
@indutny indutny deleted the fix/v8-1406133003 branch October 28, 2015 00:17
indutny added a commit that referenced this pull request Oct 28, 2015
@indutny
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 <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

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
@indutny
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 <[email protected]>
@indutny
Copy link
Member Author

indutny commented Oct 28, 2015

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

@indutny
Copy link
Member Author

indutny commented Oct 28, 2015

Argh, actually 792ac6d sorry!

indutny added a commit that referenced this pull request Oct 28, 2015
@indutny @jasnell
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 <[email protected]>
@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

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

@indutny
Copy link
Member Author

indutny commented Oct 28, 2015

Thank you, James!

@rvagg rvagg mentioned this pull request Oct 28, 2015
Closed
indutny added a commit that referenced this pull request Oct 28, 2015
@indutny @rvagg
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 <[email protected]>
@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

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
Copy link
Member

rvagg commented Oct 28, 2015

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
Copy link
Member Author

indutny commented Oct 28, 2015

@rvagg is it from 3.3.2?

@indutny
Copy link
Member Author

indutny commented Oct 28, 2015

@rvagg this patch should not be backported to it.

@rvagg
Copy link
Member

rvagg commented Oct 28, 2015

thanks, backing it out and pushing 3.3.2 forward

@indutny
Copy link
Member Author

indutny commented Oct 28, 2015

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

@bnoordhuis
Copy link
Member

Belated LGTM.

rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
@indutny @rvagg
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 <[email protected]>
@rvagg rvagg mentioned this pull request Oct 29, 2015
Merged
indutny added a commit that referenced this pull request Oct 29, 2015
@indutny @jasnell
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 <[email protected]>
@jasnell jasnell mentioned this pull request Oct 29, 2015
Closed
@indutny 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
@indutny
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bnoordhuis 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
@indutny @trevnorris @Fishrock123 @jasnell @rvagg @bnoordhuis @targos