Repository navigation
[java][bidi] Decode nested RemoteValues from the parsed tree instead of re-serializing each subtree - #18095
Conversation
…of re-serializing each subtree
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
selenium-ci
added
C-java
|
/agentic_review |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
The BiDi wire decoder change needs human compatibility review, and node decoding currently rejects previously accepted numeric-string counts.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
This PR addresses #18076 by decoding nested Java BiDi RemoteValue data from the parsed tree instead of repeatedly serializing subtrees.
Changes:
- Decode container values and node descendants directly from parsed maps and lists.
- Add unit tests for deep and large values, and a small Bazel test target.
| File | Description |
|---|---|
java/test/org/openqa/selenium/bidi/script/RemoteValueTest.java |
Adds decoding regression tests. |
java/test/org/openqa/selenium/bidi/script/BUILD.bazel |
Runs the new tests as a small target. |
java/src/org/openqa/selenium/bidi/script/RemoteValue.java |
Decodes nested values from the parsed tree. |
java/src/org/openqa/selenium/bidi/script/NodeProperties.java |
Decodes node children and shadow roots directly. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR Previous reviewsReview updated until commit 4d0bc70 Results up to commit 3f4a1ca
|
|
Code review by qodo was updated up to the latest commit 4d0bc70 |
|
No code changes since the last review — review skipped |
Related Issues
Fixes #18076
What does this PR do?
RemoteValue.deserializeValueturned each child back into aRemoteValueby serializing it to JSON text and parsing it again. Each subtree was re-serialized once for every level above it. That made decoding a largelog.entryAddedargument cost gigabytes and seconds, and it failed outright once a value was deeper thanJsonOutput.MAX_DEPTH.This PR adds a package-private
RemoteValue.fromMapthat builds aRemoteValuedirectly from the already-parsedMap/Listtree, so each node is visited once:fromMap.NodePropertiesisn't a leaf.childrenandshadowRootare remote values, and the sharedConstructorCoercerround-trips them too.NodeProperties.fromMapnow decodes them throughRemoteValue.fromMap.nodeType,childNodeCount,localName, attribute values,mode,handle,internalId,sharedId) go throughRemoteValue.coerceScalar, which applies the same JSON coercers as before (so"1"still becomes1L, and123still becomes"123"). Objects and arrays are rejected rather than serialized.RemoteValue.fromJson(JsonInput)keeps its signature and is still the only staticfromJson, soStaticInitializerCoercercontinues to bind it. The new factory is deliberately namedfromMap. No public API changes: all new methods are package-private.Before / after: one decode of each payload, measured with
ThreadMXBean.getCurrentThreadAllocatedBytes(OpenJDK 21, Linux, after one warm-up decode):{index, nested: {value}}(same shape as the .NET check on the issue)Array.from({length: 100000}, (_, i) => i)Depth ceiling: on trunk, a single-key object chain decodes up to depth 34 and fails from depth 35 with
Reached the maximum depth of 100 while writing JSON. With this PR, decoding is no longer bounded byJsonOutput.MAX_DEPTH. It goes as deep as the JSON parser itself can read (about 300 levels on a default thread stack, and 1,000+ with a larger one).Implementation Notes
nullin place of values, extra or missing map-entry items, unknown fields, bad types) through trunk and this branch. Decoded results, concrete collection types (ArrayList,HashMap,LinkedHashMap) and rejected inputs (JsonException) are identical across all of them.nullwhere a remote value is expected decodes tonull, and a map entry with more than two items uses the first two. A map entry with fewer than two items, a missingtype, or a non-array container value throwsJsonException. New error messages name the JSON type (object,array, …) rather than echoing the value, because these payloads can be megabytes.NodeProperties.fromMapkeeps theConstructorCoercerrules:nodeTypeandchildNodeCountare required, the rest are optional, andmodeis matched case-insensitively.Tests
New
RemoteValueTest(small / unit, no browser) injava/test/org/openqa/selenium/bidi/script, with asmall-teststarget. It's excluded from thelarge-testsglob. It has 12 tests:childrentyperejectednull/ extra map-entry items handled as on trunkTo confirm the tests catch the bug, I reverted
RemoteValue/NodePropertiesto trunk with the new tests in place. The two depth tests failed withReached the maximum depth of 100 while writing JSON, and the other ten passed, because they pin the existing behaviour. With the fix, all twelve pass. The tests assert structure, not timing, so they won't be flaky in CI.Additional Considerations
Out of scope here, as discussed on the issue:
BiDi Connectioncached thread pool. Decoding is now much cheaper, but a flood of events can still queue without backpressure.log.entryAdded.One related spot I didn't change:
Script.createEvaluateResultdecodes the evaluate result withJSON.convert(response.get("result"), RemoteValue.class). That is a single top-level round trip, so the cost is linear, butscript.evaluateresults nested deeper than ~33 levels still hitJsonOutput.MAX_DEPTH. Fixing it would mean callingfromMapfrombidi.module, which needs it to be public. Happy to do that here or in a follow-up if you want it.Types of changes