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

[v6.x] v8: handle proxy objects in MakeMirror() #14343

Closed
wants to merge 17 commits into from

Conversation

bnoordhuis
Copy link
Member

@bnoordhuis bnoordhuis commented Jul 18, 2017

Fix for #8224 and #10406, and the continuation of #10409.

First commit is a back-port of #10409 to v6.x, the second one is what I think is a better fix: it turns the proxy into a black box that cannot be inspected (because inspection has side-effects with proxied objects.)

Input welcome.

Node CI: https://ci.nodejs.org/job/node-test-pull-request/9215/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/792/

@nodejs-github-bot nodejs-github-bot added v6.x v8 engine Issues and PRs related to the V8 dependency. labels Jul 18, 2017
@MylesBorins
Copy link
Contributor

There seem to be quite a few failures in CI... I'm running a job against the staging branch to see if they are related to this PR

@MylesBorins
Copy link
Contributor

@targos
Copy link
Member

targos commented Jul 18, 2017

@MylesBorins you need #14219 to unbreak V8 CI

@MylesBorins
Copy link
Contributor

One more time with V8 fix https://ci.nodejs.org/job/node-test-commit-v8-linux/795/

@MylesBorins
Copy link
Contributor

So it looks like it is breaking during compilation... assuming that it is rebasing, then this change appears to be breaking stuff. @bnoordhuis would you be up for rebasing against v6.x-staging just to play it safe

@bnoordhuis
Copy link
Member Author

Rebased, although I get the impression it's a CI bug more than anything else. I saw this in the s390x build log:

/bin/sh: line 0: cd: ../deps/v8/tools/gyp: No such file or directory
python: can't open file '../../tools/js2c.py': [Errno 2] No such file or directory

Node CI: https://ci.nodejs.org/job/node-test-pull-request/9249/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/798/

@jasnell
Copy link
Member

jasnell commented Jul 19, 2017

fwiw, I've seen that pop up in CI from time to time. I agree it's likely not an issue with this PR

@addaleax addaleax changed the title v8: handle proxy objects in MakeMirror() [v6.x] v8: handle proxy objects in MakeMirror() Jul 20, 2017
@MylesBorins
Copy link
Contributor

@bnoordhuis do we land both fixes or only need to land one or the other?

/cc @nodejs/v8 for review

sam-github and others added 9 commits July 21, 2017 14:35
For consistency with 4.x and 8.x.

This commit also contains a forward port of
nodejs#14232 to confirm that 4.x and 6.x
behave identically with respect to the port argument.

PR-URL: nodejs#14234
Refs: nodejs#14205
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
This is an initial step to eliminate most of parsing errors.

Backport-PR-URL: nodejs#14067
PR-URL: nodejs#12563
Refs: nodejs#12557 (comment)
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Backport-PR-URL: nodejs#14067
PR-URL: nodejs#12563
Refs: nodejs#12557 (comment)
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
* Install [email protected]
* Add doc/.eslintrc.yaml
* Add `plugins: [markdown]` to the main .eslintrc.yaml
* .js files in doc folder added to .eslintignore
* Update Makefile, vcbuild.bat, and tools/jslint.js

Refs: nodejs#12563
Refs: nodejs#12640
Refs: nodejs#14047

PR-URL: nodejs#14067
Reviewed-By: James Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
The `no-useless-regex-char-class-escape` custom lint rule was introduced
as a less aggressive alternative to some enhancements that were
introduced into ESLint. Those enhancements were blocking us from
updating ESLint. However, they have since been relaxed and the custom
rule is no longer needed. Remove it.

Backport-PR-URL: nodejs#14360
PR-URL: nodejs#10561
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
In preparation for applying the more strict indentation linting
available in ESLint 4.0.0, correct minor indentation issues in
tools/eslint-rules/required-modules.js.

This is the only file with indentation that does not conform to the
stricter checks.

Backport-PR-URL: nodejs#14360
PR-URL: nodejs#13758
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
ESLint 4.0.0 provides stricter (and more granular) indentation checking
than previous versions. Apply the stricter indentation rules to the
tools directory.

Backport-PR-URL: nodejs#14360
PR-URL: nodejs#13758
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Fix previously-unnoticed typo in `required-modules.js`.

Backport-PR-URL: nodejs#14360
Refs: nodejs#13758 (comment)
PR-URL: nodejs#13758
Reviewed-By: Teddy Katz <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
* Remove needless RegExp flag

  In fixed case, `/g` flag is needless in the boolean context.

* Remove needless RegExp capturing

  Use non-capturing grouping or remove capturing completely when:

  * capturing is useless per se, e.g. in test() check;
  * captured groups are not used afterward at all;
  * some of the later captured groups are not used afterward.

* Use test, not match/exec in boolean context

  match() and exec() return a complicated object,
  unneeded in a boolean context.

* Do not needlessly repeat RegExp creation

  This commit takes RegExp creation out of cycles and other repetitions.

  As long as the RegExp does not use /g flag and match indices,
  we are safe here.

  In tests, this fix hardly gives a significant performance gain,
  but it increases clarity and maintainability,
  reassuring some RegExps to be identical.

  RegExp in functions are not taken out of their functions:
  while these functions are called many times
  and their RegExps are recreated with each call,
  the performance gain in test cases
  does not seem to be worth decreasing function self-dependency.

Backport-PR-URL: nodejs#14370
PR-URL: nodejs#13770
Reviewed-By: Colin Ihrig <[email protected]>
laverdet and others added 4 commits July 21, 2017 14:37
This option has been broken for almost a year when used with any of the
vm.runIn.. family of functions, except for syntax errors.

Backport-PR-URL: nodejs#14373
PR-URL: nodejs#13074
Reviewed-By: Anna Henningsen <[email protected]>
This fixes a race condition in the watchdog timer used for vm timeouts.
The condition would terminate the main stack's execution instead of the
code running under the sandbox.

Backport-PR-URL: nodejs#14373
PR-URL: nodejs#13074
Reviewed-By: Anna Henningsen <[email protected]>
Backport-PR-URL: nodejs#14416
PR-URL: nodejs#13206
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
This commit allows self signed certificates to work with
unix sockets by forwarding the rejectUnauthorized option.

Backport-PR-URL: nodejs#14415
Fixes: nodejs#13470
PR-URL: nodejs#13505
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
root_cert_store is defined as extern in node_crypto.h but only used in
node_crypto.cc. It is then set using SSL_CTX_set_cert_store. The only
usages of SSL_CTX_get_cert_store are in node_crypto.cc which would all
be accessing the same X509_STORE through the root_cert_store pointer as
far as I can tell. Am I missing something here?

This commit suggests removing it from the header and making it static
in node_crypto.cc.

PR-URL: nodejs#13194
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
@bnoordhuis
Copy link
Member Author

Okay, let's land both. Rebased again.

@MylesBorins
Copy link
Contributor

landed in d75363b...f3dfbeb

@MylesBorins
Copy link
Contributor

MylesBorins commented Aug 14, 2017

Had to back it out... the changes were breaking test-child-process-detached on AIX + vs2015

CI Job

not ok 56 parallel/test-child-process-detached
  ---
  duration_ms: 0.720
  severity: fail
  stack: |-
    
    assert.js:81
      throw new assert.AssertionError({
      ^
    AssertionError: Got unwanted exception..
        at _throws (assert.js:356:5)
        at Function.assert.doesNotThrow (assert.js:374:3)
        at process.<anonymous> (/home/iojs/build/workspace/node-test-commit-aix/nodes/aix61-ppc64/test/parallel/test-child-process-detached.js:22:10)
        at emitOne (events.js:101:20)
        at process.emit (events.js:188:7)
  ...

edit: I don't think this is related, am going to reland

@MylesBorins MylesBorins reopened this Aug 14, 2017
@MylesBorins MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from cebce8b to 5a93c16 Compare August 16, 2017 03:57
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
PR-URL: #14343
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
PR-URL: #14343
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

problem was unrelated relanded in d35e5c7...5a93c16

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.