Skip to content

Commit 1164eb0

Browse files
bnoordhuisbnoordhuis
committed
Handle DataCloneError serialization errors
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 5fad3b6 commit 1164eb0

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)