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.

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

v12.18.2 proposal #34077

Merged
merged 3 commits into from Jun 30, 2020
Merged

v12.18.2 proposal #34077

merged 3 commits into from Jun 30, 2020

Conversation

BethGriggs
Copy link
Member

@BethGriggs BethGriggs commented Jun 26, 2020

2020-06-30, Version 12.18.2 'Erbium' (LTS), @BethGriggs

Notable changes

  • deps: V8: backport fb26d0bb1835 (Matheus Marchini) #33573
    • Fixes memory leak in PrototypeUsers::Add
  • src: use symbol to store AsyncWrap resource (Anna Henningsen) #31745
    • Fixes reported memory leak in #33468

Commits


From nodejs/Release#494 (comment):

There was a regression in a recent v12.x release that is reportedly blocking users from upgrading to the latest security release. I propose (and volunteer) to prepare an additional patch release for 30th June. I expect the release to contain just bug fixes (almost a maintenance style release).

mmarchini and others added 2 commits June 26, 2020 19:31
Original commit message:

    [objects] Compact and shrink script_list

    So far creating scripts always grew the script_list without ever
    reusing cleared slots or shrinking. While this is probably not a
    problem with script_list in practice, this is still a memory leak.

    Fix this leak by using WeakArrayList::Append instead of AddToEnd.
    Append adds to the end of the array, but potentially compacts and
    shrinks the list as well. Other WeakArrayLists can use this method as
    well, as long as they are not using indices into this array.

    Bug: v8:10031
    Change-Id: If743c4cc3f8d67ab735522f0ded038b2fb43e437
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1967385
    Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
    Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#65640}

Refs: v8/v8@fb26d0b

PR-URL: #33573
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Use a symbol on the bindings object to store the public resource object,
rather than a `v8::Global` Persistent. This has several advantages:

- It’s harder to inadvertently create memory leaks this way.
  The garbage collector sees the `AsyncWrap` →  resource link like
  a regular JS property, and can collect the objects as a group,
  even if the resource object should happen to point back to the
  `AsyncWrap` object.
- This will make it easier in the future to use `owner_symbol` for
  this purpose, which is generally the direction we should be moving
  the `async_hooks` API into (i.e. using more public objects instead
  of letting internal wires stick out).

PR-URL: #31745
Backport-PR-URL: #33962
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Jun 26, 2020
Notable changes:

- deps: V8: backport fb26d0bb1835 (Matheus Marchini)
  [#33573](#33573)
- src: use symbol to store `AsyncWrap` resource (Anna Henningsen)
  [#31745](#31745)

PR-URL: #34077
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 26, 2020

@BethGriggs
Copy link
Member Author

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@BethGriggs
Copy link
Member Author

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/858/
CITGM v12.x: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-nobuild/857/
CITGM results look good to me (raised nodejs/citgm#809 to cover through2 failures)

BethGriggs added a commit that referenced this pull request Jun 30, 2020
@BethGriggs BethGriggs merged commit 412c618 into v12.x Jun 30, 2020
BethGriggs added a commit that referenced this pull request Jun 30, 2020
Notable changes:

- deps: V8: backport fb26d0bb1835 (Matheus Marchini)
  [#33573](#33573)
- src: use symbol to store `AsyncWrap` resource (Anna Henningsen)
  [#31745](#31745)

PR-URL: #34077
BethGriggs added a commit to BethGriggs/nodejs.org that referenced this pull request Jun 30, 2020
BethGriggs added a commit to nodejs/nodejs.org that referenced this pull request Jun 30, 2020
@BethGriggs BethGriggs deleted the v12.18.2-proposal branch June 30, 2020 13:49
@targos targos added release Issues and PRs related to Node.js releases. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release Issues and PRs related to Node.js releases.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants