Skip to content

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

New issue

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

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

Already on GitHub? # to your account

Conversation

indutny
Copy link
Member

@indutny indutny commented Dec 12, 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}

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

cc @nodejs/v8

indutny referenced this pull request Dec 12, 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}

PR-URL: #3549
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Dec 12, 2015
@ofrobots
Copy link
Contributor

@zcbenz
Copy link
Contributor

zcbenz commented Dec 14, 2015

The upstream patch included a condition for BytecodeArray, should it also be backported?

+      } else if (target->IsBytecodeArray()) {
+        IterateAndMarkPointersToFromSpace(
+            target, obj_address + BytecodeArray::kConstantPoolOffset,
+            obj_address + BytecodeArray::kHeaderSize, record_slots, callback);

@indutny
Copy link
Member Author

indutny commented Dec 14, 2015

@zcbenz you are absolutely right. Thanks for catching this!

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 indutny force-pushed the feature/rebackport-32237041b84faa1ddd2caa43c47b8f75135a37f7 branch from f64194b to 65c6208 Compare December 14, 2015 06:06
@indutny
Copy link
Member Author

indutny commented Dec 14, 2015

Force pushed, PTAL

@indutny
Copy link
Member Author

indutny commented Dec 14, 2015

@indutny
Copy link
Member Author

indutny commented Dec 14, 2015

Seems to be just one unrelated failure on fedora, otherwise CI is green. @ofrobots does this PR still look good to you?

@ofrobots
Copy link
Contributor

I get gateway timeouts on the tap results page for the CI so I cannot verify that the failure is unrelated . LGTM, otherwise.

@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

Rubber stamp LGTM. @indutny ... should this go into LTS?

@indutny
Copy link
Member Author

indutny commented Dec 14, 2015

@jasnell I don't think so, we just forgot to cherry-pick it when upgrading V8 somewhere after LTS (AFAIK)

@indutny
Copy link
Member Author

indutny commented Dec 14, 2015

Landed in 36ac3d6, thank you!

@indutny indutny closed this Dec 14, 2015
@indutny indutny deleted the feature/rebackport-32237041b84faa1ddd2caa43c47b8f75135a37f7 branch December 14, 2015 20:56
indutny added a commit that referenced this pull request Dec 14, 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}

PR-URL: #4259
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ofrobots
Copy link
Contributor

@jasnell: I missed this commit when upgrading from V8 4.6 to V8 4.7 😰. This is not needed in stable or LTS.

@MylesBorins
Copy link
Contributor

@ofrobots I've added dont-land-on-v4.x please let me know if this should be changed

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
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#4259
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants