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

Node must not rely on functionality exposed through --allow-natives-syntax #20409

Closed
hashseed opened this issue Apr 29, 2018 · 3 comments
Closed

Node must not rely on functionality exposed through --allow-natives-syntax #20409

hashseed opened this issue Apr 29, 2018 · 3 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@hashseed
Copy link
Member

As I found out here, Node enables --allow-natives-syntax in V8 during bootstrapping and disables later.

This is so v8.previewMapIterator and v8.previewSetIterator in lib/internal/v8.js can be compiled during bootstrapping, as the call %MapIteratorClone and %SetIteratorClone. There are two major flaws in this:

  • This relies on v8/src/runtime/runtime.h defining %MapIteratorClone. Note that this is an implementation detail deep inside of V8 and could change at any time without any warning. In this particular case, %MapIteratorClone is only used inside V8 for the outdated debug context implementation that we were planning to remove for a long time.
  • This also relies on the fact that once compiled, the function does not need to be compiled again. That could change in the future if V8 implements code aging for bytecode.

Well I guess now that V8 tests against Node, V8 developers are forced to not violate these wrong assumptions.

Please please remove these dependencies.
@addaleax addaleax added the v8 engine Issues and PRs related to the V8 dependency. label Apr 29, 2018
@addaleax
Copy link
Member

I think we can be very empathetic to this, because it’s a problem that we’ve been running into far too many times ourselves :)

Do you have any suggestions for how we’d best avoid this? We only use it for inspecting these objects at the moment in console.log()/util.inspect(). I guess we could use the inspector bindings, but maybe there’s a better way to achieve that goal?

Also, one more note: We do use --allow_natives_syntax in test/parallel/test-http-same-map for %HaveSameMap. I assume the same holds there as well?

@bnoordhuis
Copy link
Member

This one's on me, I'm afraid... the use of natives was only a stop-gap measure. It's on my todo list to file CLs to add a proper C++ API for inspecting map and set iterators, but I haven't gotten around to it yet.

kisg pushed a commit to paul99/v8mips that referenced this issue May 17, 2018
@addaleax
[api] Expose PreviewEntries as public API
ff0a979 
Turn `debug::EntriesPreview` into a public API.
This is a straightforward approach to addressing
nodejs/node#20409
(not relying on functionality behind `--allow-natives-syntax`)
in Node.js.

Refs: nodejs/node#20409
Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I7021e5846012a55a82c488408ded6591f6b139e7
Reviewed-on: https://chromium-review.googlesource.com/1057467
Reviewed-by: Yang Guo <[email protected]>
Commit-Queue: Yang Guo <[email protected]>
Cr-Commit-Position: refs/heads/master@{#53226}
addaleax added a commit to addaleax/node that referenced this issue May 18, 2018
@addaleax
lib,src: use V8 API for collection inspection
faaf08e 
Use a new public V8 API for inspecting weak collections and
collection iterators, rather than using V8-internal functions
to achieve this. This currently comes with a slight modification of
the output for inspecting iterators generated by `Set().entries()`.

Fixes: nodejs#20409
addaleax added a commit to addaleax/node that referenced this issue May 18, 2018
@addaleax
deps: cherry-pick ff0a9793334 from upstream V8
0babe5a 
Original commit message:

    [api] Expose PreviewEntries as public API

    Turn `debug::EntriesPreview` into a public API.
    This is a straightforward approach to addressing
    nodejs#20409
    (not relying on functionality behind `--allow-natives-syntax`)
    in Node.js.

Refs: v8/v8@ff0a979
addaleax added a commit to addaleax/node that referenced this issue May 18, 2018
@addaleax
lib,src: use V8 API for collection inspection
1d660fe 
Use a new public V8 API for inspecting weak collections and
collection iterators, rather than using V8-internal functions
to achieve this. This currently comes with a slight modification of
the output for inspecting iterators generated by `Set().entries()`.

Fixes: nodejs#20409
addaleax added a commit that referenced this issue May 18, 2018
@addaleax
deps: cherry-pick ff0a9793334 from upstream V8
143a2f8 
Original commit message:

    [api] Expose PreviewEntries as public API

    Turn `debug::EntriesPreview` into a public API.
    This is a straightforward approach to addressing
    #20409
    (not relying on functionality behind `--allow-natives-syntax`)
    in Node.js.

Refs: v8/v8@ff0a979

PR-URL: #20719
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@hashseed
Copy link
Member Author

Thanks for fixing this!

MylesBorins pushed a commit that referenced this issue May 22, 2018
@addaleax @MylesBorins
deps: cherry-pick ff0a9793334 from upstream V8
e56716e 
Original commit message:

    [api] Expose PreviewEntries as public API

    Turn `debug::EntriesPreview` into a public API.
    This is a straightforward approach to addressing
    #20409
    (not relying on functionality behind `--allow-natives-syntax`)
    in Node.js.

Refs: v8/v8@ff0a979

PR-URL: #20719
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue May 22, 2018
@addaleax @MylesBorins
lib,src: use V8 API for collection inspection
fb7a775 
Use a new public V8 API for inspecting weak collections and
collection iterators, rather than using V8-internal functions
to achieve this. This currently comes with a slight modification of
the output for inspecting iterators generated by `Set().entries()`.

Fixes: #20409

PR-URL: #20719
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
alexeykuzmin added a commit to electron/libchromiumcontent that referenced this issue May 25, 2018
@alexeykuzmin
Backport ff0a97933 from V8
b2863b2 
Node.js 10.2.0 needs it.

https://chromium-review.googlesource.com/1057467
nodejs/node#20409
targos pushed a commit to targos/node that referenced this issue May 31, 2018
@addaleax @targos
deps: cherry-pick ff0a9793334 from upstream V8
82368b3 
Original commit message:

    [api] Expose PreviewEntries as public API

    Turn `debug::EntriesPreview` into a public API.
    This is a straightforward approach to addressing
    nodejs#20409
    (not relying on functionality behind `--allow-natives-syntax`)
    in Node.js.

Refs: v8/v8@ff0a979

PR-URL: nodejs#20719
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 1, 2018
@addaleax @MylesBorins
deps: cherry-pick ff0a9793334 from upstream V8
109ba58 
Original commit message:

    [api] Expose PreviewEntries as public API

    Turn `debug::EntriesPreview` into a public API.
    This is a straightforward approach to addressing
    #20409
    (not relying on functionality behind `--allow-natives-syntax`)
    in Node.js.

Refs: v8/v8@ff0a979

PR-URL: #20719
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this issue Jun 1, 2018
@addaleax @MylesBorins
deps: cherry-pick ff0a9793334 from upstream V8
fb26861 
Original commit message:

    [api] Expose PreviewEntries as public API

    Turn `debug::EntriesPreview` into a public API.
    This is a straightforward approach to addressing
    #20409
    (not relying on functionality behind `--allow-natives-syntax`)
    in Node.js.

Refs: v8/v8@ff0a979

PR-URL: #20719
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@bnoordhuis @addaleax @hashseed