ia32 bug?

[mschwartz]

It is out of the blue that SilkJS compiled for 32-bit Ubuntu is throwing an error at startup.  Actually it's V8 that is throwing the error.

The script being compiled by v8 for SilkJS appears valid JavaScript and a hex dump of that file shows not odd encoding (plain ascii).

I'm wondering if maybe a bug was introduced in V8?

The output follows:

$ httpd-silk.js 

abort: API call returned invalid object

==== Stack trace ============================================

    1: 0xb5e4cc21 <Code>#0#/* warning: no JSFunction object or function name found */ (this=0x4d268795 <an Object>#1#)

Security context: 0x44242279 <JS Object>#2#

    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>)

    3: require [builtin/require.js:113] (this=0x44242321 <JS Global Object>#3#,module=0x5a9121c5 <String[7]: LogFile>)

    4: /* anonymous */ [/usr/local/bin/httpd-silk.js:8] (this=0x44242321 <JS Global Object>#3#)

==== Details ================================================

[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...

-----------------------------------------

}

==== Key         ============================================

 #0# 0xb5e4cc21: 0xb5e4cc21 <Code>

 #1# 0x4d268795: 0x4d268795 <an Object>

 #2# 0x44242279: 0x44242279 <JS Object>

 #3# 0x44242321: 0x44242321 <JS Global Object>

 #4# 0x267fa4d1: 0x267fa4d1 <Very long string[3306]>

 #5# 0x267fc029: 0x267fc029 <Foreign>

 #6# 0x44245089: 0x44245089 <JS Function compileScript>

 #7# 0x267f9661: 0x267f9661 <Very long string[3163]>

 #8# 0x267fa2c9: 0x267fa2c9 <JS Array[6]>

                 0: 0x46408125 <String[0]: >

                 1: 0x267fa2fd <String[4]: home>

                 2: 0x267fa30d <String[6]: ubuntu>

                 3: 0x267fa321 <String[3]: src>

                 4: 0x267fa331 <String[6]: SilkJS>

                 5: 0x267fa345 <String[7]: modules>

 #9# 0x267fa3b1: 0x267fa3b1 <an Object>

 #10# 0x267fbeb1: 0x267fbeb1 <an Object>

 #11# 0x267fbebd: 0x267fbebd <an Object>

                id: 0x5a9121c5 <String[7]: LogFile>

               uri: 0x267f9629 <String[42]: /home/ubuntu/src/SilkJS/modules/LogFile.js>

 #12# 0x267ec9f1: 0x267ec9f1 <JS Function runScript>

 #13# 0x267ece61: 0x267ece61 <an Object>

=====================

Trace/breakpoint trap (core dumped)

[mschwartz]

As a point of information, the X64 build works fine.

[mschwartz]

I take it back.  Code checked out this morning (PST) worked on X64, the code checked out this afternoon/evening does not.

[Richard Bullington-McGuire]

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: verw...@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: jkum...@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: jkum...@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 | jkum...@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=yan...@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

==== Stack trace ============================================

    1: 0x2724d521 <Code>#0#/* warning: no JSFunction object or function name found */ (this=0x4a368795 <an Object>#1#)
Security context: 0x34542279 <JS Object>#2#
    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>)
    3: require [/usr/local/silkjs/builtin/require.js:113] (this=0x34542321 <JS Global Object>#3#,module=0x29913321 <String[7]: LogFile>)
    4: /* anonymous */ [0x34508091 <undefined>:1] (this=0x34542321 <JS Global Object>#3#)
    5: arguments adaptor frame: 1->0
    6: main [interpreter:10] (this=0x34542321 <JS Global Object>#3#)

==== Details ================================================

[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...

-----------------------------------------
}

==== Key         ============================================

 #0# 0x2724d521: 0x2724d521 <Code>
 #1# 0x4a368795: 0x4a368795 <an Object>
 #2# 0x34542279: 0x34542279 <JS Object>
 #3# 0x34542321: 0x34542321 <JS Global Object>
 #4# 0x267f7eb9: 0x267f7eb9 <Very long string[3300]>
 #5# 0x267f9a05: 0x267f9a05 <Foreign>
 #6# 0x34545089: 0x34545089 <JS Function compileScript>
 #7# 0x267f7061: 0x267f7061 <Very long string[3163]>
 #8# 0x267f7cc9: 0x267f7cc9 <JS Array[5]>
                 0: 0x3f108125 <String[0]: >
                 1: 0x267f7cf9 <String[3]: usr>
                 2: 0x267f7d09 <String[5]: local>
                 3: 0x267f7d1d <String[6]: silkjs>
                 4: 0x267f7d31 <String[7]: modules>
 #9# 0x267f7d99: 0x267f7d99 <an Object>
 #10# 0x267f988d: 0x267f988d <an Object>
 #11# 0x267f9899: 0x267f9899 <an Object>
                id: 0x29913321 <String[7]: LogFile>
               uri: 0x267f7031 <String[36]: /usr/local/silkjs/modules/LogFile.js>
 #12# 0x267ecb3d: 0x267ecb3d <JS Function runScript>
 #13# 0x267ecfad: 0x267ecfad <an Object>
 #14# 0x267ecc9d: 0x267ecc9d <JS Function>
              main: 0x34542321 <JS Global Object>#3#
          dirStack: 0x267ece3d <JS Array[1]>#19#
            fsPath: 0x267f7d85 <String[26]: /usr/local/silkjs/modules/>
             cache: 0x267ecfad <an Object>#13#
              path: 0x267ed0a5 <JS Array[6]>#20#
 #15# 0x267f48f9: 0x267f48f9 <an Arguments>
            length: 0
            callee: 0x3fb871bd <JS Function main>#21#
 #16# 0x267f4915: 0x267f4915 <a ReadLine>
              prog: 0x299121fd <String[6]: silkjs>
       historyFile: 0x267f6429 <String[23]: /home/vagrant/.silkjsrc>
      promptString: 0x29912225 <String[8]: SilkJS> >
 #17# 0x267f6d95: 0x267f6d95 <JS Function>
 #18# 0x267f47e9: 0x267f47e9 <an Object>
               log: 0x345429b5 <JS Function log>#22#
             error: 0x34542a41 <JS Function error>#23#
               dir: 0x267f482d <JS Function>#24#
       getPassword: 0x267f4879 <JS Function>#25#
 #19# 0x267ece3d: 0x267ece3d <JS Array[1]>
                 0: 0x3f10be45 <String[0]: >
 #20# 0x267ed0a5: 0x267ed0a5 <JS Array[6]>
                 0: 0x29911075 <String[2]: ./>
                 1: 0x299118ed <String[7]: modules>
                 2: 0x29911135 <String[17]: /usr/local/silkjs>
                 3: 0x29911901 <String[25]: /usr/local/silkjs/contrib>
                 4: 0x29911929 <String[25]: /usr/local/silkjs/modules>
                 5: 0x29911951 <String[25]: /usr/share/silkjs/modules>
 #21# 0x3fb871bd: 0x3fb871bd <JS Function main>
 #22# 0x345429b5: 0x345429b5 <JS Function log>
 #23# 0x34542a41: 0x34542a41 <JS Function error>
 #24# 0x267f482d: 0x267f482d <JS Function>
 #25# 0x267f4879: 0x267f4879 <JS Function>
=====================

Trace/breakpoint trap

The calling statement is in /usr/local/silkjs/builtin/require.js in line 11, the compileScript statement below:

function runScript(src, fn) {                var script = v8.compileScript(src, fn);

The diff does have some assembly language for ia32 and x64 that does sanity checking on JavaScript functions and objects:

https://chromiumcodereview.appspot.com/10910093/patch/1/8

https://chromiumcodereview.appspot.com/10910093/patch/1/14

The x64 build yields a signal 11:

root@precise64:~/SilkJS/src/v8-read-only# silkjs 
SilkJS>  var logfile = require('LogFile');
abort: API call returned invalid object
Caught Signal 11 for process: 31978
root@precise64:~/SilkJS/src/v8-read-only# uname -a
Linux precise64 3.2.0-23-generic #36-Ubuntu SMP Tue Apr 10 20:39:51 UTC 2012 x86_64 x86_64 x86_64 GNU/Linux
root@precise64:~/SilkJS/src/v8-read-only# lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 12.04 LTS
Release:        12.04
Codename:       precise
root@precise64:~/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: jkum...@chromium.org
Last Changed Rev: 12448
Last Changed Date: 2012-09-05 16:44:50 +0000 (Wed, 05 Sep 2012)

[Sven Panne]

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?

Cheers,

   S.

[mschwartz]

Returns External::New()

Please, please, please, don't break this going forward.

struct ScriptWrapper {

    Persistent<Script> script;

};

static JSVAL compileScript (JSARGS args) {

    HandleScope scope;

    ScriptWrapper *wrapper = new ScriptWrapper;

    wrapper->script = Persistent<Script>::New(Script::New(args[0]->ToString(), args[1]->ToString()));

    return scope.Close(External::New(wrapper));

[Sven Panne]

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.

Cheers,

   S.

On Thu, Sep 6, 2012 at 8:14 PM, mschwartz <myk...@gmail.com> wrote:

Returns External::New() [...]

[Sven Panne]

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...

Cheers,

   S.

[Vyacheslav Egorov]

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.

--
Vyacheslav Egorov

> --
> v8-users mailing list
> v8-u...@googlegroups.com
> http://groups.google.com/group/v8-users

[Sven Panne]

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... :-)

[mschwartz]

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.  

Consider these C++ bindings to malloc:

  https://github.com/mschwartz/v8t/blob/master/src/binary.cpp

vs. this JavaScript wrapper for those bindings:

  https://github.com/mschwartz/v8t/blob/master/modules/binary/index.js

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++.  

[Jakob Kummerow]

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.  

Consider these C++ bindings to malloc:

  https://github.com/mschwartz/v8t/blob/master/src/binary.cpp

vs. this JavaScript wrapper for those bindings:

  https://github.com/mschwartz/v8t/blob/master/modules/binary/index.js

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 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.

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 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...

Cheers,

   S.

--

[Stephan Beal]

On Fri, Sep 7, 2012 at 5:25 PM, Jakob Kummerow <jkum...@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. 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.

>:-[

--
----- stephan beal
http://wanderinghorse.net/home/stephan/

http://gplus.to/sgbeal

[mschwartz]

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.

In fact,

mschwartz@dionysus:~/src/SilkJS/src/v8-read-only$ grep -r ENABLE_EXTRA .

mschwartz@dionysus:~/src/SilkJS/src/v8-read-only$ 

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?

[Stephan Beal]

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?

+1

[Jakob Kummerow]

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 <jkum...@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. 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've bound hundreds of classes using this approach, and i'd rather abandon my v8-using projects than go back and rework them all.

>:-[

--
----- stephan beal
http://wanderinghorse.net/home/stephan/

http://gplus.to/sgbeal

[mschwartz]

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.