Skip to content

Add failing test for function cloning #329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add failing test for function cloning #329

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d43620a
Add failing test for function cloning
SamSaffron Jan 9, 2025
344588c
fix typo in test
SamSaffron Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 26 additions & 11 deletions test/mini_racer_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,7 @@ def test_return_hash
def test_date_nan
# NoMethodError: undefined method `source_location' for "<internal:core>
# core/float.rb:114:in `to_i'":Thread::Backtrace::Location
if RUBY_ENGINE == "truffleruby"
skip "TruffleRuby bug"
end
skip "TruffleRuby bug" if RUBY_ENGINE == "truffleruby"
context = MiniRacer::Context.new
assert_raises(RangeError) { context.eval("new Date(NaN)") } # should not crash process
end
Expand Down Expand Up @@ -895,11 +893,13 @@ def test_wasm_ref
context = MiniRacer::Context.new
# Error: [object Object] could not be cloned
assert_raises(MiniRacer::RuntimeError) do
context.eval("
context.eval(
"
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]
var o = new WebAssembly.Instance(new WebAssembly.Module(new Uint8Array(b))).exports
o.refFunc()(o.createStruct) // exotic object
")
"
)
end
end

Expand All @@ -925,7 +925,7 @@ def test_proxy_support

def test_proxy_uncloneable
context = MiniRacer::Context.new()
expected = {"x" => 42}
expected = { "x" => 42 }
assert_equal expected, context.eval(<<~JS)
const o = {x: 42}
const p = new Proxy(o, {})
Expand Down Expand Up @@ -1033,9 +1033,7 @@ def test_forking
skip "TruffleRuby forking is not supported"
else
`bundle exec ruby test/test_forking.rb`
if $?.exitstatus != 0
assert false, "forking test failed"
end
assert false, "forking test failed" if $?.exitstatus != 0
end
end

Expand All @@ -1052,7 +1050,7 @@ def test_poison

def test_map
context = MiniRacer::Context.new
expected = {"x" => 42}
expected = { "x" => 42 }
assert_equal expected, context.eval("new Map([['x', 42]])")
expected = ["x", 42]
assert_equal expected, context.eval("new Map([['x', 42]]).entries()")
Expand All @@ -1067,9 +1065,26 @@ def test_regexp_string_iterator
# TODO(bnoordhuis) maybe detect the iterator object and serialize
# it as an array of strings; problem is there is no V8 API to detect
# regexp string iterator objects
assert_match(/\[object RegExp String Iterator\] could not be cloned/, e.message)
assert_match(
/\[object RegExp String Iterator\] could not be cloned/,
e.message
)
exc = true
end
assert exc
end

def test_function_cloning
message = nil
context = MiniRacer::Context.new
context.attach("log", proc { |msg| message = msg })
context.eval <<~JS
console = {
log: function(...args){ log(args.join(" ")); },
}
Comment on lines +1082 to +1084
Copy link
Collaborator

@bnoordhuis bnoordhuis Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what the problem is: the result of this statement is (edit: an object containing) the log function object. The test passes when changed to this:

Suggested change
console = {
log: function(...args){ log(args.join(" ")); },
}
console = {
log: function(...args){ log(args.join(" ")); },
}
undefined

Let me think what a good solution is.

Copy link
Collaborator

@bnoordhuis bnoordhuis Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness sake, another workaround is to make the property non-enumerable:

Suggested change
console = {
log: function(...args){ log(args.join(" ")); },
}
console = {}
Object.defineProperty(console, "log", {value: function(...args){ log(args.join(" ")); }})

Copy link
Collaborator Author

@SamSaffron SamSaffron Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we can just return something here , trouble of crashing on deserialozation boundary is that many eval and don’t care about result , even just saying “function” is fine

Copy link
Collaborator

@bnoordhuis bnoordhuis Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, I think there are 4 options:

  1. do nothing and let the error bubble up (current situation)
  2. detect the error and return something else, like an empty object or a marker
  3. deep-copy the object and replace or drop function properties
  4. ditto but only shallow-copy the object, i.e., fix up only one level

3 is O(n) time and space, 4 is only a partial fix, so I guess 2 is the best option.

JS

context.eval("console.log('Hello', 'World!')")
assert_equal "Hello World!", message
end
end
Loading