Skip to content

Commit 7b5fa11

Browse files
mattermoranmattermoran
committed
fix(url): make URLSearchParams record constructor spec compliant
1 parent faa9114 commit 7b5fa11

File tree

4 files changed

+166
-97
lines changed

4 files changed

+166
-97
lines changed

‎builtins/web/fetch/headers.cpp‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -649,10 +649,23 @@ bool Headers::init_entries(JSContext *cx, HandleObject self, HandleValue initv)
649649
// TODO: check if initv is a Headers instance and clone its handle if so.
650650
// TODO: But note: forbidden headers have to be applied correctly.
651651
bool consumed = false;
652-
if (!core::maybe_consume_sequence_or_record<host_api::HostString, validate_header_name,
653-
append_valid_header>(cx, initv, self, &consumed,
654-
"Headers")) {
655-
return false;
652+
653+
if (initv.isUndefined()) {
654+
consumed = true;
655+
}
656+
657+
if (!consumed) {
658+
if (!core::maybe_consume_sequence<host_api::HostString, validate_header_name,
659+
append_valid_header>(cx, initv, self, &consumed, "Headers")) {
660+
return false;
661+
}
662+
}
663+
664+
if (!consumed) {
665+
if (!core::maybe_consume_record<host_api::HostString, validate_header_name,
666+
append_valid_header>(cx, initv, self, &consumed, "Headers")) {
667+
return false;
668+
}
656669
}
657670

658671
if (!consumed) {

‎builtins/web/url.cpp‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,18 @@ bool append_impl(JSContext *cx, JS::HandleObject self, jsurl::SpecString key, JS
198198
jsurl::params_append(params, key, value);
199199
return true;
200200
}
201+
bool set_impl(JSContext *cx, JS::HandleObject self, jsurl::SpecString key,
202+
JS::HandleValue val, const char *_) {
203+
auto *const params = URLSearchParams::get_params(self);
204+
205+
auto value = core::encode_spec_string(cx, val);
206+
if (!value.data) {
207+
return false;
208+
}
209+
210+
jsurl::params_set(params, key, value);
211+
return true;
212+
}
201213
} // namespace
202214

203215
jsurl::SpecSlice URLSearchParams::serialize(JSContext *cx, JS::HandleObject self) {
@@ -407,9 +419,23 @@ JSObject *URLSearchParams::create(JSContext *cx, JS::HandleObject self,
407419

408420
bool consumed = false;
409421
const char *alt_text = ", or a value that can be stringified";
410-
if (!core::maybe_consume_sequence_or_record<jsurl::SpecString, append_impl_validate, append_impl>(
411-
cx, params_val, self, &consumed, "URLSearchParams", alt_text)) {
412-
return nullptr;
422+
423+
if (params_val.isUndefined()) {
424+
consumed = true;
425+
}
426+
427+
if (!consumed) {
428+
if (!core::maybe_consume_sequence<jsurl::SpecString, append_impl_validate, append_impl>(
429+
cx, params_val, self, &consumed, "URLSearchParams", alt_text)) {
430+
return nullptr;
431+
}
432+
}
433+
434+
if (!consumed) {
435+
if (!core::maybe_consume_record<jsurl::SpecString, append_impl_validate, set_impl>(
436+
cx, params_val, self, &consumed, "URLSearchParams")) {
437+
return nullptr;
438+
}
413439
}
414440

415441
if (!consumed) {
@@ -831,5 +857,3 @@ bool install(api::Engine *engine) {
831857
}
832858

833859
} // namespace builtins::web::url
834-
835-

‎runtime/sequence.hpp‎

Lines changed: 118 additions & 86 deletions
Original file line numberDiff line numberDiff line change
@@ -10,22 +10,23 @@
1010
namespace core {
1111

1212
/**
13-
* Extract <key,value> pairs from the given value if it is either a
14-
* sequence<sequence<Value> or a record<Value, Value>.
13+
* Extract <key,value> pairs from the given value if it is a
14+
* sequence<sequence<Value>>.
1515
*/
1616
template <typename T, auto validate, auto apply>
17-
bool maybe_consume_sequence_or_record(JSContext *cx, JS::HandleValue initv, JS::HandleObject target,
18-
bool *consumed, const char *ctor_name,
19-
const char *alt_text = "") {
20-
if (initv.isUndefined()) {
21-
*consumed = true;
17+
bool maybe_consume_sequence(JSContext *cx, JS::HandleValue initv, JS::HandleObject target,
18+
bool *consumed, const char *ctor_name,
19+
const char *alt_text = "") {
20+
*consumed = false;
21+
22+
if (!initv.isObject()) {
2223
return true;
2324
}
2425

2526
JS::RootedValue key(cx);
2627
JS::RootedValue value(cx);
2728

28-
// First, try consuming args[0] as a sequence<sequence<Value>>.
29+
// Try consuming args[0] as a sequence<sequence<Value>>.
2930
JS::ForOfIterator it(cx);
3031
if (!it.init(initv, JS::ForOfIterator::AllowNonIterable)) {
3132
return false;
@@ -34,109 +35,140 @@ bool maybe_consume_sequence_or_record(JSContext *cx, JS::HandleValue initv, JS::
3435
// Note: this currently doesn't treat strings as iterable even though they
3536
// are. We don't have any constructors that want to iterate over strings, and
3637
// this makes things a lot easier.
37-
if (initv.isObject() && it.valueIsIterable()) {
38-
JS::RootedValue entry(cx);
39-
40-
while (true) {
41-
bool done = false;
42-
if (!it.next(&entry, &done)) {
43-
return false;
44-
}
38+
if (!it.valueIsIterable()) {
39+
return true;
40+
}
4541

46-
if (done) {
47-
break;
48-
}
42+
JS::RootedValue entry(cx);
4943

50-
if (!entry.isObject()) {
51-
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
52-
}
53-
54-
JS::ForOfIterator entr_iter(cx);
55-
if (!entr_iter.init(entry, JS::ForOfIterator::AllowNonIterable)) {
56-
return false;
57-
}
44+
while (true) {
45+
bool done = false;
46+
if (!it.next(&entry, &done)) {
47+
return false;
48+
}
5849

59-
if (!entr_iter.valueIsIterable()) {
60-
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
61-
}
50+
if (done) {
51+
break;
52+
}
6253

63-
{
64-
bool done = false;
65-
66-
// Extract key.
67-
if (!entr_iter.next(&key, &done)) {
68-
return false;
69-
}
70-
if (done) {
71-
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
72-
}
73-
74-
T validated_key = validate(cx, key, ctor_name);
75-
if (!validated_key) {
76-
return false;
77-
}
78-
79-
// Extract value.
80-
if (!entr_iter.next(&value, &done)) {
81-
return false;
82-
}
83-
if (done) {
84-
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
85-
}
86-
87-
// Ensure that there aren't any further entries.
88-
if (!entr_iter.next(&entry, &done)) {
89-
return false;
90-
}
91-
if (!done) {
92-
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
93-
}
94-
95-
if (!apply(cx, target, std::move(validated_key), value, ctor_name)) {
96-
return false;
97-
}
98-
}
54+
if (!entry.isObject()) {
55+
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
9956
}
100-
*consumed = true;
101-
} else if (initv.isObject()) {
102-
// init isn't an iterator, so if it's an object, it must be a record to be
103-
// valid input, following https://webidl.spec.whatwg.org/#js-record exactly.
104-
JS::RootedObject init(cx, &initv.toObject());
105-
JS::RootedIdVector ids(cx);
106-
if (!js::GetPropertyKeys(cx, init, JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, &ids)) {
57+
58+
JS::ForOfIterator entr_iter(cx);
59+
if (!entr_iter.init(entry, JS::ForOfIterator::AllowNonIterable)) {
10760
return false;
10861
}
10962

110-
JS::RootedId curId(cx);
63+
if (!entr_iter.valueIsIterable()) {
64+
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
65+
}
11166

112-
JS::Rooted<mozilla::Maybe<JS::PropertyDescriptor>> desc(cx);
67+
{
68+
bool done = false;
11369

114-
for (size_t i = 0; i < ids.length(); ++i) {
115-
curId = ids[i];
116-
key = js::IdToValue(curId);
117-
if (!JS_GetOwnPropertyDescriptorById(cx, init, curId, &desc)) {
70+
// Extract key.
71+
if (!entr_iter.next(&key, &done)) {
11872
return false;
11973
}
120-
if (desc.isNothing() || !desc->enumerable()) {
121-
continue;
74+
if (done) {
75+
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
12276
}
123-
// Get call is observable and must come after any value validation
77+
12478
T validated_key = validate(cx, key, ctor_name);
12579
if (!validated_key) {
12680
return false;
12781
}
128-
if (!JS_GetPropertyById(cx, init, curId, &value)) {
82+
83+
// Extract value.
84+
if (!entr_iter.next(&value, &done)) {
12985
return false;
13086
}
87+
if (done) {
88+
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
89+
}
90+
91+
// Ensure that there aren't any further entries.
92+
if (!entr_iter.next(&entry, &done)) {
93+
return false;
94+
}
95+
if (!done) {
96+
return api::throw_error(cx, api::Errors::InvalidSequence, ctor_name, alt_text);
97+
}
98+
13199
if (!apply(cx, target, std::move(validated_key), value, ctor_name)) {
132100
return false;
133101
}
134102
}
135-
*consumed = true;
136-
} else {
137-
*consumed = false;
138103
}
139104

105+
*consumed = true;
106+
return true;
107+
}
108+
109+
/**
110+
* Extract <key,value> pairs from the given value if it is a
111+
* record<Value, Value>.
112+
*/
113+
template <typename T, auto validate, auto apply>
114+
bool maybe_consume_record(JSContext *cx, JS::HandleValue initv, JS::HandleObject target,
115+
bool *consumed, const char *ctor_name) {
116+
*consumed = false;
117+
118+
if (!initv.isObject()) {
119+
return true;
120+
}
121+
122+
JS::RootedValue key(cx);
123+
JS::RootedValue value(cx);
124+
125+
// If init is iterable, it is not a record.
126+
JS::ForOfIterator it(cx);
127+
if (!it.init(initv, JS::ForOfIterator::AllowNonIterable)) {
128+
return false;
129+
}
130+
if (it.valueIsIterable()) {
131+
return true;
132+
}
133+
134+
// init isn't an iterator, so if it's an object, it must be a record to be
135+
// valid input, following https://webidl.spec.whatwg.org/#js-record exactly.
136+
JS::RootedObject init(cx, &initv.toObject());
137+
JS::RootedIdVector ids(cx);
138+
if (!js::GetPropertyKeys(cx, init, JSITER_OWNONLY | JSITER_HIDDEN | JSITER_SYMBOLS, &ids)) {
139+
return false;
140+
}
141+
142+
JS::RootedId curId(cx);
143+
JS::Rooted<mozilla::Maybe<JS::PropertyDescriptor>> desc(cx);
144+
145+
for (size_t i = 0; i < ids.length(); ++i) {
146+
curId = ids[i];
147+
key = js::IdToValue(curId);
148+
149+
if (!JS_GetOwnPropertyDescriptorById(cx, init, curId, &desc)) {
150+
return false;
151+
}
152+
if (desc.isNothing() || !desc->enumerable()) {
153+
continue;
154+
}
155+
156+
// Get call is observable and must come after any value validation
157+
T validated_key = validate(cx, key, ctor_name);
158+
if (!validated_key) {
159+
return false;
160+
}
161+
162+
if (!JS_GetPropertyById(cx, init, curId, &value)) {
163+
return false;
164+
}
165+
166+
if (!apply(cx, target, std::move(validated_key), value, ctor_name)) {
167+
return false;
168+
}
169+
}
170+
171+
*consumed = true;
140172
return true;
141173
}
142174

‎tests/wpt-harness/expectations/url/urlsearchparams-constructor.any.js.json‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,10 @@
6969
"status": "PASS"
7070
},
7171
"Construct with 2 unpaired surrogates (no trailing)": {
72-
"status": "FAIL"
72+
"status": "PASS"
7373
},
7474
"Construct with 3 unpaired surrogates (no leading)": {
75-
"status": "FAIL"
75+
"status": "PASS"
7676
},
7777
"Construct with object with NULL, non-ASCII, and surrogate keys": {
7878
"status": "PASS"

0 commit comments

Comments
 (0)