fix(url): resync cached searchParams when search/href changes - #422
Open
edusperoni wants to merge 1 commit into
Open
fix(url): resync cached searchParams when search/href changes#422edusperoni wants to merge 1 commit into
edusperoni wants to merge 1 commit into
Conversation
The URL.prototype.searchParams getter cached a URLSearchParams built from the query string at first access. A later direct assignment to url.search or url.href left the cache untouched, so subsequent reads returned parameters from the old query, and mutating a held reference wrote that stale set back onto the URL. The cache was also stored as a plain enumerable own property, surfacing in Object.keys(url) and JSON.stringify(url). url.searchParams is a same-object accessor, so the object handed out for a URL must stay identical for the URL's lifetime. Rather than rebuild into a new instance when the query changes, the existing instance is resynced in place — cleared and repopulated through the raw methods captured at construction — and the query string it was built from is recorded alongside it. Mutators resync before applying, so a mutation through a reference held across a reassignment no longer clobbers the new query. Both bookkeeping properties are non-enumerable. The ErrorEvents specs asserted that nothing at all reached the process-wide __onUncaughtError hook during their quiet window; they now assert that the error under test did not, which is what they mean and is immune to async failures draining in from earlier suites.
📝 WalkthroughWalkthroughThe runtime now keeps ChangesURLSearchParams synchronization
Error event assertion updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant URL
participant searchParams
participant URLSearchParamsMethods
URL->>searchParams: Access cached searchParams
searchParams->>URLSearchParamsMethods: Resynchronize from URL.search
URLSearchParamsMethods-->>searchParams: Update existing parameter object
searchParams->>URL: Write serialized mutations to URL.search
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/runtime/js/blob-url.js`:
- Around line 100-124: Update the raw method caches in the searchParams setup to
define _forEach, _append, _delete, _set, and _sort via ObjectDefineProperty with
enumerable set to false while preserving their writable callable values. Extend
the cache test using _searchParams and _searchParamsSource to verify these
internal properties are absent from Object.keys and for...in enumeration.
- Around line 79-128: Update the searchParams initialization logic in the
URL.prototype.searchParams getter so read methods—get, getAll, has, keys,
values, entries, forEach, toString, size, and Symbol.iterator—resync against the
owning URL before returning data. Preserve the existing original-method
delegation pattern used by append, delete, set, and sort, or apply an equivalent
proxy covering every listed access point.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 92c8aca4-3559-4f72-bf56-7375edb6045a
📒 Files selected for processing (4)
NativeScript/runtime/js/blob-url.jsTestRunner/app/tests/ErrorEventsTests.jsTestRunner/app/tests/UrlSearchParamsTests.jsTestRunner/app/tests/index.js
Comment on lines
79
to
128
| ObjectDefineProperty(URL.prototype, 'searchParams', { | ||
| get() { | ||
| if (this._searchParams == null) { | ||
| this._searchParams = new URLSearchParamsCtor(this.search); | ||
| ObjectDefineProperty(this._searchParams, '_url', { | ||
| const params = new URLSearchParamsCtor(this.search); | ||
| ObjectDefineProperty(this, '_searchParams', { | ||
| enumerable: false, | ||
| writable: true, | ||
| configurable: true, | ||
| value: params, | ||
| }); | ||
| ObjectDefineProperty(this, '_searchParamsSource', { | ||
| enumerable: false, | ||
| writable: true, | ||
| configurable: true, | ||
| value: this.search, | ||
| }); | ||
| ObjectDefineProperty(params, '_url', { | ||
| enumerable: false, | ||
| writable: false, | ||
| value: this, | ||
| }); | ||
| this._searchParams._append = this._searchParams.append; | ||
| this._searchParams.append = function (name, value) { | ||
| params._forEach = params.forEach; | ||
| params._append = params.append; | ||
| params.append = function (name, value) { | ||
| resync(this); | ||
| this._append(name, value); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| this._searchParams._delete = this._searchParams.delete; | ||
| this._searchParams.delete = function (name) { | ||
| params._delete = params.delete; | ||
| params.delete = function (name) { | ||
| resync(this); | ||
| this._delete(name); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| this._searchParams._set = this._searchParams.set; | ||
| this._searchParams.set = function (name, value) { | ||
| params._set = params.set; | ||
| params.set = function (name, value) { | ||
| resync(this); | ||
| this._set(name, value); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| this._searchParams._sort = this._searchParams.sort; | ||
| this._searchParams.sort = function () { | ||
| params._sort = params.sort; | ||
| params.sort = function () { | ||
| resync(this); | ||
| this._sort(); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| } else { | ||
| resync(this._searchParams); | ||
| } | ||
| return this._searchParams; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files matching blob-url/tests =="
git ls-files | rg '(^|/)blob-url\.js$|UrlSearchParamsTests\.js$|blob|searchParams|URLSearchParams' || true
echo
echo "== blob-url outline/size =="
if [ -f NativeScript/runtime/js/blob-url.js ]; then
wc -l NativeScript/runtime/js/blob-url.js
ast-grep outline NativeScript/runtime/js/blob-url.js --view compact || true
echo "== relevant blob-url lines =="
sed -n '1,180p' NativeScript/runtime/js/blob-url.js | cat -n
fi
echo
echo "== UrlSearchParamsTests relevant sections =="
if [ -f NativeScript/runtime/js/UrlSearchParamsTests.js ]; then
wc -l NativeScript/runtime/js/UrlSearchParamsTests.js
git diff -- NativeScript/runtime/js/UrlSearchParamsTests.js | sed -n '1,240p'
rg -n "searchParams|search|href|append|get\\(|getAll\\(|has\\(|keys\\(|values\\(|entries\\(|forEach\\(|toString\\(|sort\\(|Symbol|iterator|size" NativeScript/runtime/js/UrlSearchParamsTests.js NativeScript/runtime/js/blob-url.js || true
fiRepository: NativeScript/ios
Length of output: 6055
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file relevant lines =="
wc -l TestRunner/app/tests/UrlSearchParamsTests.js
sed -n '1,260p' TestRunner/app/tests/UrlSearchParamsTests.js | cat -n
echo
echo "== URLSearchParams.js test list around searchParams =="
awk '
/URLSearchParams|searchParams|function \(title\)/{start=NR}
NR>=start && NR<=start+220 || NR>=1 && NR<=220 {print NR": "$0}
' TestRunner/app/tests/URLSearchParams.js | cat -n
echo
echo "== static probe for wrapped and unwrapped URLSearchParams methods in blob-url.js =="
python3 - <<'PY'
from pathlib import Path
p=Path("NativeScript/runtime/js/blob-url.js")
s=p.read_text()
start=s.index("ObjectDefineProperty(URL.prototype, 'searchParams', {")
wrap=s.index("params.append = function", start)
end=None
try:
end=s.index("});\n);", wrap)
except ValueError:
end=s.find("});", wrap + 1)
if wrap==-1:
raise SystemExit("wrapper section not found")
section=s[start:end]
unwrapped_methods="get getAll has keys values entries forEach toString size".split()
for name in unwrapped_methods:
needle=f"params.{name}"
if name != "size":
needle=f"{name}:"
print(f"{name}: wrapper_assign={'params._'+name in section and 'params.'+name+' = function' in section} wrapper_calls_resync={'params._'+name+" in section and "resync(this)\n return this._"+name in section)
print("size" in section)
PYRepository: NativeScript/ios
Length of output: 35078
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate and inspect URLSearchParams implementation =="
fd -a 'URLSearchParamsImpl\.(h|cpp)$' . | while read -r f; do
echo "--- $f ---"
wc -l "$f"
sed -n '1,260p' "$f" | cat -n
done
echo
echo "== check whether searchParams tests cover direct method stale path =="
python3 - <<'PY'
from pathlib import Path
p = Path("TestRunner/app/tests/UrlSearchParamsTests.js")
s = p.read_text()
cases = s.split("it(")
held = []
stale_read_after_url_change = []
for i, case in enumerate(cases[1:], 1):
# crude brace/expect boundary
code = case[:case.find(";)")]
if "url.searchParams" in code and "url.search" in code:
all_sp = code.count("url.searchParams")
else:
all_sp = 0
if "const sp = url.searchParams" in code and "url.search = " in code:
held.append((i, "has held sp after search reassign"))
# Check direct read methods on sp after url.search reassign without reading url.searchParams immediately.
after_assign = code.index("url.search = ")
segment = code[after_assign:]
direct_sp_reads = ["sp get", "sp getAll", "sp has", "sp keys", "sp values", "sp entries", "sp forEach", "sp toString", "sp [Symbol", "sp.iterator", "sp size"]
if "sp.set" in segment or "sp.append" in segment or "sp.delete" in segment or "sp.sort" in segment:
stale_read_after_url_change.append((i, "mutator on sp before direct read"))
else:
found = []
for term in direct_sp_reads:
if term in segment:
found.append(term)
stale_read_after_url_change.append((i, f"has held sp after search reassign; direct reads not present: {found or 'none'}"))
print("held-after-search tests:", len(held))
print("held-after-search expectations involving sp (not url.searchParams):")
for idx, text in held:
print(f" {idx}: {text}")
print("held-after-search stale direct-read status:")
for idx, text in stale_read_after_url_change:
print(f" {idx}: {text}")
PY
echo
echo "== deterministic JS semantics probe for current wrapper coverage =="
node - <<'JS'
const methods = ["get","getAll","has","keys","values","entries","forEach","toString","size"];
const source = require('fs').readFileSync('NativeScript/runtime/js/blob-url.js','utf8');
const section = source.substring(600); // after relevant constants
for (const name of methods) {
const pattern = new RegExp(`params\\.${name}\\s*=\\s*function`)
console.log(name, "has wrapper", pattern.test(source), section.includes(`resync(this)`), section.includes(`return this._${name}`), section.includes(`${name}:`));
}
JSRepository: NativeScript/ios
Length of output: 16590
Make searchParams read methods resync before returning data.
get, getAll, has, keys, values, entries, forEach, toString, size, and Symbol.iterator are currently read directly. Keep a held url.searchParams reference, assign a new search/hash, and call one of these methods; it can return the old query state because only append, delete, set, sort, and the searchParams getter itself resync. Wrap these access points with the same resync(this)/original-delegate pattern, or use a Proxy that resyncs on every access.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/js/blob-url.js` around lines 79 - 128, Update the
searchParams initialization logic in the URL.prototype.searchParams getter so
read methods—get, getAll, has, keys, values, entries, forEach, toString, size,
and Symbol.iterator—resync against the owning URL before returning data.
Preserve the existing original-method delegation pattern used by append, delete,
set, and sort, or apply an equivalent proxy covering every listed access point.
Comment on lines
+100
to
124
| params._forEach = params.forEach; | ||
| params._append = params.append; | ||
| params.append = function (name, value) { | ||
| resync(this); | ||
| this._append(name, value); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| this._searchParams._delete = this._searchParams.delete; | ||
| this._searchParams.delete = function (name) { | ||
| params._delete = params.delete; | ||
| params.delete = function (name) { | ||
| resync(this); | ||
| this._delete(name); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| this._searchParams._set = this._searchParams.set; | ||
| this._searchParams.set = function (name, value) { | ||
| params._set = params.set; | ||
| params.set = function (name, value) { | ||
| resync(this); | ||
| this._set(name, value); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; | ||
| this._searchParams._sort = this._searchParams.sort; | ||
| this._searchParams.sort = function () { | ||
| params._sort = params.sort; | ||
| params.sort = function () { | ||
| resync(this); | ||
| this._sort(); | ||
| this._url.search = this.toString(); | ||
| writeBack(this); | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'blob-url\.js$|UrlSearchParamsTests\.js$' . || true
echo "== git status/stat =="
git diff --stat || true
echo "== blob-url lines 1-160 =="
blob=$(fd 'blob-url\.js$' . | head -n1)
[ -n "${blob:-}" ] && cat -n "$blob" | sed -n '1,180p'
echo "== tests around 130-170 =="
testfile=$(fd 'UrlSearchParamsTests\.js$' . | head -n1)
[ -n "${testfile:-}" ] && cat -n "$testfile" | sed -n '110,170p'
echo "== search ObjectDefineProperty and own keys tests =="
rg -n "ObjectDefineProperty|Object\.keys|OwnPropertyKeys|for\.\\(\\w+ in|ownKeys|searchParams|_url|_searchParams|_searchParamsSource" -S .Repository: NativeScript/ios
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Object.defineProperty enumerable probe =="
node - <<'JS'
const obj = {};
obj._raw = Object.prototype.forEach.call;
Reflect.defineProperty(obj, '_bound', { enumerable: false, writable: true, configurable: true, value: Object.prototype.forEach.call });
console.log(objectKeysContains, forInContains);
function objectKeysContains(prop) {
return Object.keys(obj).includes(prop);
}
function forInContains(prop) {
let found = false;
for (const key in obj) {
if (key === prop) found = true;
}
return found;
}
console.log("_raw in Object.keys:", objectKeysContains("_raw"));
console.log("_raw in for...in:", forInContains("_raw"));
console.log("_bound in Object.keys:", objectKeysContains("_bound"));
console.log("_bound in for...in:", forInContains("_bound"));
console.log("descriptor:", Object.getOwnPropertyDescriptor(obj, "_bound"));
const keys = Object.keys(obj);
for (const key in obj) {
console.log("enumerable key:", key);
}
console.log("has own non-enumerable:", Object.prototype.hasOwnProperty.call(obj, "_bound"));
console.log("hasOwnProperty on enumerable cache key:", Object.prototype.hasOwnProperty.call(obj, "_raw"));
JS
echo "== focused tests =="
cat -n TestRunner/app/tests/UrlSearchParamsTests.js | sed -n '1,220p'Repository: NativeScript/ios
Length of output: 860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Object.prototype assignment/defineProperty enumerable probe =="
node - <<'JS'
const obj = {};
obj._raw = Function.prototype.call.bind(Object.prototype.forEach);
Reflect.defineProperty(obj, '_bound', { enumerable: false, writable: true, configurable: true, value: Function.prototype.call.bind(Object.prototype.forEach) });
function objectKeysContains(prop) {
return Object.keys(obj).includes(prop);
}
function forInContains(prop) {
let found = false;
for (const key in obj) {
if (key === prop) found = true;
}
return found;
}
console.log("_raw in Object.keys:", objectKeysContains("_raw"));
console.log("_raw in for...in:", forInContains("_raw"));
console.log("_bound in Object.keys:", objectKeysContains("_bound"));
console.log("_bound in for...in:", forInContains("_bound"));
console.log("descriptor:", Object.getOwnPropertyDescriptor(obj, "_bound"));
const keys = Object.keys(obj);
for (const key in obj) {
console.log("enumerable key:", key);
}
console.log("has own enumerable cache property:", Object.prototype.hasOwnProperty.call(obj, "_raw"));
JS
echo "== focused UrlSearchParamsTests =="
cat -n TestRunner/app/tests/UrlSearchParamsTests.js | sed -n '1,220p'Repository: NativeScript/ios
Length of output: 6127
Make the params raw method caches non-enumerable.
obj._for = obj.forEach; on url.searchParams creates a writable, enumerable own property, so Object.keys(url.searchParams) and for...in expose _forEach, _append, _delete, _set, and _sort. Use ObjectDefineProperty(params, '_<name>', { enumerable: false, ... }) for each cached method, and cover searchParams enumeration in the cache test with _searchParams/_searchParamsSource.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/js/blob-url.js` around lines 100 - 124, Update the raw
method caches in the searchParams setup to define _forEach, _append, _delete,
_set, and _sort via ObjectDefineProperty with enumerable set to false while
preserving their writable callable values. Extend the cache test using
_searchParams and _searchParamsSource to verify these internal properties are
absent from Object.keys and for...in enumeration.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes two defects in
URL.prototype.searchParamsreported by CodeRabbit on #411: #411 (comment)The defects
URLSearchParamsbuilt from the query string on first access. A later direct assignment tourl.searchorurl.hrefdid not refresh it, so subsequenturl.searchParamsreads returned parameters from the old query. Worse, mutating through a reference held across such an assignment wrote the stale set back onto the URL, silently discarding the newly assigned query._searchParamssurfaced inObject.keys(url)andJSON.stringify(url).Why resync in place instead of rebuilding
CodeRabbit's proposed diff rebuilds the cache into a new
URLSearchParamswhen the recorded query no longer matches. That breaks the WHATWG same-object guarantee —url.searchParamsmust return the identical object for the URL's lifetime, and app code routinely holds a reference to it. A rebuild would leave every existing reference pointing at an object detached from its URL.This PR keeps exactly one
URLSearchParamsper URL forever. When the recorded source string differs from the currentthis.search, the existing object is resynced in place: every key deleted, then re-appended from parsing the current query, then the recorded source updated. The resync goes through the raw methods captured at construction (_append,_delete,_forEach), never the public ones — those are wrapped to write back into_url.searchand would clobber the very string being applied. The wrapped mutators also update the recorded source after writing back, so a mutation through the object never makes the next read look stale, and they resync first so a mutation on a reference held across a reassignment no longer clobbers the new query._searchParamsand_searchParamsSourceare defined non-enumerable via the destructuredObjectDefineProperty; all new intrinsic access goes through primordials (ArrayPrototypePush,FunctionPrototypeCall, both already captured — no additions toprimordials.jsor the eslint list were needed).Tests
New
TestRunner/app/tests/UrlSearchParamsTests.js(12 specs), registered inTestRunner/app/tests/index.js:url.search = ...assignmenturl.searchassignment, afterurl.hrefassignment, and when the query is clearedset/append/delete/sortupdateurl.search/url.href)Object.keys(url),JSON.stringify(url)andfor..indo not expose_searchParams/_searchParamsSourcesearchParamsread parses correctly (includinggetAllandsize)Drive-by: ErrorEvents spec hardening
Three specs in
ErrorEventsTests.jsasserted that nothing at all reached the process-wide__onUncaughtErrorhook during their quiet window. That is inherently order-dependent — an async failure from an earlier suite (the deliberate module-not-found inNodeBuiltinsAndOptionalModulesTests.mjs) can drain into the spy while the test waits, and the extra specs added here shifted timing enough to make it land there. They now assert that the specific error under test did not reach the hook, which is what they actually mean.Suite results
./run_tests.shon the iOS simulator:origin/main): 922 tests, 0 failuresESLint (
npx eslint 'NativeScript/runtime/js/**/*.js') clean;tools/js2c.mjsregen clean.Summary by CodeRabbit
Bug Fixes
URL.searchParamssynchronization when URLs are updated throughsearchorhref.Tests