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

src: fix crash with SyntheticModule#setExports #30062

Closed
wants to merge 2 commits into from
Closed

src: fix crash with SyntheticModule#setExports #30062

wants to merge 2 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Oct 22, 2019

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@targos targos requested a review from devsnek October 22, 2019 13:31
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Oct 22, 2019
@targos targos changed the title Fix synthetic modules src: fix crash with SyntheticModule#setExports Oct 22, 2019
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 22, 2019 •
edited by targos
Loading

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

devsnek
devsnek approved these changes Oct 22, 2019
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

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

lol I was just about to open a pr for this

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos
deps: V8: backport 777fa98
f13686b 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98
@targos
src: fix crash with SyntheticModule#setExport
48c74ef 
Use the new non-deprecated V8 API for that.
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

Trott pushed a commit that referenced this pull request Oct 26, 2019
@targos @Trott
deps: V8: backport 777fa98
d29f0ed 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Trott pushed a commit that referenced this pull request Oct 26, 2019
@targos @Trott
src: fix crash with SyntheticModule#setExport
ab78d4d 
Use the new non-deprecated V8 API for that.

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
Member

Trott commented Oct 26, 2019

Landed in 7b2de22...ab78d4d

@Trott Trott closed this Oct 26, 2019
targos added a commit that referenced this pull request Oct 28, 2019
@targos
deps: V8: backport 777fa98
f3d00c5 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Oct 28, 2019
@targos
src: fix crash with SyntheticModule#setExport
d78e317 
Use the new non-deprecated V8 API for that.

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Nov 5, 2019
Merged
@targos
Copy link
Member Author

targos commented Nov 8, 2019

Should only land on v12.x if #29864 does.

@targos targos deleted the fix-synthetic-modules branch November 8, 2019 13:08
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 7, 2020
@targos @aduh95
deps: V8: backport 777fa98
72493af 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: nodejs#30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 7, 2020
@targos @aduh95
src: fix crash with SyntheticModule#setExport
a6a273c 
Use the new non-deprecated V8 API for that.

PR-URL: nodejs#30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@aduh95 aduh95 mentioned this pull request Jan 7, 2020
Closed
2 tasks
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 8, 2020
@targos @aduh95
deps: V8: backport 777fa98
abcc819 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: nodejs#30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
aduh95 pushed a commit to aduh95/node that referenced this pull request Jan 8, 2020
@targos @aduh95
src: fix crash with SyntheticModule#setExport
69afd66 
Use the new non-deprecated V8 API for that.

PR-URL: nodejs#30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Jan 8, 2020
@targos
deps: V8: backport 777fa98
42c4560 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos added a commit that referenced this pull request Jan 8, 2020
@targos
src: fix crash with SyntheticModule#setExport
32e5c39 
Use the new non-deprecated V8 API for that.

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos targos mentioned this pull request Jan 15, 2020
Closed
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@targos @BethGriggs
deps: V8: backport 777fa98
c4074e3 
Original commit message:

    Make SetSyntheticModuleExport throw instead of crash for nonexistent export name

    Per spec, Module::SetSyntheticModuleExport should throw a ReferenceError
    when called with an export name that was not supplied when constructing
    that SyntheticModule.  Instead, the current implementation crashes with
    a failed CHECK().

    Add a new Module::SyntheticModuleSetExport that throws (without an ensuing
    crash) for this case, and deprecate the old
    Module::SetSyntheticModuleExport.

    Bug: v8:9828
    Change-Id: I3b3d353064c3851882781818099bd8f6ee74c809
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1860996
    Reviewed-by: Adam Klein <[email protected]>
    Reviewed-by: Georg Neis <[email protected]>
    Commit-Queue: Dan Clark <[email protected]>
    Cr-Commit-Position: refs/heads/master@{#64438}

Refs: v8/v8@777fa98

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
@targos @BethGriggs
src: fix crash with SyntheticModule#setExport
e11a376 
Use the new non-deprecated V8 API for that.

PR-URL: #30062
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@devsnek devsnek devsnek approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@targos @nodejs-github-bot @Trott @jasnell @addaleax @cjihrig @devsnek @gireeshpunathil @BridgeAR