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

ofrobots · 2018-06-11T20:41:59Z

Checklist

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

subsystem: deps/v8

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/

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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

This adds the Http1IncomingMessage and Http1ServerReponse options to http2.createServer(). Backport-PR-URL: nodejs#20456 PR-URL: nodejs#15752 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Evan Lucas <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Backport-PR-URL: nodejs#20456 PR-URL: nodejs#18872 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

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 <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

by using package-lock.json PR-URL: nodejs#16945 Fixes: nodejs#16628 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Khaidi Chu <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]>

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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

Backport-PR-URL: nodejs#19176 PR-URL: nodejs#18530 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

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 <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Backport-PR-URL: nodejs#19191 PR-URL: nodejs#18358 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>

Backport-PR-URL: nodejs#19191 PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>

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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs#17587 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

`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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#17432 Fixes: nodejs#17430 Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs#17558 Fixes: nodejs#17540 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Richard Lau <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

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 <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

PR-URL: nodejs#17939 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]>

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 · 2018-06-11T20:42:01Z

@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/44/pipeline

MylesBorins

LGMT

MylesBorins · 2018-06-14T20:27:36Z

landed in f493001...859dc64

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

addaleax · 2018-11-13T22:16:44Z

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 · 2018-11-13T23:54:22Z

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

ofrobots · 2018-11-15T17:07:37Z

@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:

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.
Move the implementation of CheckMemoryPressure to the header.

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

addaleax · 2018-11-15T17:12:16Z

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

refack · 2018-11-15T17:51:38Z

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 · 2018-11-15T19:11:30Z

@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 · 2018-11-15T20:29:53Z

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

refack · 2018-11-15T21:09:27Z

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 · 2018-11-20T01:14:27Z

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

Peter Marton and others added 30 commits May 15, 2018 17:26

doc: fix typo in http2.md

0829b54

Backport-PR-URL: nodejs#20456 PR-URL: nodejs#18872 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

test: fix cctest -Wunused-variable warning

d27ca6d

Backport-PR-URL: nodejs#19176 PR-URL: nodejs#18530 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

errors: lazy load util in internal/errors.js

05076ac

Backport-PR-URL: nodejs#19191 PR-URL: nodejs#18358 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jon Moss <[email protected]>

util: skip type checks in internal getSystemErrorName

de86203

Backport-PR-URL: nodejs#19191 PR-URL: nodejs#18546 Reviewed-By: James M Snell <[email protected]>

tools: don't lint-md as part of main lint target

4512116

PR-URL: nodejs#17587 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Myles Borins <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

src: make FSEventWrap/StatWatcher::Start more robust

4cd0ab5

PR-URL: nodejs#17432 Fixes: nodejs#17430 Reviewed-By: James M Snell <[email protected]>

url: added space to class string of iterator objects

e0a780b

PR-URL: nodejs#17558 Fixes: nodejs#17540 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]>

src: remove duplicate words in comments

14f29e3

PR-URL: nodejs#17939 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]>

http2: remove duplicate words in comments

b046412

PR-URL: nodejs#17939 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jon Moss <[email protected]> Reviewed-By: James M Snell <[email protected]>

ofrobots added 2 commits June 11, 2018 13:40

nodejs-github-bot added v8 engine Issues and PRs related to the V8 dependency. v8.x labels Jun 11, 2018

ofrobots changed the title ~~Backport/21021~~ [8.x] deps: backport V8 memory fixes Jun 11, 2018

ofrobots mentioned this pull request Jun 11, 2018

External memory leak on node v8.x #21021

Closed

MylesBorins approved these changes Jun 14, 2018

View reviewed changes

MylesBorins force-pushed the v8.x-staging branch from 084ef60 to 859dc64 Compare June 14, 2018 20:27

MylesBorins closed this Jun 14, 2018

ofrobots deleted the backport/21021 branch June 14, 2018 21:59

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

addaleax mentioned this pull request Nov 13, 2018

Regression 4.0.1 -> 4.0.2 addaleax/lzma-native#71

Closed

ofrobots self-assigned this Nov 19, 2018

ofrobots mentioned this pull request Nov 20, 2018

[v8.x] Fix abi break #24499

Closed

2 tasks

laverdet mentioned this pull request Nov 21, 2018

Error installing 3.1.0 on linux laverdet/node-fibers#398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ofrobots commented Jun 11, 2018 •

edited

Loading

nodejs-github-bot commented Jun 11, 2018

MylesBorins left a comment

MylesBorins commented Jun 14, 2018

addaleax commented Nov 13, 2018 •

edited

Loading

Trott commented Nov 13, 2018

ofrobots commented Nov 15, 2018

addaleax commented Nov 15, 2018

refack commented Nov 15, 2018 •

edited

Loading

ofrobots commented Nov 15, 2018

ofrobots commented Nov 15, 2018

refack commented Nov 15, 2018

ofrobots commented Nov 20, 2018

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

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

Conversation

ofrobots commented Jun 11, 2018 • edited Loading

Checklist

nodejs-github-bot commented Jun 11, 2018

MylesBorins left a comment

Choose a reason for hiding this comment

MylesBorins commented Jun 14, 2018

addaleax commented Nov 13, 2018 • edited Loading

Trott commented Nov 13, 2018

ofrobots commented Nov 15, 2018

addaleax commented Nov 15, 2018

refack commented Nov 15, 2018 • edited Loading

ofrobots commented Nov 15, 2018

ofrobots commented Nov 15, 2018

refack commented Nov 15, 2018

ofrobots commented Nov 20, 2018

ofrobots commented Jun 11, 2018 •

edited

Loading

addaleax commented Nov 13, 2018 •

edited

Loading

refack commented Nov 15, 2018 •

edited

Loading