GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — v8: add a js class for Serializer/Dserializer by zimbabao · Pull Request #13541 · nodejs/node

Conversation

zimbabao commented Jun 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.

Fixes: #13326

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes (test failing on master too)
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          v8: add a js class for Serializer/Dserializer

b39baec

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.

Fixes: nodejs#13326

bnoordhuis commented Jun 8, 2017

Copy link

Copy Markdown

Member

Can you add some regression tests?

There were some gotchas around new.target and native code but I forgot the details. It would be interesting to see if class S extends v8.Serializer works and what instance.method.call({}) does.

I expect it throws an 'illegal invocation' exception but it would be good to verify that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax approved these changes

Jun 8, 2017

View reviewed changes

addaleax commented Jun 8, 2017

Copy link

Copy Markdown

Member

interesting to see if class S extends v8.Serializer works

It does, we already extend it :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis commented Jun 8, 2017

Copy link

Copy Markdown

Member

I mean when you extend the class that this PR introduces.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TimothyGu commented Jun 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

@bnoordhuis, the classes this PR introduces are already extended further down the file by DefaultSerializer and DefaultDeserializer.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bnoordhuis commented Jun 8, 2017

Copy link

Copy Markdown

Member

Okay. Regression tests for the other issues would still be good though.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mscdex added the v8 engine Issues and PRs related to the V8 dependency. label

Jun 8, 2017


          v8: add a js class for Serializer/Dserializer

90c50a6

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.

Fixes: nodejs#13326

zimbabao commented Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

@bnoordhuis : Added regression tests.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

cjihrig requested changes

Jun 8, 2017

View reviewed changes

Comment thread

test/parallel/test-v8-serdes.js Outdated

		}

		{
		try {

cjihrig Jun 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you use assert.throws() for these.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zimbabao Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig : Made suggested changes.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          v8: add a js class for Serializer/Dserializer

e09951d

Calling Serializer/Deserlizer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added refression tests.

Fixes: nodejs#13326

cjihrig reviewed

Jun 8, 2017

View reviewed changes

Comment thread

test/parallel/test-v8-serdes.js Outdated

		v8.Serializer();
		},
		Error,
		"Class constructor Serializer cannot be invoked without 'new'"

cjihrig Jun 8, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you do something more like this:

assert.throws(
  () => { v8.Serializer(); },
  /^Error: Class constructor Serializer cannot be invoked without 'new'$/
);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zimbabao Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@cjihrig : Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zimbabao force-pushed the fix-serlializer-crash branch from 88817fa to b72ec9c Compare

June 8, 2017 18:25

cjihrig approved these changes

Jun 8, 2017

View reviewed changes

mscdex commented Jun 8, 2017

Copy link

Copy Markdown

Contributor

There are typos in the commit message(s): s/Dserializer/Deserializer/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zimbabao force-pushed the fix-serlializer-crash branch from b72ec9c to cfb934e Compare

Uh oh!

There was an error while loading. Please reload this page.


          v8: add a js class for Serializer/Deserializer

9643c6c

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: nodejs#13326

zimbabao force-pushed the fix-serlializer-crash branch from cfb934e to 9643c6c Compare

June 8, 2017 23:20

zimbabao commented Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

@mscdex : Fixed on top review.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mscdex commented Jun 9, 2017

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax approved these changes

Jun 9, 2017

View reviewed changes

addaleax left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Still LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasnell reviewed

Jun 9, 2017

View reviewed changes

Comment thread

lib/v8.js Outdated


		class Serializer extends serdesBindings.Serializer {}

		class Deserializer extends serdesBindings.Deserializer {}

jasnell Jun 9, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'd prefer something like:

const { Serializer: _Serializer, Deserializer: _Deserializer } = process.binding('serdes');
// ...

class Serializer extends _Serializer { }
class Deserializer extends _Deserializer { }

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zimbabao Jun 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will resend.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zimbabao Jun 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@jasnell : Made changes and added a note.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasnell reviewed

Jun 9, 2017

View reviewed changes

Comment thread

test/parallel/test-v8-serdes.js

		() => { v8.Deserializer(); },
		/^TypeError: Class constructor Deserializer cannot be invoked without 'new'$/
		);
		}

jasnell Jun 9, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should likely also add a note to the documentation clarifying the requirement

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

refack reacted with thumbs up emoji

All reactions

👍 1 reaction


          v8: add a js class for Serializer/Deserializer

955a1bb

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: nodejs#13326

zimbabao force-pushed the fix-serlializer-crash branch from 47cccbe to 955a1bb Compare

June 9, 2017 17:57

TimothyGu reviewed

Jun 10, 2017

View reviewed changes

Comment thread

lib/v8.js Outdated

		const { Buffer } = require('buffer');
		const { Serializer, Deserializer } = process.binding('serdes');
		const {
		Serializer: _Serializer,

TimothyGu Jun 10, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Can you indent two spaces only?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

zimbabao Jun 10, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done. BTW jslint didn't catch that error. May be we should add a rule?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          v8: add a js class for Serializer/Deserializer

1de429c

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: nodejs#13326

refack approved these changes

Jun 10, 2017

View reviewed changes

refack left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM % docs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

refack commented Jun 10, 2017

Copy link

Copy Markdown

Contributor

arm: #13603
osx: #13527
linux: infrastructure fail https://ci.nodejs.org/job/node-test-commit-linux/10495/nodes=ubuntu1604_docker_alpine34-64/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax commented Jun 12, 2017

Copy link

Copy Markdown

Member

Landed in 12fd63d, thanks for the PR!

refack reacted with hooray emoji

All reactions

🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax closed this

Jun 12, 2017

addaleax pushed a commit that referenced this pull request

Jun 12, 2017


          v8: add a js class for Serializer/Dserializer

12fd63d

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: #13326
PR-URL: #13541
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

addaleax pushed a commit that referenced this pull request

Jun 12, 2017


          v8: add a js class for Serializer/Dserializer

38a1cfb

Calling Serializer/Deserializer without new crashes node.
Adding a js class which just inherits cpp bindings.
Added regression tests.

Fixes: #13326
PR-URL: #13541
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>

addaleax mentioned this pull request

Jun 12, 2017

v8.1.1 proposal #13598

Merged

gibfahn mentioned this pull request

Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

2 of 3 tasks

addaleax added dont-land-on-v4.x labels

Jul 10, 2017

ExE-Boss mentioned this pull request

Dec 17, 2020

v8: fix native serdes constructors #36549

Closed

3 tasks done

ExE-Boss added a commit to ExE-Boss/node that referenced this pull request

Dec 17, 2020


          v8: Fix native serdes constructors

3a8ce4d

Fixes: nodejs#13326

Refs: nodejs#13541

addaleax pushed a commit that referenced this pull request

Dec 22, 2020


          v8: fix native serdes constructors

d90fa19

Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

danielleadams pushed a commit that referenced this pull request

Jan 12, 2021


          v8: fix native serdes constructors

Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

targos pushed a commit that referenced this pull request

May 1, 2021


          v8: fix native serdes constructors

ba4788d

Fixes: #13326
Refs: #13541
PR-URL: #36549
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>

Reviewers

jasnell

jasnell left review comments

addaleax

addaleax approved these changes

cjihrig

cjihrig approved these changes

+2 more reviewers

TimothyGu

TimothyGu left review comments

refack

refack approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

v8 engine

Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[8.0.0] v8.Serializer crashes on bad input

Uh oh!

There was an error while loading. Please reload this page.

8 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

zimbabao commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

bnoordhuis commented Jun 8, 2017

Uh oh!

addaleax commented Jun 8, 2017

Uh oh!

bnoordhuis commented Jun 8, 2017

Uh oh!

TimothyGu commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bnoordhuis commented Jun 8, 2017

Uh oh!

zimbabao commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jun 8, 2017

Uh oh!

zimbabao commented Jun 8, 2017

Uh oh!

zimbabao commented Jun 8, 2017

Uh oh!

mscdex commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zimbabao commented Jun 8, 2017

Uh oh!

mscdex commented Jun 9, 2017

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Jun 10, 2017

Uh oh!

zimbabao commented Jun 10, 2017

Uh oh!

addaleax commented Jun 10, 2017

Uh oh!

zimbabao commented Jun 10, 2017

Uh oh!

addaleax commented Jun 10, 2017

Uh oh!

refack left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Jun 10, 2017

Uh oh!

addaleax commented Jun 12, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

zimbabao commented Jun 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

TimothyGu commented Jun 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

mscdex commented Jun 8, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.