Bug Fix for Issue #17668 - Unused prototype objects from V8 FunctionTemplate initialization #20321

brandontruggles · 2018-04-26T04:01:39Z

For issue #17668. I followed the advice of @TimothyGu to implement this fix. Please let me know if there are any issues, and I will address them as soon as possible.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes

9 JS tests here fail, although they fail when I make a fresh clone of the repo, so I do not believe they are failing due to my commits.

commit message follows commit guidelines
tests and/or benchmarks are included

AyushG3112 · 2018-04-26T07:10:12Z

You might need to change your commit messages to conform to the Commit Guidelines

bnoordhuis

Can you squash your commits and write a commit log using the format described in the contribution guide? Thanks.

bnoordhuis · 2018-04-26T07:16:24Z

test/parallel/test-env-newprotomethod-remove-unnecessary-prototypes.js

+.binding('udp_wrap').UDP.prototype.bind6.prototype),
+                   'undefined',
+                   'Unnecessary prototype "process.binding("udp_wrap").\
+UDP.prototype.bind6.prototype" was generated!');


Does this pass make lint?

I'd maybe drop the message, it's just restating the code as prose.

Perhaps additionally check that 'prototype' in UDP.prototype.bind6 is false.

Maybe check a few more methods, also from other binding objects (e.g. process.binding('tcp_wrap').TCP)

@bnoordhuis I have verified that my tests pass make lint. I also cut down the messages in my asserts to say only the names of the prototypes that they are testing, and nothing else, to avoid bloat. Furthermore, I added a few more method tests, including tests for process.binding('tcp_wrap').TCP, and some protoype in tests as well.

Please let me know if there are any further improvements needed.

TimothyGu

LGTM, assuming @bnoordhuis's comments get fixed.

TimothyGu · 2018-04-26T19:32:43Z

Should we run CITGM on this?

brandontruggles · 2018-04-27T01:50:25Z

@TimothyGu @bnoordhuis I just added a new squashed commit with the changes that @bnoordhuis requested. Please let me know if everything checks out OK. Thank you!

joyeecheung · 2018-04-27T12:04:57Z

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1387/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1388/

BridgeAR · 2018-04-29T14:12:11Z

test/parallel/test-env-newprotomethod-remove-unnecessary-prototypes.js

+require('../common/fixtures');
+
+// This test ensures that unnecessary prototypes are no longer
+// being genrated by Environment::NewFunctionTemplate


Nit: typo in generated. While being on it, please also punctuate the sentence (add a dot at the end).

BridgeAR · 2018-04-29T14:12:45Z

test/parallel/test-env-newprotomethod-remove-unnecessary-prototypes.js

@@ -0,0 +1,57 @@
+'use strict';
+require('../common');
+require('../common/fixtures');


Nit: unnecessary require (../common/fixtures).

BridgeAR · 2018-04-29T14:12:53Z

test/parallel/test-env-newprotomethod-remove-unnecessary-prototypes.js

+
+const assert = require('assert');
+
+


Nit: unnecessary double line breaks.

BridgeAR · 2018-04-29T14:19:43Z

test/parallel/test-env-newprotomethod-remove-unnecessary-prototypes.js

+
+assert.strictEqual((process
+.binding('udp_wrap').UDP.prototype.bind6.prototype), undefined,
+                   'process.binding("udp_wrap").UDP.prototype.bind6.prototype');


All the process.[...].prototype === undefined checks in here are redundant and should be removed.

'prototype' in process.[...] already checks this as well.

As soon as that is the case, the tests could be shortened to e.g.,

[ process.binding('udp_wrap').UDP.prototype.bind6, process.binding('tcp_wrap').TCP.prototype.bind6, ... ].forEach((binding, i) => { assert.strictEqual('prototype' in binding, false, `Test ${i} failed`); });

Added an optional parameter of type v8::ConstructorBehavior to Environment::NewFunctionTemplate, defaulting to v8::ConstructorBehavior::kAllow. Also modified Environment::SetProtoMethod to pass v8::ConstructorBehavior::kThrow to its call to Environment::NewFunctionTemplate. Fixes: nodejs#17668 Refs: nodejs#20321

brandontruggles · 2018-04-29T17:21:29Z

@BridgeAR I've updated my commit to reflect the changes you requested above. I've also ensured that the updated tests pass make test. Thank you for the feedback, and please let me know if there are further issues.

addaleax

Just reaffirming my earlier approval :)

BridgeAR · 2018-04-29T17:27:44Z

CI https://ci.nodejs.org/job/node-test-pull-request/14587/

addaleax · 2018-05-05T12:04:55Z

Landed in 64348f5 🎉 Congratulations to your first commit!

Added an optional parameter of type v8::ConstructorBehavior to Environment::NewFunctionTemplate, defaulting to v8::ConstructorBehavior::kAllow. Also modified Environment::SetProtoMethod to pass v8::ConstructorBehavior::kThrow to its call to Environment::NewFunctionTemplate. Fixes: #17668 Refs: #20321 PR-URL: #20321 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 26, 2018

addaleax approved these changes Apr 26, 2018

View reviewed changes

bnoordhuis reviewed Apr 26, 2018

View reviewed changes

TimothyGu approved these changes Apr 26, 2018

View reviewed changes

brandontruggles force-pushed the brandon-prototype-kthrow branch from 9aac6b7 to f1cf34e Compare April 27, 2018 01:15

brandontruggles force-pushed the brandon-prototype-kthrow branch from f1cf34e to b2e21f2 Compare April 27, 2018 01:37

jasnell approved these changes Apr 27, 2018

View reviewed changes

bnoordhuis approved these changes Apr 28, 2018

View reviewed changes

BridgeAR reviewed Apr 29, 2018

View reviewed changes

brandontruggles force-pushed the brandon-prototype-kthrow branch from b2e21f2 to 18108b4 Compare April 29, 2018 17:18

addaleax approved these changes Apr 29, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2018

Trott mentioned this pull request Apr 30, 2018

doc: remove squash guideline from onboarding doc #20413

Closed

2 tasks

addaleax added the dont-land-on-v8.x label May 5, 2018

addaleax closed this May 5, 2018

MylesBorins mentioned this pull request May 8, 2018

v10.1.0 proposal #20606

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug Fix for Issue #17668 - Unused prototype objects from V8 FunctionTemplate initialization #20321

Bug Fix for Issue #17668 - Unused prototype objects from V8 FunctionTemplate initialization #20321

brandontruggles commented Apr 26, 2018 •

edited

Loading

AyushG3112 commented Apr 26, 2018

bnoordhuis left a comment

bnoordhuis Apr 26, 2018

brandontruggles Apr 27, 2018

TimothyGu left a comment

TimothyGu commented Apr 26, 2018

brandontruggles commented Apr 27, 2018

joyeecheung commented Apr 27, 2018 •

edited

Loading

BridgeAR Apr 29, 2018 •

edited

Loading

BridgeAR Apr 29, 2018

BridgeAR Apr 29, 2018

BridgeAR Apr 29, 2018

brandontruggles commented Apr 29, 2018

addaleax left a comment

BridgeAR commented Apr 29, 2018

addaleax commented May 5, 2018

Bug Fix for Issue #17668 - Unused prototype objects from V8 FunctionTemplate initialization #20321

Bug Fix for Issue #17668 - Unused prototype objects from V8 FunctionTemplate initialization #20321

Conversation

brandontruggles commented Apr 26, 2018 • edited Loading

Checklist

AyushG3112 commented Apr 26, 2018

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis Apr 26, 2018

Choose a reason for hiding this comment

brandontruggles Apr 27, 2018

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu commented Apr 26, 2018

brandontruggles commented Apr 27, 2018

joyeecheung commented Apr 27, 2018 • edited Loading

BridgeAR Apr 29, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Apr 29, 2018

Choose a reason for hiding this comment

BridgeAR Apr 29, 2018

Choose a reason for hiding this comment

BridgeAR Apr 29, 2018

Choose a reason for hiding this comment

brandontruggles commented Apr 29, 2018

addaleax left a comment

Choose a reason for hiding this comment

BridgeAR commented Apr 29, 2018

addaleax commented May 5, 2018

brandontruggles commented Apr 26, 2018 •

edited

Loading

joyeecheung commented Apr 27, 2018 •

edited

Loading

BridgeAR Apr 29, 2018 •

edited

Loading