[1]: 0xb5e4cc21 <Code>#0#/* warning: no JSFunction object or function name found */ (this=0x4d268795 <an Object>#1#) { // expression stack (top to bottom) [09] : 0 [08] : -608931120 [07] : 0x267fc029 <Foreign>#5# [06] : -538220396 [05] : -538220370 [04] : -538220366 [03] : 1 [02] : 0 [01] : 68866650 [00] : 0x267fa4d1 <Very long string[3306]>#4#
}
[2]: runScript [builtin/require.js:11] (this=0x44242321 <JS Global Object>#3#,src=0x267fa4d1 <Very long string[3306]>#4#,fn=0x267f9629 <String[42]: /home/ubuntu/src/SilkJS/modules/LogFile.js>) { // stack-allocated locals var script = 0x44208091 <undefined> var exports = 0x44208091 <undefined> // expression stack (top to bottom) [08] : 0x4d268795 <an Object>#1# [07] : 0x44245089 <JS Function compileScript>#6# [06] : 0x44208091 <undefined> [05] : 68768524 [04] : 0x267f9629 <String[42]: /home/ubuntu/src/SilkJS/modules/LogFile.js> [03] : 0x267fa4d1 <Very long string[3306]>#4# [02] : 0x4d268795 <an Object>#1# --------- s o u r c e c o d e --------- function runScript(src, fn) {???var script = v8.compileScript(src, fn);???var exports = v8.runScript(script);???v8.freeScript(script);???return exports;??} -----------------------------------------
}
[3]: require [builtin/require.js:113] (this=0x44242321 <JS Global Object>#3#,module=0x5a9121c5 <String[7]: LogFile>) { // stack-allocated locals var m = 0x44208091 <undefined> var modulePath = 0x267f9629 <String[42]: /home/ubuntu/src/SilkJS/modules/LogFile.js> var content = 0x267f9661 <Very long string[3163]>#7# var fsPath = 0x267fa2c9 <JS Array[6]>#8# var exports = 0x267fa3b1 <an Object>#9# var script = 0x267fa4d1 <Very long string[3306]>#4# var context = 0x267fbeb1 <an Object>#10# var _module = 0x267fbebd <an Object>#11# // expression stack (top to bottom) [13] : 0x267f9629 <String[42]: /home/ubuntu/src/SilkJS/modules/LogFile.js> [12] : 0x267fa4d1 <Very long string[3306]>#4# [11] : 0x44242321 <JS Global Object>#3# [10] : 0x267ec9f1 <JS Function runScript>#12# [09] : 0x267f9629 <String[42]: /home/ubuntu/src/SilkJS/modules/LogFile.js> [08] : 0x267ece61 <an Object>#13# --------- s o u r c e c o d e --------- function (module) {???if (module.substr(0, 8) == 'builtin/') {????var m = builtin[module.substr(8)];????return m;???}???var modulePath = locateFile(module);???if (require.cache[modulePath]) {????return require.cache[modulePath];???}? if (soRegEx.test(modulePath)) {? require.cache[modulePath...
-----------------------------------------
}
[4]: /* anonymous */ [/usr/local/bin/httpd-silk.js:8] (this=0x44242321 <JS Global Object>#3#) { // stack-allocated locals var .result = 0x44208091 <undefined> // expression stack (top to bottom) [02] : 0x5a9121c5 <String[7]: LogFile> [01] : 0x44242321 <JS Global Object>#3# --------- s o u r c e c o d e --------- ///usr/local/bin/silkjs?// httpd/main.js??print_r = require('builtin/print_r');?console = require('console');?fs = require('fs');??LogFile = require('LogFile');?net = require('builtin/net');?process = require('builtin/process');?async = require('builtin/async');?v8 = require('builtin/v8');?http = re...
I've been able to verify this using a fresh install of SilkJS and v8 on 32 bit Ubuntu 12.04 on both EC2 hosts and a VirtualBox host instantiated by Vagrant (see https://github.com/obscurerichard/vagrant-silkjs for a quick way to reproduce the environment).
This version of v8 worked:root@domU-12-31-39-03-44-E1:~/SilkJS/src/v8-read-only# svn info Path: . URL: http://v8.googlecode.com/svn/trunk Repository Root: http://v8.googlecode.com/svn Repository UUID: ce2b1a6d-e550-0410-aec6-3dcde31c8c00 Revision: 12443 Node Kind: directory Schedule: normal Last Changed Author: verwa...@chromium.org Last Changed Rev: 12436 Last Changed Date: 2012-09-04 09:58:32 +0000 (Tue, 04 Sep 2012)
These two did not:
root@precise32:~/SilkJS/src/v8-read-only# svn info Path: . URL: http://v8.googlecode.com/svn/trunk Repository Root: http://v8.googlecode.com/svn Repository UUID: ce2b1a6d-e550-0410-aec6-3dcde31c8c00 Revision: 12452 Node Kind: directory Schedule: normal Last Changed Author: jkumme...@chromium.org Last Changed Rev: 12448 Last Changed Date: 2012-09-05 16:44:50 +0000 (Wed, 05 Sep 2012)
root@domU-12-31-39-0E-8E-09:~/SilkJS/src/v8-read-only# svn info Path: . URL: http://v8.googlecode.com/svn/trunk Repository Root: http://v8.googlecode.com/svn Repository UUID: ce2b1a6d-e550-0410-aec6-3dcde31c8c00 Revision: 12449 Node Kind: directory Schedule: normal Last Changed Author: jkumme...@chromium.org Last Changed Rev: 12448 Last Changed Date: 2012-09-05 16:44:50 +0000 (Wed, 05 Sep 2012)
These were both built from the trunk today. Revision 12448's diff is 509 lines of code relating to fixing a number of bugs: # svn log -r 12448 ------------------------------------------------------------------------ r12448 | jkumme...@chromium.org | 2012-09-05 16:44:50 +0000 (Wed, 05 Sep 2012) | 21 lines Merged r12434, r12435, r12440, r12441, r12443, r12444, r12445, r12446 into trunk branch. Support register as right operand in min/max support. Fixed test expectation. Add build system infrastructure for ENABLE_EXTRA_CHECKS flag (not used yet) Fix missing colon in common.gypi Push stacktrace and die if the receiver is of unknown type. Disable accessor inlining (due to broken deopts) Add empty-handle checks to API functions (#ifdef ENABLE_EXTRA_CHECKS) Check the return value of API calls on ia32 and x64. R=yang...@chromium.org Review URL: https://chromiumcodereview.appspot.com/10910093
It looks as if this can be triggered by simply requiring the LogFile javascript file, also:
vagrant@precise32:~$ silkjs SilkJS> var logfile = require('LogFile'); abort: API call returned invalid object
[1]: 0x2724d521 <Code>#0#/* warning: no JSFunction object or function name found */ (this=0x4a368795 <an Object>#1#) { // expression stack (top to bottom) [10] : 0 [09] : -608974128 [08] : 0x267f9a05 <Foreign>#5# [07] : -538717972 [06] : -538717944 [05] : -538717940 [04] : 1 [03] : 0 [02] : 77258652 [01] : 0x345080a1 <the hole> [00] : 0x267f7eb9 <Very long string[3300]>#4#
}
[2]: runScript [/usr/local/silkjs/builtin/require.js:11] (this=0x34542321 <JS Global Object>#3#,src=0x267f7eb9 <Very long string[3300]>#4#,fn=0x267f7031 <String[36]: /usr/local/silkjs/modules/LogFile.js>) { // stack-allocated locals var script = 0x34508091 <undefined> var exports = 0x34508091 <undefined> // expression stack (top to bottom) [08] : 0x4a368795 <an Object>#1# [07] : 0x34545089 <JS Function compileScript>#6# [06] : 0x34508091 <undefined> [05] : 77163276 [04] : 0x267f7031 <String[36]: /usr/local/silkjs/modules/LogFile.js> [03] : 0x267f7eb9 <Very long string[3300]>#4# [02] : 0x4a368795 <an Object>#1# --------- s o u r c e c o d e --------- function runScript(src, fn) {???var script = v8.compileScript(src, fn);???var exports = v8.runScript(script);???v8.freeScript(script);???return exports;??} -----------------------------------------
}
[3]: require [/usr/local/silkjs/builtin/require.js:113] (this=0x34542321 <JS Global Object>#3#,module=0x29913321 <String[7]: LogFile>) { // stack-allocated locals var m = 0x34508091 <undefined> var modulePath = 0x267f7031 <String[36]: /usr/local/silkjs/modules/LogFile.js> var content = 0x267f7061 <Very long string[3163]>#7# var fsPath = 0x267f7cc9 <JS Array[5]>#8# var exports = 0x267f7d99 <an Object>#9# var script = 0x267f7eb9 <Very long string[3300]>#4# var context = 0x267f988d <an Object>#10# var _module = 0x267f9899 <an Object>#11# // expression stack (top to bottom) [13] : 0x267f7031 <String[36]: /usr/local/silkjs/modules/LogFile.js> [12] : 0x267f7eb9 <Very long string[3300]>#4# [11] : 0x34542321 <JS Global Object>#3# [10] : 0x267ecb3d <JS Function runScript>#12# [09] : 0x267f7031 <String[36]: /usr/local/silkjs/modules/LogFile.js> [08] : 0x267ecfad <an Object>#13# --------- s o u r c e c o d e --------- function (module) {???if (module.substr(0, 8) == 'builtin/') {????var m = builtin[module.substr(8)];????return m;???}???var modulePath = locateFile(module);???if (require.cache[modulePath]) {????return require.cache[modulePath];???}? if (soRegEx.test(modulePath)) {? require.cache[modulePath...
-----------------------------------------
}
[4]: /* anonymous */ [0x34508091 <undefined>:1] (this=0x34542321 <JS Global Object>#3#) { // expression stack (top to bottom) [02] : 0x29913321 <String[7]: LogFile> [01] : 0x34542321 <JS Global Object>#3# [00] : 0x267ecc9d <JS Function>#14# --------- s o u r c e c o d e --------- var logfile = require('LogFile'); -----------------------------------------
}
[5]: arguments adaptor frame: 1->0 { // actual arguments [00] : 0x267f6525 <String[34]: var logfile = require('LogFile');> // not passed to callee
}
[6]: main [interpreter:10] (this=0x34542321 <JS Global Object>#3#) { // heap-allocated locals var arguments = 0x267f48f9 <an Arguments>#15# var stdin = 0x267f4915 <a ReadLine>#16# var line = 0x267f6525 <String[34]: var logfile = require('LogFile');> // expression stack (top to bottom) [08] : 0x267f6525 <String[34]: var logfile = require('LogFile');> [07] : 0x34542321 <JS Global Object>#3# [06] : 0x267f6d95 <JS Function>#17# [05] : 0x267f47e9 <an Object>#18# --------- s o u r c e c o d e --------- function main() {? var stdin = new ReadLine('silkjs');? stdin.prompt('SilkJS> ');? while (1) {? try {? var line = stdin.gets();? console.log(eval(line));? }? catch (e) {? if (e === 'SIGQUIT') {? break;? }? else if...
The line "abort: API call returned invalid object" is caused by one of the
sanity checks added yesterday: We now check if the value returned from a
native getter (and under some circumstances the value returned from an
interceptor) is a valid JavaScript object, see
http://codereview.chromium.org/10918071/. Do you return something which is
not a Smi/string/some JavaScript object/heap
number/undefined/true/false/null in your bindings?
} On Wednesday, September 5, 2012 11:31:15 PM UTC-7, Sven Panne wrote:
> The line "abort: API call returned invalid object" is caused by one of > the sanity checks added yesterday: We now check if the value returned from > a native getter (and under some circumstances the value returned from an > interceptor) is a valid JavaScript object, see > http://codereview.chromium.org/10918071/. Do you return something which > is not a Smi/string/some JavaScript object/heap > number/undefined/true/false/null in your bindings?
Yep, we diagnosed the same yesterday. What happens is this: When trying to
construct an External value, we check if the given void* fits in a Smi. If
it does, we return a Smi, otherwise we create a Foreign object which wraps
the void*. The latter case is problematic, and this is exactly what is
intentionally caught by the recently introduced checks: Foreign is a
v8-internal data structure which gets suddenly exposed on the JavaScript
side, but you can't do anything with it apart from reading and storing it,
everything else will crash v8.
We don't want to loosen the checks, because we want to catch exactly those
kind of non-JavaScript values returned from native getters/interceptors,
something which indicates a bug in the binding. So the correct fix is that
External::New should always return a valid JavaScript object. Currently I
see two ways of implementing it:
* Continue encoding it as a Smi if it fits, but use the 64bit payload of
a HeapNumber for all other void*. This would give us "floating pointers"...
:-) It's a bit of a hack, but probably the quickest way of fixing the
problem at hand.
* Create an empty new JavaScript Object which carries the void* as a
hidden property (probably as a Foreign). This is the cleaner solution, and
I'd like to try this first.
Anyway, any sane binding should keep the External value private, so our
choice how to fix this shouldn't matter for the embedder.
As a quick intermediate workaround, make sure that ENABLE_EXTRA_CHECKS is
#undefined when building SilkJS, but this should really only be a temporary
measure.
After several discussions, it is not so clear anymore what to do. First of
all, SilkJS does not follow https://developers.google.com/v8/embed#dynamic on
how to handle foreign (i.e. C/C++) pointers when embedding v8. The return
value of External::New is supposed to live in an internal field, but it is
*not* a valid JavaScript value, it is just a Foreign in disguise, sometimes
optimized to a Smi. Our v8.h header is very confusing regarding this fact,
and having External as a subclass of Value is basically wrong. Furthermore
Value::IsExternal is completely broken. I can see 2 ways of fixing things:
* Keep External's implementation basically as it is. i.e. either a Smi
or a Foreign. If we do this, we should not keep External as a subclass of
Value (perhaps a subclass of Data?) and we should remove the IsExternal
predicate. This means that e.g. SilkJS has to change, following
https://developers.google.com/v8/embed#dynamic. As it is, one can easily
crash SilkJS by pure JavaScript.
* Make External basically a convenience wrapper for a JavaScript object
with an internal property containing a Foreign. This way we could keep
External a subclass of value and we could fix IsExternal. The downside is
that all code already following
https://developers.google.com/v8/embed#dynamic would basically do a useless
double indirection, punishing people following that guide.
We will discuss these options, there are good arguments for both of them...
On Fri, Sep 7, 2012 at 2:50 PM, Sven Panne <svenpa...@chromium.org> wrote:
> After several discussions, it is not so clear anymore what to do. First of
> all, SilkJS does not follow https://developers.google.com/v8/embed#dynamic > on how to handle foreign (i.e. C/C++) pointers when embedding v8. The return
> value of External::New is supposed to live in an internal field, but it is
> *not* a valid JavaScript value, it is just a Foreign in disguise, sometimes
> optimized to a Smi. Our v8.h header is very confusing regarding this fact,
> and having External as a subclass of Value is basically wrong. Furthermore
> Value::IsExternal is completely broken. I can see 2 ways of fixing things:
> * Keep External's implementation basically as it is. i.e. either a Smi or
> a Foreign. If we do this, we should not keep External as a subclass of Value
> (perhaps a subclass of Data?) and we should remove the IsExternal predicate.
> This means that e.g. SilkJS has to change, following
> https://developers.google.com/v8/embed#dynamic. As it is, one can easily
> crash SilkJS by pure JavaScript.
> * Make External basically a convenience wrapper for a JavaScript object
> with an internal property containing a Foreign. This way we could keep
> External a subclass of value and we could fix IsExternal. The downside is
> that all code already following
> https://developers.google.com/v8/embed#dynamic would basically do a useless
> double indirection, punishing people following that guide.
> We will discuss these options, there are good arguments for both of them...
Shouldn't use use GetPointerFromInternalField and SetPointerInInternalField
for this? For SetInternalField one could change the signature to use Data
or provide an overloaded version. Anyway, saying External is-a Value in its
current state is plainly wrong... :-)
On Fri, Sep 7, 2012 at 3:02 PM, Vyacheslav Egorov <vego...@chromium.org>wrote:
> Chromium's bindings layer uses External to associate opaque data with
> V8 objects and callbacks (see methods accepting Handle<Value> data in
> V8 api).
> So making External into a non-Value might involve some bindings work.
I have no issue with JS being able to crash SilkJS. In fact, I see it as a feature. The whole point of SilkJS is to provide a C-like and low-level Linux/OSX programming environment using JavaScript.
In C, you can crash your programs as trivially as:
fwrite(buf, 10, 1, (FILE *)0xdeadbeef);
The philosophy of SilkJS is that you can do likewise. However, the low-level bindings do return opaque handles to things as External::New() type things, and I implement JavaScript classes and methods that wrap the low-level bindings and add sanity and error checking of the arguments.
I fully understand the dynamic C++ class pattern in your link below, but I absolutely do not want to use it (for the most part). The cost of using it is obfuscating the implementation of such classes, as they need to be compiled and linked. The pattern I chose allows virtually all the code you'd require me to write in C++ to be coded in pure JavaScript, except for as thin a layer of glue functions to convert JS arguments to C parameters/call a library/OS function/return JS object. I expect people who use a server-side JavaScript platform to know JavaScript. I do not expect them to know C++.
On Friday, September 7, 2012 5:50:34 AM UTC-7, Sven Panne wrote:
> After several discussions, it is not so clear anymore what to do. First of > all, SilkJS does not follow https://developers.google.com/v8/embed#dynamic on > how to handle foreign (i.e. C/C++) pointers when embedding v8. The return > value of External::New is supposed to live in an internal field, but it is > *not* a valid JavaScript value, it is just a Foreign in disguise, sometimes > optimized to a Smi. Our v8.h header is very confusing regarding this fact, > and having External as a subclass of Value is basically wrong. Furthermore > Value::IsExternal is completely broken. I can see 2 ways of fixing things:
> * Keep External's implementation basically as it is. i.e. either a Smi > or a Foreign. If we do this, we should not keep External as a subclass of > Value (perhaps a subclass of Data?) and we should remove the IsExternal > predicate. This means that e.g. SilkJS has to change, following > https://developers.google.com/v8/embed#dynamic. As it is, one can easily > crash SilkJS by pure JavaScript.
> * Make External basically a convenience wrapper for a JavaScript object > with an internal property containing a Foreign. This way we could keep > External a subclass of value and we could fix IsExternal. The downside is > that all code already following > https://developers.google.com/v8/embed#dynamic would basically do a > useless double indirection, punishing people following that guide.
> We will discuss these options, there are good arguments for both of them...
On Fri, Sep 7, 2012 at 3:18 PM, mschwartz <myk...@gmail.com> wrote:
> I have no issue with JS being able to crash SilkJS. In fact, I see it as
> a feature. The whole point of SilkJS is to provide a C-like and low-level
> Linux/OSX programming environment using JavaScript.
> In C, you can crash your programs as trivially as:
> fwrite(buf, 10, 1, (FILE *)0xdeadbeef);
> The philosophy of SilkJS is that you can do likewise. However, the
> low-level bindings do return opaque handles to things as External::New()
> type things, and I implement JavaScript classes and methods that wrap the
> low-level bindings and add sanity and error checking of the arguments.
> I fully understand the dynamic C++ class pattern
Doesn't sound like you do, or at least not how it applies to the situation
at hand. Nobody is saying that you have to implement classes in C++ that
you could previously implement in JavaScript. Values created in JS and
passed into C++ land are not the issue.
The part of the V8 API documentation that applies here is how to wrap an
External (that was created in C++!) safely before passing it into JS (where
it can't be used anyway! It can only be read from C++). And that's by
storing it as in internal field of a valid JS object, not as a property
that's accessible on the JavaScript side. Because the latter would be
inherently unsafe.
Just because V8 in the past didn't strictly check for it doesn't mean
SilkJS (or any other embedder) should be doing this. V8's API should never
have allowed this in the first place; the fact that it did was a bug. The
question is not "do we or don't we fix this". The question is "how do we
fix this".
> in your link below, but I absolutely do not want to use it (for the most
> part). The cost of using it is obfuscating the implementation of such
> classes, as they need to be compiled and linked. The pattern I chose
> allows virtually all the code you'd require me to write in C++ to be coded
> in pure JavaScript, except for as thin a layer of glue functions to convert
> JS arguments to C parameters/call a library/OS function/return JS object.
> I expect people who use a server-side JavaScript platform to know
> JavaScript. I do not expect them to know C++.
And yet you want the JavaScript to be able to crash just as if it were
C/C++? On a server, even? That doesn't make sense.
> On Friday, September 7, 2012 5:50:34 AM UTC-7, Sven Panne wrote:
>> After several discussions, it is not so clear anymore what to do. First
>> of all, SilkJS does not follow https://developers.** >> google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic> on
>> how to handle foreign (i.e. C/C++) pointers when embedding v8. The return
>> value of External::New is supposed to live in an internal field, but it is
>> *not* a valid JavaScript value, it is just a Foreign in disguise, sometimes
>> optimized to a Smi. Our v8.h header is very confusing regarding this fact,
>> and having External as a subclass of Value is basically wrong. Furthermore
>> Value::IsExternal is completely broken. I can see 2 ways of fixing things:
>> * Keep External's implementation basically as it is. i.e. either a Smi
>> or a Foreign. If we do this, we should not keep External as a subclass of
>> Value (perhaps a subclass of Data?) and we should remove the IsExternal
>> predicate. This means that e.g. SilkJS has to change, following
>> https://developers.**google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic>.
>> As it is, one can easily crash SilkJS by pure JavaScript.
I'm in favor of this. I guess we could provide a convenience function that
wraps an External as an internal field into an otherwise empty object and
returns that, but it would have to be called explicitly.
>> * Make External basically a convenience wrapper for a JavaScript
>> object with an internal property containing a Foreign. This way we could
>> keep External a subclass of value and we could fix IsExternal. The downside
>> is that all code already following https://developers.** >> google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic>
>> **would basically do a useless double indirection, punishing people
>> following that guide.
>> We will discuss these options, there are good arguments for both of
>> them...
> The part of the V8 API documentation that applies here is how to wrap an
> External (that was created in C++!) safely before passing it into JS (where
> it can't be used anyway! It can only be read from C++). And that's by
> storing it as in internal field of a valid JS object, not as a property
> that's accessible on the JavaScript side. Because the latter would be
> inherently unsafe.
>> ...
> * Keep External's implementation basically as it is. i.e. either a Smi
>>> or a Foreign. If we do this, we should not keep External as a subclass of
>>> Value (perhaps a subclass of Data?) and we should remove the IsExternal
>>> predicate. This means that e.g. SilkJS has to change, following
>>> https://developers.**google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic>.
>>> As it is, one can easily crash SilkJS by pure JavaScript.
> I'm in favor of this. I guess we could provide a convenience function that
> wraps an External as an internal field into an otherwise empty object and
> returns that, but it would have to be called explicitly.
FWIW, making an External not-a Value would break break ALL class bindings
i've ever done (that's no small number of them), as well as every class
binding in every framework i'm currently aware of (which of course is not
all of them). That's a degree of incompatibility where i would have to pack
up and move elsewhere (that's the reason i left Qt and the Autotools - too
many incompatible changes breaking too many of my apps, causing me to spend
too much time fixing things other people broke).
"In the beginning" i used External directly as properties (because there
was no documentation warning me otherwise), but quickly found that passing
them around in JS segfaults (and i belong to the school of thought that
script code is a sandbox and should never be able to segfault a native
app). So i starting storing my (void*) via External in an InternalField
(because that's how the API is apparently designed to be used). i've bound
hundreds of classes using this approach, and i'd rather abandon my v8-using
projects than go back and rework them all.
There are hundreds of thousands of google search results (hits) about NodeJS crashing and core dumping and how to keep your server up and running. I see Chrome throw up "Aw snap" pages frequently enough. I've seen segfault messages in apache logfiles.
The fact that SilkJS can crash is fine because software does crash.
A bug is a bug. Squash it. We're engineers, we are supposed to make things work.
As I see it, the API you exposed in your header files and documentation is something of a contract, and I abided by it. My code has worked extremely well and quite robustly for well over a year. You change the API, you break my code and expect me to edit tens of thousands of lines of C++ code and the deal with the potential instabilities those changes will introduce.
I added #undef ENABLE_EXTRA_CHECKS to my master header file, immediately after the #include of v8.h and it has no effect.
As a compromise, may I suggest an alternative to External::New()? How about you implement an Opaque::New() that satisfies whatever you feel necessary under the hood?
On Friday, September 7, 2012 8:26:18 AM UTC-7, Jakob Kummerow wrote:
> On Fri, Sep 7, 2012 at 3:18 PM, mschwartz <myk...@gmail.com <javascript:>>wrote:
>> I have no issue with JS being able to crash SilkJS. In fact, I see it as >> a feature. The whole point of SilkJS is to provide a C-like and low-level >> Linux/OSX programming environment using JavaScript.
>> In C, you can crash your programs as trivially as:
>> fwrite(buf, 10, 1, (FILE *)0xdeadbeef);
>> The philosophy of SilkJS is that you can do likewise. However, the >> low-level bindings do return opaque handles to things as External::New() >> type things, and I implement JavaScript classes and methods that wrap the >> low-level bindings and add sanity and error checking of the arguments.
>> I fully understand the dynamic C++ class pattern
> Doesn't sound like you do, or at least not how it applies to the situation > at hand. Nobody is saying that you have to implement classes in C++ that > you could previously implement in JavaScript. Values created in JS and > passed into C++ land are not the issue.
> The part of the V8 API documentation that applies here is how to wrap an > External (that was created in C++!) safely before passing it into JS (where > it can't be used anyway! It can only be read from C++). And that's by > storing it as in internal field of a valid JS object, not as a property > that's accessible on the JavaScript side. Because the latter would be > inherently unsafe.
> Just because V8 in the past didn't strictly check for it doesn't mean > SilkJS (or any other embedder) should be doing this. V8's API should never > have allowed this in the first place; the fact that it did was a bug. The > question is not "do we or don't we fix this". The question is "how do we > fix this".
>> in your link below, but I absolutely do not want to use it (for the most >> part). The cost of using it is obfuscating the implementation of such >> classes, as they need to be compiled and linked. The pattern I chose >> allows virtually all the code you'd require me to write in C++ to be coded >> in pure JavaScript, except for as thin a layer of glue functions to convert >> JS arguments to C parameters/call a library/OS function/return JS object. >> I expect people who use a server-side JavaScript platform to know >> JavaScript. I do not expect them to know C++.
> And yet you want the JavaScript to be able to crash just as if it were > C/C++? On a server, even? That doesn't make sense.
>> On Friday, September 7, 2012 5:50:34 AM UTC-7, Sven Panne wrote:
>>> After several discussions, it is not so clear anymore what to do. First >>> of all, SilkJS does not follow https://developers.** >>> google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic> on >>> how to handle foreign (i.e. C/C++) pointers when embedding v8. The return >>> value of External::New is supposed to live in an internal field, but it is >>> *not* a valid JavaScript value, it is just a Foreign in disguise, sometimes >>> optimized to a Smi. Our v8.h header is very confusing regarding this fact, >>> and having External as a subclass of Value is basically wrong. Furthermore >>> Value::IsExternal is completely broken. I can see 2 ways of fixing things:
>>> * Keep External's implementation basically as it is. i.e. either a >>> Smi or a Foreign. If we do this, we should not keep External as a subclass >>> of Value (perhaps a subclass of Data?) and we should remove the IsExternal >>> predicate. This means that e.g. SilkJS has to change, following >>> https://developers.**google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic>. >>> As it is, one can easily crash SilkJS by pure JavaScript.
> I'm in favor of this. I guess we could provide a convenience function that > wraps an External as an internal field into an otherwise empty object and > returns that, but it would have to be called explicitly.
>>> * Make External basically a convenience wrapper for a JavaScript >>> object with an internal property containing a Foreign. This way we could >>> keep External a subclass of value and we could fix IsExternal. The downside >>> is that all code already following https://developers.** >>> google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic> >>> **would basically do a useless double indirection, punishing people >>> following that guide.
>>> We will discuss these options, there are good arguments for both of >>> them...
On Fri, Sep 7, 2012 at 5:57 PM, mschwartz <myk...@gmail.com> wrote:
> As I see it, the API you exposed in your header files and documentation is
> something of a contract, and I abided by it. My code has worked extremely
> well and quite robustly for well over a year. You change the API, you
> break my code and expect me to edit tens of thousands of lines of C++ code
> and the deal with the potential instabilities those changes will introduce.
Amen, brother.
As a compromise, may I suggest an alternative to External::New()? How
> about you implement an Opaque::New() that satisfies whatever you feel
> necessary under the hood?
On Fri, Sep 7, 2012 at 5:48 PM, Stephan Beal <sgb...@googlemail.com> wrote:
> On Fri, Sep 7, 2012 at 5:25 PM, Jakob Kummerow <jkumme...@chromium.org>wrote:
>> The part of the V8 API documentation that applies here is how to wrap an
>> External (that was created in C++!) safely before passing it into JS (where
>> it can't be used anyway! It can only be read from C++). And that's by
>> storing it as in internal field of a valid JS object, not as a property
>> that's accessible on the JavaScript side. Because the latter would be
>> inherently unsafe.
>>> ...
>> * Keep External's implementation basically as it is. i.e. either a
>>>> Smi or a Foreign. If we do this, we should not keep External as a subclass
>>>> of Value (perhaps a subclass of Data?) and we should remove the IsExternal
>>>> predicate. This means that e.g. SilkJS has to change, following
>>>> https://developers.**google.com/v8/embed#dynamic<https://developers.google.com/v8/embed#dynamic>.
>>>> As it is, one can easily crash SilkJS by pure JavaScript.
>> I'm in favor of this. I guess we could provide a convenience function
>> that wraps an External as an internal field into an otherwise empty object
>> and returns that, but it would have to be called explicitly.
> FWIW, making an External not-a Value would break break ALL class bindings
> i've ever done (that's no small number of them), as well as every class
> binding in every framework i'm currently aware of (which of course is not
> all of them). That's a degree of incompatibility where i would have to pack
> up and move elsewhere (that's the reason i left Qt and the Autotools - too
> many incompatible changes breaking too many of my apps, causing me to spend
> too much time fixing things other people broke).
> "In the beginning" i used External directly as properties (because there
> was no documentation warning me otherwise), but quickly found that passing
> them around in JS segfaults (and i belong to the school of thought that
> script code is a sandbox and should never be able to segfault a native
> app). So i starting storing my (void*) via External in an InternalField
> (because that's how the API is apparently designed to be used).
And that's exactly what you should be doing. (I haven't designed V8's API,
but I agree that this is apparently how it is supposed to be used.) The
idea is precisely not to break code doing this, nor to make it slower.
There would obviously have to be a drop-in replacement (overloaded version,
most likely) of existing functions that allows you to keep doing this. I
don't think you need to be concerned.
I'd also point out that the traditional means that API designers use to change an API is to mark existing API they want people to stop using as deprecated, but allow that API to continue to work. Additionally, if I turn on some switch, the API might emit a message saying that some deprecated method is being used.
On Friday, September 7, 2012 8:58:57 AM UTC-7, Stephan Beal wrote:
> On Fri, Sep 7, 2012 at 5:57 PM, mschwartz <myk...@gmail.com <javascript:>>wrote:
>> As I see it, the API you exposed in your header files and documentation >> is something of a contract, and I abided by it. My code has worked >> extremely well and quite robustly for well over a year. You change the >> API, you break my code and expect me to edit tens of thousands of lines of >> C++ code and the deal with the potential instabilities those changes will >> introduce.
> Amen, brother.
> As a compromise, may I suggest an alternative to External::New()? How >> about you implement an Opaque::New() that satisfies whatever you feel >> necessary under the hood?
