Conversation
WalkthroughAdds a test helper ScriptContextHelper and a RunScriptEngineSuites test harness; extends the scripting js API with Array[T] and new Value accessors/predicates (Number, IsNumber, IsArray) plus global helpers (IsNullish, IsBoolean); implements deep Clone for js values with cycle detection; changes ValueFactory to add NewNumber and make NewArray return Array; updates Sobek and V8 engine implementations (NewNumber, array handling, array wrappers) and adds engine tests and scriptContext.isArray helper. (≈72 words) Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| global.CreateOperation("store", func(ctx js.CallbackContext[T]) (js.Value[T], error) { | ||
| v, ok := ctx.ConsumeArg() | ||
| if !ok { | ||
| return nil, ctx.NewTypeError("Missing argument") | ||
| } | ||
| c, err := js.As[entity.Components](ctx.GlobalThis().NativeValue(), nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| entity.SetComponentType(c, v) | ||
| return nil, nil | ||
| }) |
There was a problem hiding this comment.
Guard against nil js.Value from ConsumeArg.
ConsumeArg can yield a nil js.Value[T] (e.g., undefined). Storing nil risks clone failures later; treat it as a missing argument.
🛠️ Proposed fix
- global.CreateOperation("store", func(ctx js.CallbackContext[T]) (js.Value[T], error) {
- v, ok := ctx.ConsumeArg()
- if !ok {
- return nil, ctx.NewTypeError("Missing argument")
- }
+ global.CreateOperation("store", func(ctx js.CallbackContext[T]) (js.Value[T], error) {
+ v, ok := ctx.ConsumeArg()
+ if !ok || v == nil {
+ return nil, ctx.NewTypeError("Missing argument")
+ }
c, err := js.As[entity.Components](ctx.GlobalThis().NativeValue(), nil)
if err != nil {
return nil, err
}Based on learnings: In the gost-dom JavaScript binding system, js.Value[T] is a Go interface that can be nil even though it represents a JavaScript value. Always check v != nil before calling methods like v.Boolean() to prevent panics when the Go interface is nil.
| func (c *scriptContext) isArray(v value) bool { | ||
| vm := c.vm | ||
| fn, _ := vm.RunString("x => Array.isArray(x)") | ||
| fnn, _ := sobek.AssertFunction(fn) | ||
| res, err := fnn(vm.GlobalObject(), v.value) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return res.ToBoolean() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider caching the Array.isArray helper.
Calling RunString on each invocation is relatively expensive; a cached function (initialized once per scriptContext) would reduce overhead if IsArray is used frequently (e.g., in clone logic).
| type dummyContext struct { | ||
| *entity.Entity | ||
| ctx context.Context | ||
| } | ||
|
|
||
| func (c dummyContext) Context() context.Context { return c.ctx } | ||
| func (c dummyContext) HTTPClient() http.Client { return *http.DefaultClient } | ||
| func (c dummyContext) LocationHREF() string { return "http://example.com" } | ||
| func (c dummyContext) Logger() *slog.Logger { return nil } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove duplicate/unused dummyContext in this file.
RunScriptEngineSuites already owns dummyContext in the scripttests package, so this local type appears unused and adds import overhead.
♻️ Suggested cleanup
-import (
- "context"
- "log/slog"
- "net/http"
+import (
"testing"
"github.com/gost-dom/browser/html"
- "github.com/gost-dom/browser/internal/entity"
"github.com/gost-dom/browser/internal/testing/browsertest"
. "github.com/gost-dom/browser/internal/testing/gomega-matchers"
"github.com/gost-dom/browser/scripting/internal/js"
"github.com/gost-dom/browser/scripting/internal/scripttests"
"github.com/onsi/gomega"
)
@@
-type dummyContext struct {
- *entity.Entity
- ctx context.Context
-}
-
-func (c dummyContext) Context() context.Context { return c.ctx }
-func (c dummyContext) HTTPClient() http.Client { return *http.DefaultClient }
-func (c dummyContext) LocationHREF() string { return "http://example.com" }
-func (c dummyContext) Logger() *slog.Logger { return nil }
-
func TestV8Engine(t *testing.T) {
scripttests.RunScriptEngineSuites(t,
func(c js.Configurer[jsTypeParam]) html.ScriptEngine { return newEngine(c) },
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type dummyContext struct { | |
| *entity.Entity | |
| ctx context.Context | |
| } | |
| func (c dummyContext) Context() context.Context { return c.ctx } | |
| func (c dummyContext) HTTPClient() http.Client { return *http.DefaultClient } | |
| func (c dummyContext) LocationHREF() string { return "http://example.com" } | |
| func (c dummyContext) Logger() *slog.Logger { return nil } | |
| import ( | |
| "testing" | |
| "github.com/gost-dom/browser/html" | |
| "github.com/gost-dom/browser/internal/testing/browsertest" | |
| . "github.com/gost-dom/browser/internal/testing/gomega-matchers" | |
| "github.com/gost-dom/browser/scripting/internal/js" | |
| "github.com/gost-dom/browser/scripting/internal/scripttests" | |
| "github.com/onsi/gomega" | |
| ) | |
| func TestV8Engine(t *testing.T) { | |
| scripttests.RunScriptEngineSuites(t, | |
| func(c js.Configurer[jsTypeParam]) html.ScriptEngine { return newEngine(c) }, | |
| ) | |
| } |
| func (v v8Value) String() string { return v.Value.String() } | ||
| func (v v8Value) Number() float64 { return v.Value.Number() } | ||
| func (v v8Value) Int32() int32 { return v.Value.Int32() } | ||
| func (v v8Value) Uint32() uint32 { return v.Value.Uint32() } | ||
| func (v v8Value) Boolean() bool { return v.Value.Boolean() } | ||
|
|
||
| func (v v8Value) IsUndefined() bool { return v.Value == nil || v.Value.IsUndefined() } | ||
| func (v v8Value) IsNull() bool { return v.Value.IsNull() } | ||
| func (v v8Value) IsBoolean() bool { return v.Value.IsBoolean() } | ||
| func (v v8Value) IsString() bool { return v.Value.IsString() } | ||
| func (v v8Value) IsNumber() bool { return v.Value.IsNumber() } | ||
| func (v v8Value) IsSymbol() bool { return v.Value.IsSymbol() } | ||
| func (v v8Value) IsObject() bool { return v.Value.IsObject() } | ||
| func (v v8Value) IsArray() bool { return v.Value.IsArray() } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the file mentioned in the review
find . -name "value.go" -path "*/v8engine/*" | head -20Repository: gost-dom/browser
Length of output: 89
🏁 Script executed:
# Search for newV8Value function
rg -n "newV8Value" --type goRepository: gost-dom/browser
Length of output: 2195
🏁 Script executed:
# Look at the v8Value type and related code
rg -n "type v8Value" --type go -A 10Repository: gost-dom/browser
Length of output: 731
🏁 Script executed:
# Read the complete newV8Value function implementation
sed -n '43,50p' ./scripting/v8engine/value.goRepository: gost-dom/browser
Length of output: 362
🏁 Script executed:
# Look at how v8go.Value is used elsewhere and if there are any nil checks
rg -n "v\.Value == nil" --type goRepository: gost-dom/browser
Length of output: 177
🏁 Script executed:
# Check the v8go module documentation or type definition to understand the Value type
rg -n "v8go" ./scripting/v8engine/value.go | head -20Repository: gost-dom/browser
Length of output: 1033
🏁 Script executed:
# Look for how undefined values are created or handled
rg -n "undefined" --type go -i | grep -E "(v8go|Value)" | head -20Repository: gost-dom/browser
Length of output: 1064
🏁 Script executed:
# Check if there are any tests that demonstrate nil behavior
rg -n "nil.*v8Value\|v8Value.*nil" --type go | head -20Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Look for tests that call Number(), IsNumber(), IsArray() on undefined
rg -n "Number\|IsArray" ./scripting/v8engine/ --type go -B 2 -A 2 | head -40Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Check the broader context of how the methods are used
sed -n '57,75p' ./scripting/v8engine/value.goRepository: gost-dom/browser
Length of output: 1042
🏁 Script executed:
# Search for how v8go methods handle nil and if there are any protective measures
rg -n "Value\.Number\|Value\.IsNumber\|Value\.IsArray" --type go | head -20Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Check if there are any places where Number/IsNumber/IsArray are called on potentially nil values
rg -B 3 "\.Number\(\)\|\.IsNumber\(\)\|\.IsArray\(\)" ./scripting/v8engine/ --type go | head -40Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Look at how the code handles undefined in other methods that already have nil checks
sed -n '57,80p' ./scripting/v8engine/value.goRepository: gost-dom/browser
Length of output: 1175
🏁 Script executed:
# Search for any other nil checks on v.Value in the file
grep -n "v\.Value == nil\|v\.Value != nil" ./scripting/v8engine/value.goRepository: gost-dom/browser
Length of output: 149
🏁 Script executed:
# Search for places where these methods are called to understand the actual risk
rg -n "IsNumber\(\)|IsArray\(\)|\.Number\(\)" --type go -B 5 | head -50Repository: gost-dom/browser
Length of output: 3004
🏁 Script executed:
# Check if there are actual callers that might pass nil-wrapped values
rg -n "\.IsArray\|\.IsNumber\|\.Number\(\)" ./scripting/ --type go | head -30Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Verify the v8go.Value type - can we find its source or documentation?
find . -name "*.go" -path "*v8go*" | head -10Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Check where newV8Value is called with potentially nil values
rg -B 5 "newV8Value" ./scripting/v8engine/ --type go | grep -E "newV8Value|:=|return" | head -40Repository: gost-dom/browser
Length of output: 3443
🏁 Script executed:
# Look at the GetPromise case specifically
sed -n '15,25p' ./scripting/v8engine/v8_promise.goRepository: gost-dom/browser
Length of output: 427
🏁 Script executed:
# Check if there are any patterns where values might be nil before wrapping
rg -n "\.Value.*nil\|nil.*Value" ./scripting/v8engine/ --type go | head -20Repository: gost-dom/browser
Length of output: 42
🏁 Script executed:
# Look for Object.Get or similar patterns that might return nil
rg -n "\.Get\(" ./scripting/v8engine/ --type go -A 2 | head -40Repository: gost-dom/browser
Length of output: 854
🏁 Script executed:
# Check if Uint32, Int32, Boolean, String might also need guards
sed -n '57,75p' ./scripting/v8engine/value.goRepository: gost-dom/browser
Length of output: 1042
🏁 Script executed:
# Look for actual usages that might hit nil values
rg -n "\.IsNumber\(\)|\.Number\(\)|\.IsArray\(\)" ./scripting/v8engine/script.go -B 3 -A 1Repository: gost-dom/browser
Length of output: 324
🏁 Script executed:
# Check if there are guard patterns used elsewhere in the codebase
rg -n "if.*\.Value != nil" ./scripting/v8engine/ --type goRepository: gost-dom/browser
Length of output: 42
Add nil checks to Number(), IsNumber(), IsArray(), and other methods to match IsUndefined() guard.
The newV8Value() function is documented to accept nil *v8go.Value to represent undefined, but Number(), IsNumber(), and IsArray() directly call methods on v.Value without null checks, causing panics when undefined values are passed. The IsUndefined() method already guards against nil, establishing a precedent; apply the same pattern to all other methods for consistency.
🐛 Proposed fix
import (
"errors"
"fmt"
+ "math"
"github.com/gost-dom/browser/internal/monads/result"
"github.com/gost-dom/browser/scripting/internal/js"
"github.com/gost-dom/v8go"
)
@@
-func (v v8Value) Number() float64 { return v.Value.Number() }
+func (v v8Value) Number() float64 {
+ if v.Value == nil {
+ return math.NaN()
+ }
+ return v.Value.Number()
+}
@@
-func (v v8Value) IsNumber() bool { return v.Value.IsNumber() }
+func (v v8Value) IsNumber() bool { return v.Value != nil && v.Value.IsNumber() }
@@
-func (v v8Value) IsArray() bool { return v.Value.IsArray() }
+func (v v8Value) IsArray() bool { return v.Value != nil && v.Value.IsArray() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (v v8Value) String() string { return v.Value.String() } | |
| func (v v8Value) Number() float64 { return v.Value.Number() } | |
| func (v v8Value) Int32() int32 { return v.Value.Int32() } | |
| func (v v8Value) Uint32() uint32 { return v.Value.Uint32() } | |
| func (v v8Value) Boolean() bool { return v.Value.Boolean() } | |
| func (v v8Value) IsUndefined() bool { return v.Value == nil || v.Value.IsUndefined() } | |
| func (v v8Value) IsNull() bool { return v.Value.IsNull() } | |
| func (v v8Value) IsBoolean() bool { return v.Value.IsBoolean() } | |
| func (v v8Value) IsString() bool { return v.Value.IsString() } | |
| func (v v8Value) IsNumber() bool { return v.Value.IsNumber() } | |
| func (v v8Value) IsSymbol() bool { return v.Value.IsSymbol() } | |
| func (v v8Value) IsObject() bool { return v.Value.IsObject() } | |
| func (v v8Value) IsArray() bool { return v.Value.IsArray() } | |
| func (v v8Value) String() string { return v.Value.String() } | |
| func (v v8Value) Number() float64 { | |
| if v.Value == nil { | |
| return math.NaN() | |
| } | |
| return v.Value.Number() | |
| } | |
| func (v v8Value) Int32() int32 { return v.Value.Int32() } | |
| func (v v8Value) Uint32() uint32 { return v.Value.Uint32() } | |
| func (v v8Value) Boolean() bool { return v.Value.Boolean() } | |
| func (v v8Value) IsUndefined() bool { return v.Value == nil || v.Value.IsUndefined() } | |
| func (v v8Value) IsNull() bool { return v.Value.IsNull() } | |
| func (v v8Value) IsBoolean() bool { return v.Value.IsBoolean() } | |
| func (v v8Value) IsString() bool { return v.Value.IsString() } | |
| func (v v8Value) IsNumber() bool { return v.Value != nil && v.Value.IsNumber() } | |
| func (v v8Value) IsSymbol() bool { return v.Value.IsSymbol() } | |
| func (v v8Value) IsObject() bool { return v.Value.IsObject() } | |
| func (v v8Value) IsArray() bool { return v.Value != nil && v.Value.IsArray() } |
stroiman
force-pushed
the
feat/structured-clone
branch
from
50056ae to
1e73362
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripting/v8engine/script.go (2)
53-68:⚠️ Potential issue | 🟡 MinorPotential integer overflow when allocating error slice.
At line 57,
errs := make([]error, l+1)could overflow iflequalsmath.MaxUint32, causingl+1to wrap to 0. While unlikely in practice, this could cause index-out-of-bounds panics.🛡️ Proposed fix to prevent overflow
if result.IsArray() { obj, _ := result.AsObject() length, err := obj.Get("length") l := length.Uint32() + if l == math.MaxUint32 { + return nil, errors.New("gost-dom/v8engine: array too large") + } errs := make([]error, l+1) result := make([]any, l)This would require adding
"math"to the imports.
54-56: 🧹 Nitpick | 🔵 TrivialIgnoring error from AsObject after IsArray check.
While
IsArray()being true impliesAsObject()should succeed, silently discarding the error could mask unexpected edge cases. Consider at minimum logging or propagating it.♻️ Proposed improvement
- obj, _ := result.AsObject() + obj, err := result.AsObject() + if err != nil { + return nil, fmt.Errorf("gost-dom/v8engine: array AsObject failed: %w", err) + } - length, err := obj.Get("length") + length, err := obj.Get("length")
| func clone[T any](v Value[T], s Scope[T], objects *[][2]Value[T]) (Value[T], error) { | ||
| switch { | ||
| case v == nil || v.IsUndefined(): | ||
| return s.Undefined(), nil | ||
| case v.IsNull(): | ||
| return s.Null(), nil | ||
| case v.IsUndefined(): | ||
| return s.Undefined(), nil | ||
| case v.IsString(): | ||
| return s.NewString(v.String()), nil | ||
| case v.IsNumber(): | ||
| return s.NewNumber(v.Number()), nil | ||
| case v.IsBoolean(): | ||
| return s.NewBoolean(v.Boolean()), nil | ||
| case v.IsArray(): | ||
| return cloneArray(v, s, objects) | ||
| case v.IsFunction(): | ||
| //TODO: Use correct error | ||
| return nil, errors.New("Serialize function") | ||
| } | ||
| if o, ok := v.AsObject(); ok { | ||
| return cloneObject(o, s, objects) | ||
| } | ||
| return nil, fmt.Errorf("Unable to clone value: %v", v) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove the duplicate IsUndefined branch to avoid dead code.
The first case already handles undefined values, so the later branch is unreachable.
♻️ Proposed cleanup
case v == nil || v.IsUndefined():
return s.Undefined(), nil
case v.IsNull():
return s.Null(), nil
-case v.IsUndefined():
- return s.Undefined(), nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func clone[T any](v Value[T], s Scope[T], objects *[][2]Value[T]) (Value[T], error) { | |
| switch { | |
| case v == nil || v.IsUndefined(): | |
| return s.Undefined(), nil | |
| case v.IsNull(): | |
| return s.Null(), nil | |
| case v.IsUndefined(): | |
| return s.Undefined(), nil | |
| case v.IsString(): | |
| return s.NewString(v.String()), nil | |
| case v.IsNumber(): | |
| return s.NewNumber(v.Number()), nil | |
| case v.IsBoolean(): | |
| return s.NewBoolean(v.Boolean()), nil | |
| case v.IsArray(): | |
| return cloneArray(v, s, objects) | |
| case v.IsFunction(): | |
| //TODO: Use correct error | |
| return nil, errors.New("Serialize function") | |
| } | |
| if o, ok := v.AsObject(); ok { | |
| return cloneObject(o, s, objects) | |
| } | |
| return nil, fmt.Errorf("Unable to clone value: %v", v) | |
| } | |
| func clone[T any](v Value[T], s Scope[T], objects *[][2]Value[T]) (Value[T], error) { | |
| switch { | |
| case v == nil || v.IsUndefined(): | |
| return s.Undefined(), nil | |
| case v.IsNull(): | |
| return s.Null(), nil | |
| case v.IsString(): | |
| return s.NewString(v.String()), nil | |
| case v.IsNumber(): | |
| return s.NewNumber(v.Number()), nil | |
| case v.IsBoolean(): | |
| return s.NewBoolean(v.Boolean()), nil | |
| case v.IsArray(): | |
| return cloneArray(v, s, objects) | |
| case v.IsFunction(): | |
| //TODO: Use correct error | |
| return nil, errors.New("Serialize function") | |
| } | |
| if o, ok := v.AsObject(); ok { | |
| return cloneObject(o, s, objects) | |
| } | |
| return nil, fmt.Errorf("Unable to clone value: %v", v) | |
| } |
| func cloneArray[T any]( | ||
| v Value[T], | ||
| s Scope[T], | ||
| knownObjects *[][2]Value[T], | ||
| ) (Value[T], error) { | ||
| if existing, ok := findKnownValue(v, knownObjects); ok { | ||
| return existing, nil | ||
| } | ||
| o, ok := v.AsObject() | ||
| if !ok { | ||
| return nil, fmt.Errorf( | ||
| "Object was an array, but not convertible to object. %w", | ||
| constants.ErrGostDomBug, | ||
| ) | ||
| } | ||
| res := s.NewArray() | ||
| *knownObjects = append(*knownObjects, [2]Value[T]{v, res}) | ||
|
|
||
| for v, err := range Iterate(o) { | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| cloned, err := clone(v, s, knownObjects) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| res.Push(cloned) | ||
| } | ||
| // TODO: Potential bug here, if the array references itself recursively, | ||
| // this would lead to a stack overflow error. | ||
| return res, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Iterate function definition
rg -n 'func\s+Iterate' -C 5 --type=go
# Search for Array interface definition
rg -n 'type\s+Array\[' -C 5 --type=go
# Search for Push method
rg -n 'func.*\bPush\b' -C 3 --type=go
# Search for Length method
rg -n 'func.*\bLength\b' -C 3 --type=goRepository: gost-dom/browser
Length of output: 4509
🏁 Script executed:
#!/bin/bash
# Get the full Iterate function implementation
sed -n '117,140p' scripting/internal/js/object.go
# Also check what happens after line 140 in that function
sed -n '117,170p' scripting/internal/js/object.go
# Look at how Object iteration is handled (for properties/indices)
rg -n 'for.*range.*Object' -C 3 --type=go
# Check v8engine array iteration implementation
cat scripting/v8engine/array.go
# Check sobekengine array implementation
cat scripting/sobekengine/array.goRepository: gost-dom/browser
Length of output: 3674
🏁 Script executed:
#!/bin/bash
# Search for sparse array handling or tests
rg -i 'sparse' --type=go
# Search for structured-clone references
rg -i 'structured.*clone|clone.*structured' --type=go
# Look at the full clone function to understand the context
sed -n '140,200p' scripting/internal/js/value.go
# Search for any tests related to cloning
fd -e test.go -o -e _test.go | xargs rg -l 'clone|Clone' | head -5Repository: gost-dom/browser
Length of output: 2133
🏁 Script executed:
#!/bin/bash
# Search for array length access or index-based methods
rg -n '\.Length\(\)|\.Get.*\[|for.*i.*<.*Length' -C 2 --type=go | head -50
# Look for tests directory and check for array cloning tests
find . -type f -name '*test*.go' | xargs rg -l 'clone.*[Aa]rray|[Aa]rray.*clone' 2>/dev/null
# Check for any Get(index) or SetIndex methods on Array
rg -n 'func.*\(.*Array\).*Get|func.*\(.*Array\).*SetIndex' -C 2 --type=go
# Search for how array indices are accessed in the codebase
rg -n 'Get.*\(\s*[0-9]' -C 2 --type=go | head -30Repository: gost-dom/browser
Length of output: 4134
🏁 Script executed:
#!/bin/bash
# Let's verify JavaScript array iterator behavior with sparse arrays
# Search for any documentation or comments about array iteration and sparse arrays
rg -i 'hole|sparse|empty.*slot|undefined.*index' -C 3 --type=go
# Check if there are any tests that demonstrate array cloning behavior
fd '.*test.*\.go$' | xargs grep -l 'cloneArray\|clone.*Array' 2>/dev/null
# Look at the complete clone function signature and how cloneObject handles this
sed -n '140,220p' scripting/internal/js/value.go
# Search for reference to structured clone spec
rg -n 'spec|standard|algorithm' -i --type=go | grep -i cloneRepository: gost-dom/browser
Length of output: 4926
🏁 Script executed:
#!/bin/bash
# Check if there's an Array.length getter/setter or any length preservation
rg -n 'func.*Length|\.length|array\.length' -i -C 2 --type=go | head -50
# Search for array iteration tests or examples
find . -type f -name '*.go' | xargs grep -l 'Iterate.*array\|array.*Iterate' 2>/dev/null
# Check how NewArray is created and if it has any special properties
rg -n 'NewArray\(\)' -C 5 --type=go
# Look for any references to array.length preservation
rg -n 'Length.*array|array.*Length' -i -C 3 --type=go | grep -i 'set\|prop\|clone' | head -20
# Check the v8engine and sobekengine implementations of array length
rg -n 'func.*\(.*array\).*Length|func.*Length.*\(.*array' -C 3 --type=goRepository: gost-dom/browser
Length of output: 3547
🏁 Script executed:
#!/bin/bash
# Check if array length is set after cloning
sed -n '168,210p' scripting/internal/js/value.go
# Search for any special array handling after iteration
rg -n 'res\.(Push|Set.*length|Length)' -C 3 --type=go
# Look for how arrays are created and if length is preserved
rg -n 'NewArray' -C 8 --type=go | head -60
# Check if there's any reference to JavaScript array length property
rg -n '\.Get\s*\(\s*"length"' -C 2 --type=go
# Check what the Iterate function returns for sparse arrays specifically
# by looking at how Symbol.iterator is handled
rg -n 'Symbol.*iterator|Symbol\.iterator' -i -C 3 --type=goRepository: gost-dom/browser
Length of output: 12689
🏁 Script executed:
#!/bin/bash
# Let's verify the actual behavior with a comprehensive search
# Check if array.length is accessed anywhere after cloning
rg -n 'Get.*"length"|Get.*length' -C 5 --type=go | grep -A 5 -B 5 clone
# Check how v8engine's NewArray works - does it handle sparse arrays?
sed -n '200,220p' scripting/v8engine/callback_context.go
# Check if there's any code that reads array.length property
rg -n '\.Get\s*\(\s*"length"\s*\)' -C 3 --type=go
# Look for any tests that verify cloning behavior
fd '.*test.*\.go$' -exec grep -l 'structuredClone\|clone.*array\|sparse' {} \;
# Check if Iterate behavior is documented anywhere
rg -n 'Iterate' -B 5 -A 10 scripting/internal/js/object.go | head -80Repository: gost-dom/browser
Length of output: 2429
🏁 Script executed:
#!/bin/bash
# Search for any clone tests or sparse array tests
find . -type f -name '*test.go' | head -10 | xargs grep -l 'clone\|Clone' 2>/dev/null
# Look for structured clone in tests
find . -type f -name '*test.go' -exec grep -l 'structuredClone' {} \;
# Check if there are any array cloning tests
find . -type f -name '*test.go' -exec grep -l 'Array\|array' {} \; | xargs grep -l 'clone' 2>/dev/null
# Verify: does Iterate call the array's Symbol.iterator which should iterate all indices?
# Let's check ObjectGetIterator
rg -n 'func ObjectGetIterator' -A 15 --type=go
# Most importantly: after cloning with Push, is the array.length set correctly?
# Let's search for any length assignment or restoration
rg -n 'res\.(length|Length|Set.*length)' -C 3 --type=goRepository: gost-dom/browser
Length of output: 1120
🏁 Script executed:
#!/bin/bash
# Let's check what the actual behavior of Iterate is for sparse arrays
# by looking at the ObjectEnumerableOwnPropertyKeys which seems related
rg -n 'ObjectEnumerableOwnPropertyKeys|ObjectOwnPropertyKeys' -A 20 --type=go
# Check PropertyDescriptor to understand enumerable property handling
rg -n 'type PropertyDescriptor' -A 10 --type=go
# See how keys/indices are enumerated in ObjectEnumerableOwnPropertyKeys
sed -n '200,250p' scripting/internal/js/object.go
# Check if array length is being preserved after cloning anywhere
rg -n 'array.*length|length.*array' -i -C 3 --type=go | grep -i 'clone\|set'Repository: gost-dom/browser
Length of output: 4696
🏁 Script executed:
#!/bin/bash
# Critical check: does the cloneArray function preserve array length?
# Let's look at the full function and what comes after the Push loop
sed -n '168,210p' scripting/internal/js/value.go
# Check if there's any array.length reading or restoration after cloning
rg -n 'v\.Get\("length"\)|o\.Get\("length"' -C 5 --type=go | grep -A 10 -B 10 clone
# Verify what JavaScript's Array iterator actually does
# Let's search for any documentation or comments about this
rg -n 'Array.*iterator|iterator.*Array' -i -C 3 --type=go
# Most importantly - check if after cloning, the array length is set
sed -n '168,200p' scripting/internal/js/value.goRepository: gost-dom/browser
Length of output: 1874
🏁 Script executed:
#!/bin/bash
# The key question: does JavaScript's Array iterator skip holes or yield them as undefined?
# Let's search for documentation or tests that clarify this
rg -n 'Symbol\.iterator|iterator.*hole|hole.*iterator' -i -C 3 --type=go
# Check if there's any use of array indices (0, 1, 2...) vs just using iterator protocol
rg -n 'for.*i.*:=.*0.*i.*<|Get.*[0-9]|Get.*i|SetIndex|GetIndex' -C 2 --type=go | head -60
# Look for cloning tests to see if sparse arrays are tested
find . -type f -name '*.go' -path '*/test*' -o -name '*_test.go' | xargs grep -l 'Clone\|clone' 2>/dev/null | head -5
# Check how other code handles array length preservation
rg -n '\.Get.*length|\.Set.*length' -C 2 --type=go | grep -i arrayRepository: gost-dom/browser
Length of output: 8797
🏁 Script executed:
#!/bin/bash
# Critical verification: Check if array.length is being accessed or preserved
# during or after cloning
rg -n 'v\.Get\("length"\)|o\.Get\("length"\)|Get.*Length' -C 8 --type=go | grep -B 8 -A 8 'cloneArray\|clone'
# Look specifically at what happens after the cloneArray function
sed -n '168,210p' scripting/internal/js/value.go
# Check if the original array's length is ever read
rg -n 'Get.*"length"|Get.*Length' scripting/internal/js/value.go
# Let's verify the exact behavior by checking what Iterate does
# Get the complete Iterate implementation
sed -n '117,170p' scripting/internal/js/object.goRepository: gost-dom/browser
Length of output: 2238
Preserve sparse array structure during cloning.
The cloneArray implementation converts sparse arrays to dense arrays. JavaScript's Array iterator yields undefined for holes (it doesn't skip them), and pushing those values into a new array makes holes into explicit undefined elements, violating structured-clone behavior. Additionally, the original array's length is never read or preserved, which could cause length mismatches if the source array has non-integer properties or custom descriptors.
Consider reading the original array's length property and preserving array structure through index-based cloning rather than relying on the iterator protocol, or ensure all hole handling aligns with the structured-clone algorithm.
| type dummyContext struct { | ||
| *entity.Entity | ||
| ctx context.Context | ||
| } | ||
|
|
||
| func (c dummyContext) Context() context.Context { return c.ctx } | ||
| func (c dummyContext) HTTPClient() http.Client { return *http.DefaultClient } | ||
| func (c dummyContext) LocationHREF() string { return "http://example.com" } | ||
| func (c dummyContext) Logger() *slog.Logger { return nil } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -nC3 '\.Logger\(\)' --type=go | head -100Repository: gost-dom/browser
Length of output: 5411
🏁 Script executed:
rg -nC3 'dummyContext' --type=goRepository: gost-dom/browser
Length of output: 2982
🏁 Script executed:
rg -nC5 'func.*NewContext' --type=go | head -60Repository: gost-dom/browser
Length of output: 2232
🏁 Script executed:
rg -n 'console\.' scripting/internal/scripttests/ --type=go | head -20Repository: gost-dom/browser
Length of output: 167
Return a no-op logger instead of nil from Logger().
Logger() returns nil, which will cause panics when test scripts invoke console logging methods (e.g., console.log()). The script host's consoleAPIMessage() function calls host.Logger().Debug/Info/Warn/Error directly without nil checks.
🛡️ Proposed fix using slog's default or discard handler
-func (c dummyContext) Logger() *slog.Logger { return nil }
+func (c dummyContext) Logger() *slog.Logger { return slog.New(slog.DiscardHandler) }Alternatively, if slog.DiscardHandler is not available in your Go version:
func (c dummyContext) Logger() *slog.Logger { return slog.Default() }
stroiman
force-pushed
the
feat/structured-clone
branch
from
1e73362 to
99de621
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripting/v8engine/script.go (1)
53-67:⚠️ Potential issue | 🟡 MinorError from
Get("length")is not checked before usinglength.If
obj.Get("length")returns an error,lengthmay be in an invalid state, yetlength.Uint32()is still called on line 56. The error is only aggregated intoerrs[0]but the conversion proceeds regardless.Based on learnings, V8Go returns an
undefinedvalue (not nil) when a property doesn't exist, which safely converts to 0. However, an actual error fromGetcould indicate a more serious problem. Consider either:
- Returning early if
err != nil, or- Explicitly documenting this is intentional because
Geterrors are unlikely for arrays.
| // TODO: Potential bug here, if the array references itself recursively, | ||
| // this would lead to a stack overflow error. | ||
| return res, nil |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO comment appears outdated—cycles are already handled.
The comment suggests recursive arrays could cause stack overflow, but cycle detection via findKnownValue at lines 173-175 should prevent this. The array is registered in knownObjects before iterating its elements, so recursive references will return the existing clone.
Consider removing or updating this TODO to reflect the actual behavior.
| res, err := js.Clone(val, ctx) | ||
| if err != nil { | ||
| t.Errorf("Clone error: %v", err) | ||
| } | ||
| return res, err |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Return early on clone error to avoid returning partial results.
Line 47 logs the error with t.Errorf but execution continues, potentially returning a nil res with a non-nil err. Consider returning immediately after logging or restructuring the error handling.
♻️ Proposed fix
res, err := js.Clone(val, ctx)
if err != nil {
- t.Errorf("Clone error: %v", err)
+ return nil, fmt.Errorf("Clone error: %w", err)
}
return res, errOr if you want to log and return:
res, err := js.Clone(val, ctx)
if err != nil {
t.Errorf("Clone error: %v", err)
+ return nil, err
}
- return res, err
+ return res, nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res, err := js.Clone(val, ctx) | |
| if err != nil { | |
| t.Errorf("Clone error: %v", err) | |
| } | |
| return res, err | |
| res, err := js.Clone(val, ctx) | |
| if err != nil { | |
| return nil, fmt.Errorf("Clone error: %w", err) | |
| } | |
| return res, err |
| res, err := js.Clone(val, ctx) | |
| if err != nil { | |
| t.Errorf("Clone error: %v", err) | |
| } | |
| return res, err | |
| res, err := js.Clone(val, ctx) | |
| if err != nil { | |
| t.Errorf("Clone error: %v", err) | |
| return nil, err | |
| } | |
| return res, nil |
| func (a array) Push(v jsValue) error { | ||
| push := a.obj.Get("push") | ||
| if push == nil { | ||
| return errors.New("gost-dom/sobekengine: array.push: Underlying object doesn't have a push") | ||
| } | ||
| p, ok := sobek.AssertFunction(push) | ||
| if !ok { | ||
| return errors.New( | ||
| "gost-dom/sobekengine: array.push: Underlying object's push is not a function", | ||
| ) | ||
| } | ||
| _, err := p(a.obj, v.Self().value) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Guard against nil value in Push to prevent panic.
If v is nil, calling v.Self() at line 29 will panic. Based on learnings, js.Value[T] can be nil even though it represents a JavaScript value.
🛡️ Proposed fix
func (a array) Push(v jsValue) error {
+ if v == nil {
+ return errors.New("gost-dom/sobekengine: array.push: Cannot push nil value")
+ }
push := a.obj.Get("push")
if push == nil {
return errors.New("gost-dom/sobekengine: array.push: Underlying object doesn't have a push")Alternatively, if pushing undefined is the intended behavior for nil:
func (a array) Push(v jsValue) error {
push := a.obj.Get("push")
if push == nil {
return errors.New("gost-dom/sobekengine: array.push: Underlying object doesn't have a push")
}
p, ok := sobek.AssertFunction(push)
if !ok {
return errors.New(
"gost-dom/sobekengine: array.push: Underlying object's push is not a function",
)
}
- _, err := p(a.obj, v.Self().value)
+ var arg sobek.Value
+ if v != nil {
+ arg = v.Self().value
+ }
+ _, err := p(a.obj, arg)
return err
}Based on learnings: In the gost-dom JavaScript binding system, js.Value[T] is a Go interface that can be nil even though it represents a JavaScript value. Always check v != nil before calling methods.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (a array) Push(v jsValue) error { | |
| push := a.obj.Get("push") | |
| if push == nil { | |
| return errors.New("gost-dom/sobekengine: array.push: Underlying object doesn't have a push") | |
| } | |
| p, ok := sobek.AssertFunction(push) | |
| if !ok { | |
| return errors.New( | |
| "gost-dom/sobekengine: array.push: Underlying object's push is not a function", | |
| ) | |
| } | |
| _, err := p(a.obj, v.Self().value) | |
| return err | |
| } | |
| func (a array) Push(v jsValue) error { | |
| if v == nil { | |
| return errors.New("gost-dom/sobekengine: array.push: Cannot push nil value") | |
| } | |
| push := a.obj.Get("push") | |
| if push == nil { | |
| return errors.New("gost-dom/sobekengine: array.push: Underlying object doesn't have a push") | |
| } | |
| p, ok := sobek.AssertFunction(push) | |
| if !ok { | |
| return errors.New( | |
| "gost-dom/sobekengine: array.push: Underlying object's push is not a function", | |
| ) | |
| } | |
| _, err := p(a.obj, v.Self().value) | |
| return err | |
| } |
| obj, ok := res.AsObject() | ||
| if !ok { | ||
| panic("not ok") | ||
| } | ||
| return newV8Array(f.V8ScriptContext, obj.(*v8Object).Object) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Improve panic message for debugging.
The panic message "not ok" on line 215 provides no context about what failed. Since this is in array construction code, a more descriptive message would help diagnose issues.
♻️ Suggested improvement
obj, ok := res.AsObject()
if !ok {
- panic("not ok")
+ panic("gost-dom/v8engine: NewArray: Array.of result is not an object")
}
return newV8Array(f.V8ScriptContext, obj.(*v8Object).Object)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| obj, ok := res.AsObject() | |
| if !ok { | |
| panic("not ok") | |
| } | |
| return newV8Array(f.V8ScriptContext, obj.(*v8Object).Object) | |
| obj, ok := res.AsObject() | |
| if !ok { | |
| panic("gost-dom/v8engine: NewArray: Array.of result is not an object") | |
| } | |
| return newV8Array(f.V8ScriptContext, obj.(*v8Object).Object) |
1 participant
Create a structured clone algorithm that supports creating a clone accessible in a different script host/context