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

[8.x] deps: backport V8 memory fixes #21269

Closed
wants to merge 51 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Jun 11, 2018

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

subsystem: deps/v8

Fixes: #21021

V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1418/
CI: https://ci.nodejs.org/job/node-test-pull-request/15400/

Peter Marton and others added 30 commits May 15, 2018 17:26
This adds the optional options argument to `http.createServer()`.
It contains two options: the `IncomingMessage` and `ServerReponse`
option.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
This adds the Http1IncomingMessage and Http1ServerReponse options
to http2.createServer().

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15752
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Add optional Http2ServerRequest and Http2ServerResponse options
to createServer and createSecureServer. Allows custom req & res
classes that extend the default ones to be used without
overriding the prototype.

Backport-PR-URL: nodejs#20456
PR-URL: nodejs#15560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: nodejs#20456
PR-URL: nodejs#18872
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
When configure with --debug-http2 --debug-nghttp2 the following
compilation error is generated:

DEBUG_HTTP2SESSION2(this, "fatal error receiving data: %d", ret);
                          ^
../src/node_http2.cc:1690:27:
error: invalid use of 'this' outside of a non-static member function

1 errors generated.

OnStreamReadImpl is static and I think the intention was to pass in the
session variable here.

PR-URL: nodejs#20815
Refs: nodejs#20806
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
by using package-lock.json

PR-URL: nodejs#16945
Fixes: nodejs#16628
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Khaidi Chu <i@2333.moe>
Reviewed-By: James M Snell <jasnell@gmail.com>
The error check doesn't matter because a failure would be ignored
as part of the loop condition.

PR-URL: nodejs#16950
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Allow the user to specify the filepath for the trace_events log file
using a template string.

Backport-PR-URL: nodejs#19145
PR-URL: nodejs#18480
Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Before these changes, only V8 added postmortem metadata to Node's
binary, limiting the possibilities for debugger's developers to add some
features that rely on investigating Node's internal structures.

These changes are first steps towards empowering debug tools to
navigate Node's internal structures. One example of what can be
achieved with this is shown at nodejs/llnode#122 (a command which prints
information about handles and requests on the queue for a core dump
file). Node postmortem metadata are prefixed with nodedbg_.

This also adds tests to validate if all postmortem metadata are
calculated correctly, plus some documentation on what is postmortem
metadata and a few care to be taken to avoid breaking it.

Ref: nodejs/llnode#122
Ref: nodejs/post-mortem#46

Backport-PR-URL: nodejs#19176
PR-URL: nodejs#14901
Refs: nodejs/post-mortem#46
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Backport-PR-URL: nodejs#19176
PR-URL: nodejs#18530
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Redefining private breaks any private inheritance in the
included files. We can simply declare GenDebugSymbols()
as friends in related classes to gain the access that we need.

Backport-PR-URL: nodejs#19176
PR-URL: nodejs#18653
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add a errno -> [error code, uv error message] map to the uv binding
so the error message can be assembled in the JS layer.

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#17338
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reimplement uv.errname() as internal/util.getSystemErrorName() to
avoid the memory leaks caused by unknown error codes
and avoid calling into C++ for the error names. Also
expose it as a public API for external use.

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18186
Refs: http://docs.libuv.org/en/v1.x/errors.html#c.uv_err_name
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18358
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18546
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit moves error creation helpers scattered around
under lib/ into lib/internal/errors.js in the hope of being clearer
about the differences of errors that we throw into the user land.

- Move util._errnoException and util._exceptionWithHostPort
  into internal/errors.js and simplify their logic so it's
  clearer what the properties these helpers create.
- Move the errnoException helper in dns.js to internal/errors.js
  into internal/errors.js and rename it to dnsException. Simplify
  it's logic so it no longer calls errnoException and skips
  the unnecessary argument checks.

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#18546
Reviewed-By: James M Snell <jasnell@gmail.com>
A error message should always be non-enumerable. This makes sure
that is true for dns errors as well. It also adds another check
in `common.expectsError` to make sure no other regressions are
introduced going forward.

Fixes nodejs#19716

Backport-PR-URL: nodejs#19191
PR-URL: nodejs#19719
Fixes: nodejs#19716
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Currently this test will overwrite the clientOpts object with the port,
instead of setting the port property on the clientOpts object which
looks like the original intent.

Doing this the test fails reporting that the fake-cnnic-root-cert has
expired. This is indeed true:
$ openssl x509 -in test/fixtures/keys/fake-cnnic-root-cert.pem \
-text -noout
Certificate:
        ...
        Validity
            Not Before: Jun  9 17:15:16 2015 GMT
            Not After : Mar 29 17:15:16 2018 GMT

This commit sets the errorCode to CERT_HAS_EXPIRED. I tried updating the
certificate using test/fixtures/keys/Makefile but then no error is
thrown and I'm currently looking into this.

Backport-PR-URL: nodejs#20776
PR-URL: nodejs#19767
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
I looks like this test has not worked as expected since commit
2bc7841 ("test: use random ports
where possible"). The test in that commit checked for `CERT_REVOKED`
which was returned by CheckWhitelistedServerCert.

CheckWhitelistedServerCert was later removed in commit
6ee4228 ("src: drop CNNIC+StartCom
certificate whitelisting").

I'm suggesting that this test case be removed as I don't think it is
valid anymore.

Backport-PR-URL: nodejs#20776
PR-URL: nodejs#19767
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
I've not been able to find any reason for calling
BIO_set_shutdown(bio, 1). This is done by default for the following
versions of OpenSSL:

https://github.com/openssl/openssl/blob/OpenSSL_1_1_0/
crypto/bio/bio_lib.c#L26

https://github.com/openssl/openssl/blob/OpenSSL_1_0_1/
crypto/bio/bio_lib.c#L90

https://github.com/openssl/openssl/blob/OpenSSL_1_0_2/
crypto/bio/bio_lib.c#L88

https://github.com/openssl/openssl/blob/OpenSSL_1_0_0/
crypto/bio/bio_lib.c#L90

This commit removes the call and the comment.

PR-URL: nodejs#17542
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs#17587
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
`tls.Socket` does not exist, and the deprecation message
should refer to `tls.TLSSocket` (like the documentation
for the deprecation message already does).

PR-URL: nodejs#17561
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#17432
Fixes: nodejs#17430
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#17558
Fixes: nodejs#17540
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Percent-encoded additional characters in fragment state with new
FRAGMENT_ENCODE_SET lookup table. The fragment percent-encode set
includes the C0 control percent-encode set and code points U+0020,
U+0022, U+003C, U+003E, and U+0060.

PR-URL: nodejs#17627
Fixes: nodejs#17540
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
This commit adds a test to validate postmortem debugging metadata.
When this test runs, it can check for the presence of metadata
constants used by tools such as llnode and mdb and report if any
have accidentally been removed.

PR-URL: nodejs#17685
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Is safer to use a `process.binding(config)` defined boolean, than to
regex on `process.execArgv`. Also, this better falls in line with the
conventions of checking flags passed to the executable.

PR-URL: nodejs#17814
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Use an interval to keep the event loop open so the test does not exit
before receiving all signals fom asynchronous `exec()` calls.

PR-URL: nodejs#17827
Fixes: nodejs#14070
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
PR-URL: nodejs#17939
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#17939
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Original commit message:
  [heap] Activate memory reducer on external memory activity.

  BUG=chromium:728228,chromium:626082
  CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

  Review-Url: https://codereview.chromium.org/2917853004
  Cr-Commit-Position: refs/heads/master@{nodejs#45671}

Refs: nodejs#21021
Original commit message:
  [heap] Lower external allocation limit when external memory shrinks.

  BUG=chromium:728228
  CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

  Review-Url: https://codereview.chromium.org/2921883002
  Cr-Commit-Position: refs/heads/master@{nodejs#45726}

Fixes: nodejs#21021
@nodejs-github-bot nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Jun 11, 2018
@ofrobots ofrobots changed the title Backport/21021 [8.x] deps: backport V8 memory fixes Jun 11, 2018
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGMT

@MylesBorins
Copy link
Contributor

landed in f493001...859dc64

MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Original commit message:
  [heap] Activate memory reducer on external memory activity.

  BUG=chromium:728228,chromium:626082
  CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

  Review-Url: https://codereview.chromium.org/2917853004
  Cr-Commit-Position: refs/heads/master@{#45671}

PR-URL: #21269
Fixes: #21021
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
Original commit message:
  [heap] Lower external allocation limit when external memory shrinks.

  BUG=chromium:728228
  CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

  Review-Url: https://codereview.chromium.org/2921883002
  Cr-Commit-Position: refs/heads/master@{#45726}

PR-URL: #21269
Fixes: #21021
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@ofrobots ofrobots deleted the backport/21021 branch June 14, 2018 21:59
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Original commit message:
  [heap] Activate memory reducer on external memory activity.

  BUG=chromium:728228,chromium:626082
  CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

  Review-Url: https://codereview.chromium.org/2917853004
  Cr-Commit-Position: refs/heads/master@{#45671}

PR-URL: #21269
Fixes: #21021
Reviewed-By: Myles Borins <myles.borins@gmail.com>
rvagg pushed a commit that referenced this pull request Aug 16, 2018
Original commit message:
  [heap] Lower external allocation limit when external memory shrinks.

  BUG=chromium:728228
  CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_rel_ng

  Review-Url: https://codereview.chromium.org/2921883002
  Cr-Commit-Position: refs/heads/master@{#45726}

PR-URL: #21269
Fixes: #21021
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@addaleax
Copy link
Member

addaleax commented Nov 13, 2018

Just fyi, d868eb7 changed the ABI, so that addons compiled with >=8.12.0 don’t run on <8.12.0… I think that’s the kind of thing we want to call out in the changelog, because it means addon authors (like me) need to explicitly specify that they want to build and test against old versions of Node for pre-built binaries.

@Trott
Copy link
Member

Trott commented Nov 13, 2018

@addaleax Does that make it a breaking change? Should it be rolled back?

@ofrobots
Copy link
Contributor Author

@addaleax I'm was bit lost and was not able to understand how d868eb7 broke ABI. But as part of writing this response, I think I understand it now. Addition of non-virtual methods is not ABI breaking typically (code compiled previously continues to work with new release. Same code compiled with new release will continue to work with old releases since it doesn't use the new function).

The problem here is that the same change uses the newly added method internally from Isolate::AdjustAmountOfExternalAllocatedMemory which now transitively exposes the new ABI to the users to Isolate::AdjustAmountOfExternalAllocatedMemory .

I think this exposes a gap that we aren't able to test for ABI compatibility very robustly. I don't think I would have caught it even after thinking about it more carefully, since I still couldn't get it until now 😢.

I think we should fix this. Here are some proposals:

  1. Roll it back as suggested by @Trott . I do not believe there will be much use of CheckMemoryPressure at this point, so this should be safe.
  2. Move the implementation of CheckMemoryPressure to the header.

I prefer the latter, but am open to other opinions.

@addaleax
Copy link
Member

@ofrobots Yeah, I think the latter also works well enough to try it and should resolve these issues fully… go for it? :)

@refack
Copy link
Contributor

refack commented Nov 15, 2018

I think this exposes a gap that we aren't able to test for ABI compatibility very robustly. I don't think I would have caught it even after thinking about it more carefully, since I still couldn't get it until now 😢.

I think it shouldn't be too difficult to devise a CI job for this (build the addons with PR code, then replace node binary with LKGR for relevant branch, and test)

@ofrobots
Copy link
Contributor Author

@refack the challenge will be that this probably may only show up (depending on the compiler) if tested against a native module that references Isolate::AdjustAmountOfExternalAllocatedMemory.

@ofrobots
Copy link
Contributor Author

Moving CheckMemoryPressure to the header is not that straightforward. I think we have to do a revert here.

@refack
Copy link
Contributor

refack commented Nov 15, 2018

the challenge will be that this probably may only show up (depending on the compiler) if tested against a native module that references

Well Nan::AdjustExternalMemory will do it (so yeah it's an issue of covrage 🤷‍♂️)

inline int AdjustExternalMemory(int bc) {
    return static_cast<int>(
        v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(bc));
  }

https://github.com/nodejs/nan/blob/2e5ed4fc3a8ac80a6ef1f2a55099ab3ac8800dc6/nan.h#L700-L703

@ofrobots ofrobots self-assigned this Nov 19, 2018
@ofrobots ofrobots mentioned this pull request Nov 20, 2018
2 tasks
@ofrobots
Copy link
Contributor Author

Here's the best solution I could come up with: #24499

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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.