Implement setTimeout

src/GlobalJsObject.cpp

 #include <vector>
+#include <stdexcept>
 
 #include "GlobalJsObject.h"
 #include "Thread.h"
 
 using namespace AdblockPlus;
 
 namespace
 {
   class TimeoutThread : public Thread
   {
   public:
     TimeoutThread(v8::Isolate* const isolate, const v8::Arguments& arguments)
       : isolate(isolate)
     {
+      if (arguments.Length() < 2)
+        throw std::runtime_error("setTimeout requires at least 2 parameters");
+
+      const v8::Local<v8::Value> functionValue = arguments[0];
+      if (!functionValue->IsFunction())
+        throw std::runtime_error("No valid function passed to setTimeout");

[Wladimir Palant, 2013/04/08 14:03:47]

Nit: I think the canonical message is: "First argument to setTimeout must be a
function".

+
+      const v8::Local<v8::Value> delayValue = arguments[1];
+      if (!delayValue->IsNumber())
+        throw std::runtime_error("No valid delay passed to setTimeout");

[Wladimir Palant, 2013/04/08 14:03:47]

Nit: I think the canonical message is: "Second argument to setTimeout must be a
number".

+
       function = v8::Persistent<v8::Function>::New(
-        isolate, v8::Local<v8::Function>::Cast(arguments[0]));

[Felix Dahlke, 2013/04/08 13:07:49]

Phew, good find. V8 isn't exactly documented but I assumed they'd throw an
exception and that's it... Instead it's actually causing a fatal error and
crashes the whole runtime.

I made sure to check argument number and types properly and added some tests.

-      delay = arguments[1]->ToNumber()->Value();
+        isolate, v8::Local<v8::Function>::Cast(functionValue));
+      delay = delayValue->ToNumber()->Value();
       for (int i = 2; i < arguments.Length(); i++)
       {
         const Value argument = Value::New(isolate, arguments[i]);
         functionArguments.push_back(argument);
       }
     }
 
     ~TimeoutThread()
     {
       function.Dispose(isolate);
       for (Values::iterator it = functionArguments.begin();
            it != functionArguments.end(); it++)
         it->Dispose(isolate);
     }
 
     void Run()
     {
       Sleep(delay);
       const v8::Locker locker(isolate);

[Wladimir Palant, 2013/04/08 08:36:00]

Does this really lock when this line is reached or already when the scope is
entered? I would add an additional scope after the Sleep() call.

       const v8::HandleScope handleScope;
       function->Call(function, functionArguments.size(), &functionArguments[0]);
       delete this;

[Wladimir Palant, 2013/04/08 08:36:00]

Self-deleting object?

1) Is that a good practice?
2) Is it thread-safe? Won't it corrupt the memory allocation table if delete/new
operations run simultaneously on different threads?

[Felix Dahlke, 2013/04/08 13:07:49]

1) Depends. It's not generally considered a bad practice, but I can't think of
many situations where it makes sense. I can't think of another way that doesn't
involve some additional house keeping thread, which I'd rather avoid.

2) It's not thread safe, but we can ensure thread safety by making sure that we
don't delete the object from the outside. And since we don't even store a
reference to it, that's safe right now.

[Wladimir Palant, 2013/04/08 14:03:47]

I checked, delete is actually thread-safe - so no issue here. The deleting part
might be made a bit nicer with smart pointers but probably not worth it.

     }
 
   private:
     typedef v8::Persistent<v8::Value> Value;
     typedef std::vector<Value> Values;
 
     v8::Isolate* const isolate;
     v8::Persistent<v8::Function> function;
     int delay;
     Values functionArguments;
   };
 
   v8::Handle<v8::Value> SetTimeoutCallback(const v8::Arguments& arguments)
   {
-    TimeoutThread* const timeoutThread =
-      new TimeoutThread(v8::Isolate::GetCurrent(), arguments);
+    TimeoutThread* timeoutThread;
+    try
+    {
+      timeoutThread = new TimeoutThread(v8::Isolate::GetCurrent(), arguments);
+    }
+    catch (const std::exception& e)
+    {
+      return v8::ThrowException(v8::String::New(e.what()));
+    }
     timeoutThread->Start();
 
     // We should actually return the timer ID here, which could be
     // used via clearTimeout(). But since we don't seem to need
-    // clearTimeout(), we can safe that for later.

[Wladimir Palant, 2013/04/08 08:36:00]

safe => save?

[Felix Dahlke, 2013/04/08 13:07:49]

Yes, of course.

+    // clearTimeout(), we can save that for later.
     return v8::Undefined();
   }
 }
 
 v8::Handle<v8::ObjectTemplate> GlobalJsObject::Create(
   ErrorCallback& errorCallback)
 {
   const v8::Locker locker(v8::Isolate::GetCurrent());
   v8::HandleScope handleScope;
   const v8::Handle<v8::ObjectTemplate> global = v8::ObjectTemplate::New();
   const v8::Handle<v8::ObjectTemplate> console =
     AdblockPlus::ConsoleJsObject::Create(errorCallback);
   global->Set(v8::String::New("console"), console);
   const v8::Handle<v8::FunctionTemplate> setTimeoutFunction =
     v8::FunctionTemplate::New(SetTimeoutCallback);
   global->Set(v8::String::New("setTimeout"), setTimeoutFunction);
   return handleScope.Close(global);
 }