Skip to content

Commit

Permalink
v8: add a js class for Serializer/Dserializer
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
zimbabao authored and addaleax committed Jun 12, 2017
1 parent 68c0518 commit 38a1cfb
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 1 deletion.
12 changes: 11 additions & 1 deletion lib/v8.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,21 @@
'use strict';

const { Buffer } = require('buffer');
const { Serializer, Deserializer } = process.binding('serdes');
const {
Serializer: _Serializer,
Deserializer: _Deserializer
} = process.binding('serdes');
const { copy } = process.binding('buffer');
const { objectToString } = require('internal/util');
const { FastBuffer } = require('internal/buffer');

// Calling exposed c++ functions directly throws exception as it expected to be
// called with new operator and caused an assert to fire.
// Creating JS wrapper so that it gets caught at JS layer.
class Serializer extends _Serializer { }

class Deserializer extends _Deserializer { }

const {
cachedDataVersionTag,
setFlagsFromString,
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-v8-serdes.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,15 @@ const objects = [

assert.deepStrictEqual(v8.deserialize(buf), expectedResult);
}

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

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

3 comments on commit 38a1cfb

@MylesBorins
Copy link
Contributor

@MylesBorins MylesBorins commented on 38a1cfb Jul 10, 2017

Choose a reason for hiding this comment

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

This patch did not bump the V8 patch number. We have also generally been using the "deps" subsystem for V8.

edit: under further inspection the subsystem here should have been lib... and no bump to patch

@addaleax
Copy link
Member

Choose a reason for hiding this comment

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

@MylesBorins This is about the require('v8') core module, not V8 as in deps/v8.

@MylesBorins
Copy link
Contributor

Choose a reason for hiding this comment

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

see edit above... this got wrapped up in auditing a0f8faa3a4

Please sign in to comment.