Re: [therubyracer] Exception handling

[Charles Lowell]

On Sep 21, 2012, at 12:50 PM, Philipp Pirozhkov wrote:

Hi all,

I would like to describe my concerns on current exception handling below.

How it is handled currently:

def a
  some_method_that_doesnt_exist
end

begin
  cxt['a'] = method :a
  cxt.eval('a')
rescue V8::JSError => e
  if e.in_ruby?
    puts "some ruby error'
  else
    puts "some JS error'
  end
end

Doesn't look so good, right?

First proposal is to split V8::JSError into V8::JSError::Ruby and V8::JSError:JS, thus we can rescue them individually or together, using their common ancestor. (described in https://github.com/cowboyd/therubyracer/issues/200)

My second concern is that there's no sane way to understand what kind of exception we got from ruby:

    require 'v8'

    class Ex < StandardError
    end

    class A
      def c
        puts "raising"
        raise Ex.new "i'm a specific ruby exception"
      end
    end

    c = V8::Context.new
    c['a'] = A.new

    begin
      c.eval('a.c')
    rescue Ex => e
      puts "rescued ex: #{e}"
    rescue V8::JSError => e2
      puts "rescued js: #{e2.inspect}"
      puts e2.in_ruby?
    rescue StandardError => e3
      puts "rescued se: #{e3}"
    end

Guess, which one is rescued?

V8::JSError, and `in_ruby?` is returning true.




If we are passing a wrapper for Net::HTTP to JS code, how do we distinguish between Net::HTTPBadResponse and NoMethodError?

In first case there's an error with networking, and it is recoverable in JS code (try another host, report, log et c.).

The second case is quite different and means there's an error in evaluated JS.

How do we distinguish these two?

Does the new error handling in 0.11.0 address these concerns?

Let's move to my third concern.

JS is known to be a safe sandbox. If JS is trying to do something malicious, we should be notified.

require 'v8'

class MaliciousScript < Exception
end

class A
  def a path
    raise MaliciousScript.new "hey, the script is trying to `rm -rf /`!" if path =~ /\//
  end
end

c = V8::Context.new
c['a'] = A.new

begin
  c.eval 'a.a("my")'
  puts "ok"
  c.eval <<-EOF
try {
  a.a("/")
} catch(e){
  // Hey let's hack those bastards!
}
EOF
  puts "No exception!"
rescue MaliciousScript => e
  puts "rescued maliciuos script: #{e}"
rescue V8::JSError => e2
  puts "rescued js: #{e2.inspect}"
  puts e2.in_ruby?
rescue Exception => e3
  puts "rescued se: #{e3}"
end

Guess what?

ok
No exception!

\

This is definitely NOT good. Thanks for spotting this. 

My proposals here abridged:

1. Split V8::JSError into two, JS specific and Ruby specific

2. Let Ruby catch ruby specific exceptions in a straightforward way

we could mix in modules to the V8::Error instance indicating whether the root cause was in ruby or Javascript

module V8::Error:JS

end

module V8::Error::Ruby

end

that way, you could distinguish (optionally) in your rescue

begin

rescue V8::Error::JavaScriptError => e

  #root cause from JS

rescue V8::Error::RubyError => e

  #root cause was from Ruby

rescue V8::Error

 #don't care which language originated the exception, only that one did occur during JS evaluation

end

3. Restrict JS from catching exceptions it's not intended to catch

This is tichy, need to dig into V8 to see how to do this, but it is critical for sandboxing. This is easier in Rhino, I think.

Please let me know what you think.

PS Just for the case you may have questions regarding what i'm doing. I'm working on a platform which allows users to host and execute JS, and it's Ruby controlled/based. I provide an API to JS, and would like to do so in a controlled way.

I'd love to hear your thoughts on this. If you want to take a stab at time boxing scripts in V8 (like you can already do with Rhino) I'd be happy to help you with it.

PPS Should I crosspost/move this to javascript-and-friends? Not sure if it's common to therubyrhino or execjs.

Since therubyracer and therubyrhino share a common api, I think it is wise. I'll do so.

BR, Phil

Charles Lowell 

thefrontside.net | twitter: @cowboyd | github: cowboyd

(p) +1.512.207.0229 (f) +1.512.374.0777

[Karol Bucek]

 Hey, just a few quick notes from me ...

I'm a bit against the proposed error "split" into two classes I'd rather prefer using a sub-class (I was even considering this with therubyrhino lately) :

https://github.com/kares/therubyrhino/commit/db54adc223d8f5881fee81e533ae6ea419f27228#L1R165

As the spec shows it's a bit cumbersome to get the original Ruby error, I was thinking of using a `Rhino::JSError` sub-class when a Ruby error is being wrapped.
This would also keep things backwards-compatible if we'd have a `class Rhino::JSRubyError < Rhino::JSError` while JSRubyError will provide a simple reader to retrieve the wrapper (Ruby embedded into JavaScript) error that is expected to be always a non-nil "native" error.

It does not address your second concern but is good to know as a reminder: therubyracer and therubyrhino do share a common policy of only rescuing StandardError and ScriptError :

https://github.com/cowboyd/redjs/blob/0d844f066666f967a78b20beb164c52d9ac3f5ca/lib/redjs/spec/context_spec.rb#L952

Since NoMethodError is a StandardError and not a "fatal" one it will be handled by the V8/Rhino engine - I do think this behaviour is correct.

I wonder if the introduction of a JSRubyError sub-class would solve the issue for you (you will still need to do some of your own error checking) or it will still be too much of a hustle and thus would require an introduction of some error mixins as Charlie proposed. I personally would prefer not to do so since that does make things more complicated when switching from one engine to another ... but that is just "my-preferred-way" of doing portable MRI/JRuby development :)

As this is primarily a V8 concern currently I will most-likely adapt Rhino in a similar way as V8 handles this.

K.

[Charles Lowell]

On Oct 2, 2012, at 3:31 AM, Karol Bucek wrote:

 Hey, just a few quick notes from me ...

I'm a bit against the proposed error "split" into two classes I'd rather prefer using a sub-class (I was even considering this with therubyrhino lately) :

https://github.com/kares/therubyrhino/commit/db54adc223d8f5881fee81e533ae6ea419f27228#L1R165

As the spec shows it's a bit cumbersome to get the original Ruby error, I was thinking of using a `Rhino::JSError` sub-class when a Ruby error is being wrapped.
This would also keep things backwards-compatible if we'd have a `class Rhino::JSRubyError < Rhino::JSError` while JSRubyError will provide a simple reader to retrieve the wrapper (Ruby embedded into JavaScript) error that is expected to be always a non-nil "native" error.

I think that the module solution and the subclass solution are essentially equivalent because rescue clauses discriminate on all ancestors including modules. (the discriminator module would be mixed in directly to the instance upon initialization). Maybe using a module is too "clever" when a subclass would work just as well. 

We should decide something that works for both the racer and the rhino because exception handling is critical to API compatibility.

It does not address your second concern but is good to know as a reminder: therubyracer and therubyrhino do share a common policy of only rescuing StandardError and ScriptError :

https://github.com/cowboyd/redjs/blob/0d844f066666f967a78b20beb164c52d9ac3f5ca/lib/redjs/spec/context_spec.rb#L952

Since NoMethodError is a StandardError and not a "fatal" one it will be handled by the V8/Rhino engine - I do think this behaviour is correct.

Agreed. If you wanted to make NoMethodError fatal, then the answer is to rescue it in method_missing and raise something that is uncatchable. That said, I believe Phil was pointing to a bug in 0.11.0beta8 where even fatal exceptions are catchable by JavaScript.

[Philipp Pirozhkov]

I'm a bit against the proposed error "split" into two classes I'd rather prefer using a sub-class 

This would also keep things backwards-compatible if we'd have a `class Rhino::JSRubyError < Rhino::JSError`

This is exactly what I mean and I completely agree with that.

It does not address your second concern but is good to know as a reminder: therubyracer and therubyrhino do share a common policy of only rescuing StandardError and ScriptError

I've put a bad example.

This is resolved in 0.11 with the `cause` method.

But the code is still far from shining:

class APIError < StandardError; end

class A

  def a

    raise APIError.new "ooooo sometimes" if rand(2) > 0

  end

end

def controller_action_which_evaluates_js some_parameter1, some_parameter2

  raise APIError.new "Parameter 1 is empty" if some_parameter1.empty?

  raise APIError.new "Parameter 1 is empty" if some_parameter2.empty?

  c = V8::Context.new

  c['a'] = A.new

  c.eval 'a.a'

rescue APIError => e # CASE 1

  UserLog.create message: e.message

  render 400, e.message

rescue V8::Error => e

  if e.in_ruby? and e.cause.is_a? APIError  # CASE 2

    UserLog.create message: e.message

    render 500, e.message

  elsif e.in_ruby?  # CASE 3

    log.error "System error #{e.message}"

    render 500, "Unrecoverable system error"

  else  # CASE 4

    UserLog.create message: e.message

    render 500, e.message

  end

rescue StandardError => e  # CASE 5

  log.error "System error #{e.message}"

  render 500, "Unrecoverable system error"

end

Looks messy, doesn't it?

It's nice to have JS backtrace for some kind of Ruby errors, maybe we can add a V8::Error::Ruby mixin to an error raised in Ruby code called from JS?

could look shorter:

  ...

rescue APIError => e # CASE 1 and 2

  UserLog.create message: e.message

  render 400, e.message

rescue V8::Error::JS => e # CASE 4

  UserLog.create message: e.message

  render 500, e.message

rescue StandardError => e  # CASES 3 and 5

  log.error "System error #{e.message}"

  render 500, "Unrecoverable system error"

end

As this is primarily a V8 concern currently I will most-likely adapt Rhino in a similar way as V8 handles this.

Cool!

BR, Phil

[Philipp Pirozhkov]

Does the new error handling in 0.11.0 address these concerns?

It's really really better now. I've finally moved to 0.11 today and that was the only way to handle one kind of errors. I believe this could be done by major refactorings, but with 0.11 it was quick and simple to get the root error as `cause` and set an `if`.

we could mix in modules to the V8::Error instance indicating whether the root cause was in ruby or Javascript

Both subclassing and mixins should work and will look just the same from caller/rescuer's perspective.

 

that way, you could distinguish (optionally) in your rescue

begin

rescue V8::Error::JavaScriptError => e

  #root cause from JS

rescue V8::Error::RubyError => e

  #root cause was from Ruby

rescue V8::Error

 #don't care which language originated the exception, only that one did occur during JS evaluation

end

Yep, looks perfect.

I'd love to hear your thoughts on this. If you want to take a stab at time boxing scripts in V8 (like you can already do with Rhino) I'd be happy to help you with it.

Really appreciated. Will time boxing allow for limiting the execution of the script somehow? I wonder if this is V8 already. Is that something like Firefox's feature "the script processing is taking to long, do you want to stop it?" ? 

BR, Phil 

[Charles Lowell]

On Oct 3, 2012, at 4:29 PM, Philipp Pirozhkov wrote:

Does the new error handling in 0.11.0 address these concerns?

It's really really better now. I've finally moved to 0.11 today and that was the only way to handle one kind of errors. I believe this could be done by major refactorings, but with 0.11 it was quick and simple to get the root error as `cause` and set an `if`.

we could mix in modules to the V8::Error instance indicating whether the root cause was in ruby or Javascript

Both subclassing and mixins should work and will look just the same from caller/rescuer's perspective.

 

that way, you could distinguish (optionally) in your rescue

begin

rescue V8::Error::JavaScriptError => e

  #root cause from JS

rescue V8::Error::RubyError => e

  #root cause was from Ruby

rescue V8::Error

 #don't care which language originated the exception, only that one did occur during JS evaluation

end

Yep, looks perfect.

Karol, is this agreeable then if we use a scheme like this with subclasses and/or modules? 

class V8::JavaScriptError < V8::Error # in_ruby? == false; in_javascript? == true

class V8::RubyError < V8::Error # in_ruby? == true; in_javascript? == false

I'm open to other names for the exceptions.

I'd love to hear your thoughts on this. If you want to take a stab at time boxing scripts in V8 (like you can already do with Rhino) I'd be happy to help you with it.

Really appreciated. Will time boxing allow for limiting the execution of the script somehow? I wonder if this is V8 already. Is that something like Firefox's feature "the script processing is taking to long, do you want to stop it?" ? 

This facility does exist in V8 and it is exposed in 0.11, but only at the very low-level metal. We'd need to implement the Rhino interface for V8 based on this low level. Unfortunately, V8 does not give you instruction level callbacks from the JavaScript VM. Instead you have to have a monitoring thread that pre-empts the javascript execution thread and terminates it forcibly if it exceeds the allotted time box. Would love to get together and work on this. When are you available generally?

cheers,

Charles

BR, Phil 

[Karol Bucek]

 Hey Charlie and Philipp,

sure it's agreeable, but it's a bit different in Rhino (I should have
noticed this earlier) but I shall manage a refactoring :

rename Rhino::JSError into Rhino::Error (and make sure "generic" non-JS
RhinoExceptions are created as such)
introduce JSError as a sub-class of Error (for JavaScriptExceptions -
sub-class of RhinoException)
and than add RubyError as a sub-class of ? - seems to me that this kind of
error is actually still a JSError (JavaScriptError)
shouldn't we RubyError < JSError instead of RubyError < Error ?

K.

[Charles Lowell]

On Oct 4, 2012, at 6:11 AM, Karol Bucek wrote:

 Hey Charlie and Philipp,

sure it's agreeable, but it's a bit different in Rhino (I should have
noticed this earlier) but I shall manage a refactoring :

rename Rhino::JSError into Rhino::Error (and make sure "generic" non-JS
RhinoExceptions are created as such)
introduce JSError as a sub-class of Error (for JavaScriptExceptions -
sub-class of RhinoException)
and than add RubyError as a sub-class of ? - seems to me that this kind of
error is actually still a JSError (JavaScriptError)
shouldn't we RubyError < JSError instead of RubyError < Error ?

It all boils down to what it is that each exceptional case is supposed to represent. My idea was that [ENGINE]::Error would represent that some exception happened during JavaScript execution, so perhaps an explicit JavaScriptError is not necessary, and all we need is two error classes? *::Error and *::RubyError < *::Error

The other question, which is not addressed so far, but we might as well, is do we want to reflect the four JavaScript exception types into Ruby so that you can discriminate in rescue clauses

*::ReferenceError < *::Error

*::RangeError < *::Error

*::TypeError < *::Error

*::SyntaxError < *::Error

If we do, then it probably *would* be handy to have a superclass  JavaScriptError so that you could catch any one of these with a single statement:

+-Error

 +-JavaScriptError

  | +ReferenceError

  | +RangeError

  | +TypeError

  | +SyntaxError

  +-RubyError

    

cheers,

Charles

[Karol Bucek]

On Thu, Oct 4, 2012 at 1:43 PM, Charles Lowell <cow...@thefrontside.net> wrote:

On Oct 4, 2012, at 6:11 AM, Karol Bucek wrote:

 Hey Charlie and Philipp,

sure it's agreeable, but it's a bit different in Rhino (I should have
noticed this earlier) but I shall manage a refactoring :

rename Rhino::JSError into Rhino::Error (and make sure "generic" non-JS
RhinoExceptions are created as such)
introduce JSError as a sub-class of Error (for JavaScriptExceptions -
sub-class of RhinoException)
and than add RubyError as a sub-class of ? - seems to me that this kind of
error is actually still a JSError (JavaScriptError)
shouldn't we RubyError < JSError instead of RubyError < Error ?

It all boils down to what it is that each exceptional case is supposed to represent. My idea was that [ENGINE]::Error would represent that some exception happened during JavaScript execution, so perhaps an explicit JavaScriptError is not necessary, and all we need is two error classes? *::Error and *::RubyError < *::Error

I see your point Charlie, that's what I have assumed as well as there was (and is) only a single JSError is Rhino.
But there's other kinds of errors coming from the engine there's the "generic" RhinoException (JavaScriptException is it's sub-class) for "other" non-JS errors.
I'm sure you can find similar cases for V8 (for a kind of engine expection), thus I thought we should address this as well while reviewing errors.
 

The other question, which is not addressed so far, but we might as well, is do we want to reflect the four JavaScript exception types into Ruby so that you can discriminate in rescue clauses

*::ReferenceError < *::Error

*::RangeError < *::Error

*::TypeError < *::Error

*::SyntaxError < *::Error

If we do, then it probably *would* be handy to have a superclass  JavaScriptError so that you could catch any one of these with a single statement:

+-Error

 +-JavaScriptError

  | +ReferenceError

  | +RangeError

  | +TypeError

  | +SyntaxError

  +-RubyError
 

    

Optionally this sound great, I'm not sure I'll implement this on Rhino cause it might require some error message parsing in my-case.
But if you can do this easily with V8 it might turn out useful eventually ...
 

[Charles Lowell]

On Oct 4, 2012, at 7:45 AM, Karol Bucek wrote:

On Thu, Oct 4, 2012 at 1:43 PM, Charles Lowell <cow...@thefrontside.net> wrote:

On Oct 4, 2012, at 6:11 AM, Karol Bucek wrote:

 Hey Charlie and Philipp,

sure it's agreeable, but it's a bit different in Rhino (I should have
noticed this earlier) but I shall manage a refactoring :

rename Rhino::JSError into Rhino::Error (and make sure "generic" non-JS
RhinoExceptions are created as such)
introduce JSError as a sub-class of Error (for JavaScriptExceptions -
sub-class of RhinoException)
and than add RubyError as a sub-class of ? - seems to me that this kind of
error is actually still a JSError (JavaScriptError)
shouldn't we RubyError < JSError instead of RubyError < Error ?

It all boils down to what it is that each exceptional case is supposed to represent. My idea was that [ENGINE]::Error would represent that some exception happened during JavaScript execution, so perhaps an explicit JavaScriptError is not necessary, and all we need is two error classes? *::Error and *::RubyError < *::Error

I see your point Charlie, that's what I have assumed as well as there was (and is) only a single JSError is Rhino.
But there's other kinds of errors coming from the engine there's the "generic" RhinoException (JavaScriptException is it's sub-class) for "other" non-JS errors.
I'm sure you can find similar cases for V8 (for a kind of engine expection), thus I thought we should address this as well while reviewing errors.

Hmmm, yes I remember now that there are weird non-js VM related exceptions . Actually those don't really exist in V8 because it does not use C++ exceptions as part of its public API like Java does, so the only "exceptions" it will ever see are actual JavaScript exceptions, the equivalent of the org.mozilla.javascript.EvaluatorException. Not sure what the best approach is to reconcile these different views are.

 

The other question, which is not addressed so far, but we might as well, is do we want to reflect the four JavaScript exception types into Ruby so that you can discriminate in rescue clauses

*::ReferenceError < *::Error

*::RangeError < *::Error

*::TypeError < *::Error

*::SyntaxError < *::Error

If we do, then it probably *would* be handy to have a superclass  JavaScriptError so that you could catch any one of these with a single statement:

+-Error

 +-JavaScriptError

  | +ReferenceError

  | +RangeError

  | +TypeError

  | +SyntaxError

  +-RubyError
 

    

Optionally this sound great, I'm not sure I'll implement this on Rhino cause it might require some error message parsing in my-case.
But if you can do this easily with V8 it might turn out useful eventually ...
 

In Rhino, you should be able to get a reference to the constructor of the Error object if it is a JavaScript error right?.

In any case, it doesn't make sense to implement anything in V8 that we can't replicate on both engines. The current discrepancies in API cause headaches for me already, so I'm not keen on introducing more. Whatever we decide, I think it should work everywhere. 

That said, what is an exception hierarchy you would say that would work well on Rhino?

cheers,

Charles