Skip to content

Commit e161d55

Browse files
bnoordhuisbnoordhuis
committed
Handle DataCloneError serialization errors (rubyjs#330)
Instead of trying to detect upfront if an object can be serialized, just do it and see if it throws a DataCloneError. Unexpected DataCloneErrors were of course already being handled but the new approach does it in a centralized and generic manner. Failed serialization now passes an object with a single "error" string property back to Ruby land. Fixes the failing test from rubyjs#329
1 parent ffb7061 commit e161d55

File tree

2 files changed

+69
-87
lines changed

2 files changed

+69
-87
lines changed

‎ext/mini_racer_extension/mini_racer_v8.cc‎

Lines changed: 50 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ struct State
3434
v8::Local<v8::Context> safe_context;
3535
v8::Persistent<v8::Context> persistent_context; // single-thread mode only
3636
v8::Persistent<v8::Context> persistent_safe_context; // single-thread mode only
37-
v8::Persistent<v8::Object> persistent_webassembly_instance;
38-
v8::Local<v8::Object> webassembly_instance;
3937
Context *ruby_context;
4038
int64_t max_memory;
4139
int err_reason;
@@ -81,21 +79,54 @@ bool reply(State& st, v8::Local<v8::Value> v)
8179
return serialized.data != nullptr; // exception pending if false
8280
}
8381

82+
bool reply(State& st, v8::Local<v8::Value> result, v8::Local<v8::Value> err)
83+
{
84+
v8::TryCatch try_catch(st.isolate);
85+
try_catch.SetVerbose(st.verbose_exceptions);
86+
v8::Local<v8::Array> response;
87+
{
88+
v8::Context::Scope context_scope(st.safe_context);
89+
response = v8::Array::New(st.isolate, 2);
90+
}
91+
response->Set(st.context, 0, result).Check();
92+
response->Set(st.context, 1, err).Check();
93+
if (reply(st, response)) return true;
94+
if (!try_catch.CanContinue()) { // termination exception?
95+
try_catch.ReThrow();
96+
return false;
97+
}
98+
v8::String::Utf8Value s(st.isolate, try_catch.Exception());
99+
const char *message = *s ? *s : "unexpected failure";
100+
// most serialization errors will be DataCloneErrors but not always
101+
// DataCloneErrors are not directly detectable so use a heuristic
102+
if (!strstr(message, "could not be cloned")) {
103+
try_catch.ReThrow();
104+
return false;
105+
}
106+
// return an {"error": "foo could not be cloned"} object
107+
v8::Local<v8::Object> error;
108+
{
109+
v8::Context::Scope context_scope(st.safe_context);
110+
error = v8::Object::New(st.isolate);
111+
}
112+
auto key = v8::String::NewFromUtf8Literal(st.isolate, "error");
113+
v8::Local<v8::String> val;
114+
if (!v8::String::NewFromUtf8(st.isolate, message).ToLocal(&val)) {
115+
val = v8::String::NewFromUtf8Literal(st.isolate, "unexpected error");
116+
}
117+
error->Set(st.context, key, val).Check();
118+
response->Set(st.context, 0, error).Check();
119+
if (!reply(st, response)) {
120+
try_catch.ReThrow();
121+
return false;
122+
}
123+
return true;
124+
}
125+
84126
v8::Local<v8::Value> sanitize(State& st, v8::Local<v8::Value> v)
85127
{
86128
// punch through proxies
87129
while (v->IsProxy()) v = v8::Proxy::Cast(*v)->GetTarget();
88-
// things that cannot be serialized
89-
if (v->IsArgumentsObject() ||
90-
v->IsPromise() ||
91-
v->IsModule() ||
92-
v->IsModuleNamespaceObject() ||
93-
v->IsWasmMemoryObject() ||
94-
v->IsWasmModuleObject() ||
95-
v->IsWasmNull() ||
96-
v->IsWeakRef()) {
97-
return v8::Object::New(st.isolate);
98-
}
99130
// V8's serializer doesn't accept symbols
100131
if (v->IsSymbol()) return v8::Symbol::Cast(*v)->Description(st.isolate);
101132
// TODO(bnoordhuis) replace this hack with something more principled
@@ -111,17 +142,10 @@ v8::Local<v8::Value> sanitize(State& st, v8::Local<v8::Value> v)
111142
return array;
112143
}
113144
}
114-
// WebAssembly.Instance objects are not serializable but there
115-
// is no direct way to detect them through the V8 C++ API
116-
if (!st.webassembly_instance.IsEmpty() &&
117-
v->IsObject() &&
118-
v->InstanceOf(st.context, st.webassembly_instance).FromMaybe(false)) {
119-
return v8::Object::New(st.isolate);
120-
}
121145
return v;
122146
}
123147

124-
v8::Local<v8::Value> to_error(State& st, v8::TryCatch *try_catch, int cause)
148+
v8::Local<v8::String> to_error(State& st, v8::TryCatch *try_catch, int cause)
125149
{
126150
v8::Local<v8::Value> t;
127151
char buf[1024];
@@ -216,18 +240,7 @@ extern "C" State *v8_thread_init(Context *c, const uint8_t *snapshot_buf,
216240
st.safe_context = v8::Context::New(st.isolate);
217241
st.context = v8::Context::New(st.isolate);
218242
v8::Context::Scope context_scope(st.context);
219-
auto global = st.context->Global();
220-
// globalThis.WebAssembly is missing in --jitless mode
221-
auto key = v8::String::NewFromUtf8Literal(st.isolate, "WebAssembly");
222-
v8::Local<v8::Value> wasm_v;
223-
if (global->Get(st.context, key).ToLocal(&wasm_v) && wasm_v->IsObject()) {
224-
auto key = v8::String::NewFromUtf8Literal(st.isolate, "Instance");
225-
st.webassembly_instance =
226-
wasm_v.As<v8::Object>()
227-
->Get(st.context, key).ToLocalChecked().As<v8::Object>();
228-
}
229243
if (single_threaded) {
230-
st.persistent_webassembly_instance.Reset(st.isolate, st.webassembly_instance);
231244
st.persistent_safe_context.Reset(st.isolate, st.safe_context);
232245
st.persistent_context.Reset(st.isolate, st.context);
233246
return pst; // intentionally returning early and keeping alive
@@ -246,7 +259,7 @@ void v8_api_callback(const v8::FunctionCallbackInfo<v8::Value>& info)
246259
v8::Local<v8::Array> request;
247260
{
248261
v8::Context::Scope context_scope(st.safe_context);
249-
request = v8::Array::New(st.isolate, 2);
262+
request = v8::Array::New(st.isolate, 1 + info.Length());
250263
}
251264
for (int i = 0, n = info.Length(); i < n; i++) {
252265
request->Set(st.context, i, sanitize(st, info[i])).Check();
@@ -354,11 +367,6 @@ extern "C" void v8_call(State *pst, const uint8_t *p, size_t n)
354367
v8::ValueDeserializer des(st.isolate, p, n);
355368
std::vector<v8::Local<v8::Value>> args;
356369
des.ReadHeader(st.context).Check();
357-
v8::Local<v8::Array> response;
358-
{
359-
v8::Context::Scope context_scope(st.safe_context);
360-
response = v8::Array::New(st.isolate, 2);
361-
}
362370
v8::Local<v8::Value> result;
363371
int cause = INTERNAL_ERROR;
364372
{
@@ -420,9 +428,7 @@ extern "C" void v8_call(State *pst, const uint8_t *p, size_t n)
420428
if (!cause && try_catch.HasCaught()) cause = RUNTIME_ERROR;
421429
if (cause) result = v8::Undefined(st.isolate);
422430
auto err = to_error(st, &try_catch, cause);
423-
response->Set(st.context, 0, result).Check();
424-
response->Set(st.context, 1, err).Check();
425-
if (!reply(st, response)) {
431+
if (!reply(st, result, err)) {
426432
assert(try_catch.HasCaught());
427433
goto fail; // retry; can be termination exception
428434
}
@@ -437,11 +443,6 @@ extern "C" void v8_eval(State *pst, const uint8_t *p, size_t n)
437443
v8::HandleScope handle_scope(st.isolate);
438444
v8::ValueDeserializer des(st.isolate, p, n);
439445
des.ReadHeader(st.context).Check();
440-
v8::Local<v8::Array> response;
441-
{
442-
v8::Context::Scope context_scope(st.safe_context);
443-
response = v8::Array::New(st.isolate, 2);
444-
}
445446
v8::Local<v8::Value> result;
446447
int cause = INTERNAL_ERROR;
447448
{
@@ -475,9 +476,7 @@ extern "C" void v8_eval(State *pst, const uint8_t *p, size_t n)
475476
if (!cause && try_catch.HasCaught()) cause = RUNTIME_ERROR;
476477
if (cause) result = v8::Undefined(st.isolate);
477478
auto err = to_error(st, &try_catch, cause);
478-
response->Set(st.context, 0, result).Check();
479-
response->Set(st.context, 1, err).Check();
480-
if (!reply(st, response)) {
479+
if (!reply(st, result, err)) {
481480
assert(try_catch.HasCaught());
482481
goto fail; // retry; can be termination exception
483482
}
@@ -638,11 +637,6 @@ extern "C" void v8_snapshot(State *pst, const uint8_t *p, size_t n)
638637
v8::HandleScope handle_scope(st.isolate);
639638
v8::ValueDeserializer des(st.isolate, p, n);
640639
des.ReadHeader(st.context).Check();
641-
v8::Local<v8::Array> response;
642-
{
643-
v8::Context::Scope context_scope(st.safe_context);
644-
response = v8::Array::New(st.isolate, 2);
645-
}
646640
v8::Local<v8::Value> result;
647641
v8::StartupData blob{nullptr, 0};
648642
int cause = INTERNAL_ERROR;
@@ -682,9 +676,7 @@ extern "C" void v8_snapshot(State *pst, const uint8_t *p, size_t n)
682676
} else {
683677
err = to_error(st, &try_catch, cause);
684678
}
685-
response->Set(st.context, 0, result).Check();
686-
response->Set(st.context, 1, err).Check();
687-
if (!reply(st, response)) {
679+
if (!reply(st, result, err)) {
688680
assert(try_catch.HasCaught());
689681
goto fail; // retry; can be termination exception
690682
}
@@ -699,11 +691,6 @@ extern "C" void v8_warmup(State *pst, const uint8_t *p, size_t n)
699691
std::vector<uint8_t> storage;
700692
v8::ValueDeserializer des(st.isolate, p, n);
701693
des.ReadHeader(st.context).Check();
702-
v8::Local<v8::Array> response;
703-
{
704-
v8::Context::Scope context_scope(st.safe_context);
705-
response = v8::Array::New(st.isolate, 2);
706-
}
707694
v8::Local<v8::Value> result;
708695
v8::StartupData blob{nullptr, 0};
709696
int cause = INTERNAL_ERROR;
@@ -761,9 +748,7 @@ extern "C" void v8_warmup(State *pst, const uint8_t *p, size_t n)
761748
} else {
762749
err = to_error(st, &try_catch, cause);
763750
}
764-
response->Set(st.context, 0, result).Check();
765-
response->Set(st.context, 1, err).Check();
766-
if (!reply(st, response)) {
751+
if (!reply(st, result, err)) {
767752
assert(try_catch.HasCaught());
768753
goto fail; // retry; can be termination exception
769754
}
@@ -807,14 +792,12 @@ extern "C" void v8_single_threaded_enter(State *pst, Context *c, void (*f)(Conte
807792
v8::Isolate::Scope isolate_scope(st.isolate);
808793
v8::HandleScope handle_scope(st.isolate);
809794
{
810-
st.webassembly_instance = v8::Local<v8::Object>::New(st.isolate, st.persistent_webassembly_instance);
811795
st.safe_context = v8::Local<v8::Context>::New(st.isolate, st.persistent_safe_context);
812796
st.context = v8::Local<v8::Context>::New(st.isolate, st.persistent_context);
813797
v8::Context::Scope context_scope(st.context);
814798
f(c);
815799
st.context = v8::Local<v8::Context>();
816800
st.safe_context = v8::Local<v8::Context>();
817-
st.webassembly_instance = v8::Local<v8::Object>();
818801
}
819802
}
820803

@@ -830,7 +813,6 @@ State::~State()
830813
{
831814
v8::Locker locker(isolate);
832815
v8::Isolate::Scope isolate_scope(isolate);
833-
persistent_webassembly_instance.Reset();
834816
persistent_safe_context.Reset();
835817
persistent_context.Reset();
836818
}

‎test/mini_racer_test.rb‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -893,14 +893,13 @@ def test_wasm_ref
893893
skip "TruffleRuby does not support WebAssembly"
894894
end
895895
context = MiniRacer::Context.new
896-
# Error: [object Object] could not be cloned
897-
assert_raises(MiniRacer::RuntimeError) do
898-
context.eval("
899-
var b = [0,97,115,109,1,0,0,0,1,26,5,80,0,95,0,80,0,95,1,127,0,96,0,1,110,96,1,100,2,1,111,96,0,1,100,3,3,4,3,3,2,4,7,26,2,12,99,114,101,97,116,101,83,116,114,117,99,116,0,1,7,114,101,102,70,117,110,99,0,2,9,5,1,3,0,1,0,10,23,3,8,0,32,0,20,2,251,27,11,7,0,65,12,251,0,1,11,4,0,210,0,11,0,44,4,110,97,109,101,1,37,3,0,11,101,120,112,111,114,116,101,100,65,110,121,1,12,99,114,101,97,116,101,83,116,114,117,99,116,2,7,114,101,102,70,117,110,99]
900-
var o = new WebAssembly.Instance(new WebAssembly.Module(new Uint8Array(b))).exports
901-
o.refFunc()(o.createStruct) // exotic object
902-
")
903-
end
896+
expected = {"error" => "Error: [object Object] could not be cloned."}
897+
actual = context.eval("
898+
var b = [0,97,115,109,1,0,0,0,1,26,5,80,0,95,0,80,0,95,1,127,0,96,0,1,110,96,1,100,2,1,111,96,0,1,100,3,3,4,3,3,2,4,7,26,2,12,99,114,101,97,116,101,83,116,114,117,99,116,0,1,7,114,101,102,70,117,110,99,0,2,9,5,1,3,0,1,0,10,23,3,8,0,32,0,20,2,251,27,11,7,0,65,12,251,0,1,11,4,0,210,0,11,0,44,4,110,97,109,101,1,37,3,0,11,101,120,112,111,114,116,101,100,65,110,121,1,12,99,114,101,97,116,101,83,116,114,117,99,116,2,7,114,101,102,70,117,110,99]
899+
var o = new WebAssembly.Instance(new WebAssembly.Module(new Uint8Array(b))).exports
900+
o.refFunc()(o.createStruct) // exotic object
901+
")
902+
assert_equal expected, actual
904903
end
905904

906905
def test_proxy_support
@@ -1060,16 +1059,17 @@ def test_map
10601059

10611060
def test_regexp_string_iterator
10621061
context = MiniRacer::Context.new
1063-
exc = false
1064-
begin
1065-
context.eval("'abc'.matchAll(/./g)")
1066-
rescue MiniRacer::RuntimeError => e
1067-
# TODO(bnoordhuis) maybe detect the iterator object and serialize
1068-
# it as an array of strings; problem is there is no V8 API to detect
1069-
# regexp string iterator objects
1070-
assert_match(/\[object RegExp String Iterator\] could not be cloned/, e.message)
1071-
exc = true
1072-
end
1073-
assert exc
1062+
# TODO(bnoordhuis) maybe detect the iterator object and serialize
1063+
# it as a string or array of strings; problem is there is no V8 API
1064+
# to detect regexp string iterator objects
1065+
expected = {"error" => "Error: [object RegExp String Iterator] could not be cloned."}
1066+
assert_equal expected, context.eval("'abc'.matchAll(/./g)")
1067+
end
1068+
1069+
def test_function_property
1070+
context = MiniRacer::Context.new
1071+
# regrettably loses the non-function properties
1072+
expected = {"error" => "Error: f() {} could not be cloned."}
1073+
assert_equal expected, context.eval("({ x: 42, f() {} })")
10741074
end
10751075
end

0 commit comments

Comments
 (0)