Skip to content

Commit ffdf566

Browse files
jasnelljasnell
committed
Update CfProperty to use jsg::JsObject/jsg::JsValue
1 parent d4d3f60 commit ffdf566

File tree

7 files changed

+87
-62
lines changed

7 files changed

+87
-62
lines changed

‎src/workerd/api/cf-property.c++‎

Lines changed: 43 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,70 +7,77 @@
77

88
namespace workerd::api {
99

10-
static constexpr auto kDefaultBotManagementValue = R"DATA({
10+
static constexpr kj::StringPtr kDefaultBotManagementValue = R"DATA({
1111
"corporateProxy": false,
1212
"verifiedBot": false,
1313
"jsDetection": { "passed": false },
1414
"staticResource": false,
1515
"detectionIds": {},
1616
"score": 99
17-
})DATA";
17+
})DATA"_kjc;
1818

19-
static void handleDefaultBotManagement(jsg::Lock& js, v8::Local<v8::Object> handle) {
19+
static void handleDefaultBotManagement(jsg::Lock& js, jsg::JsObject handle) {
2020
// When the cfBotManagementNoOp compatibility flag is set, we'll check the
2121
// request cf blob to see if it contains a botManagement field. If it does
2222
// *not* we will add it using the following default fields.
2323
// Note that if the botManagement team changes any of the fields they provide,
2424
// this default value may need to be changed also.
25-
auto context = js.v8Context();
26-
if (!js.v8Has(handle, "botManagement"_kj)) {
27-
auto sym = v8::Private::ForApi(js.v8Isolate,
28-
jsg::v8StrIntern(js.v8Isolate, "botManagement"_kj));
25+
static constexpr auto kName = "botManagement"_kjc;
26+
if (!handle.has(js, kName)) {
2927
// For performance reasons, we only want to construct the default values
3028
// once per isolate so we cache the constructed value using an internal
3129
// private field on the global scope. Whenever we need to use it again we
3230
// pull the exact same value.
33-
auto defaultBm = jsg::check(context->Global()->GetPrivate(context, sym));
34-
if (defaultBm->IsUndefined()) {
35-
auto bm = js.parseJson(kj::StringPtr(kDefaultBotManagementValue));
36-
KJ_DASSERT(bm.getHandle(js)->IsObject());
37-
js.recursivelyFreeze(bm);
38-
defaultBm = bm.getHandle(js);
39-
jsg::check(context->Global()->SetPrivate(context, sym, defaultBm));
31+
auto bm = js.global().getPrivate(js, kName);
32+
if (bm.isUndefined()) {
33+
bm = jsg::JsValue::fromJson(js, kDefaultBotManagementValue);
34+
KJ_DASSERT(bm.isObject());
35+
js.global().setPrivate(js, kName, bm);
4036
}
41-
js.v8Set(handle, "botManagement"_kj, defaultBm);
37+
handle.set(js, kName, bm);
4238
}
4339
}
4440

45-
jsg::Optional<v8::Local<v8::Object>> CfProperty::get(jsg::Lock& js) {
46-
return getRef(js).map([&js](jsg::V8Ref<v8::Object>&& ref) mutable {
41+
CfProperty::CfProperty(kj::Maybe<kj::StringPtr> unparsed) {
42+
KJ_IF_MAYBE(str, unparsed) {
43+
value = kj::str(*str);
44+
}
45+
}
46+
47+
CfProperty::CfProperty(jsg::Lock& js, const jsg::JsObject& object)
48+
: CfProperty(kj::Maybe(jsg::JsRef(js, object))) {}
49+
50+
CfProperty::CfProperty(kj::Maybe<jsg::JsRef<jsg::JsObject>>&& parsed) {
51+
KJ_IF_MAYBE(v, parsed) {
52+
value = kj::mv(*v);
53+
}
54+
}
55+
56+
jsg::Optional<jsg::JsObject> CfProperty::get(jsg::Lock& js) {
57+
return getRef(js).map([&js](jsg::JsRef<jsg::JsObject>&& ref) mutable {
4758
return ref.getHandle(js);
4859
});
4960
}
5061

51-
jsg::Optional<jsg::V8Ref<v8::Object>> CfProperty::getRef(jsg::Lock& js) {
62+
jsg::Optional<jsg::JsRef<jsg::JsObject>> CfProperty::getRef(jsg::Lock& js) {
5263
KJ_IF_MAYBE(cf, value) {
5364
KJ_SWITCH_ONEOF(*cf) {
54-
KJ_CASE_ONEOF(parsed, jsg::V8Ref<v8::Object>) {
65+
KJ_CASE_ONEOF(parsed, jsg::JsRef<jsg::JsObject>) {
5566
return parsed.addRef(js);
5667
}
5768
KJ_CASE_ONEOF(unparsed, kj::String) {
58-
auto parsed = js.parseJson(unparsed);
59-
auto handle = parsed.getHandle(js);
60-
KJ_ASSERT(handle->IsObject());
69+
auto parsed = jsg::JsValue::fromJson(js, unparsed);
70+
auto object = KJ_ASSERT_NONNULL(parsed.tryCast<jsg::JsObject>());
6171

62-
auto objectHandle = handle.As<v8::Object>();
6372
if (!FeatureFlags::get(js).getNoCfBotManagementDefault()) {
64-
handleDefaultBotManagement(js, objectHandle);
73+
handleDefaultBotManagement(js, object);
6574
}
6675

67-
// For the inbound request, we make the `cf` blob immutable.
68-
js.recursivelyFreeze(parsed);
76+
object.recursivelyFreeze(js);
6977

7078
// replace unparsed string with a parsed v8 object
71-
auto parsedObject = parsed.cast<v8::Object>(js);
72-
this->value = parsedObject.addRef(js);
73-
return kj::mv(parsedObject);
79+
this->value = object.addRef(js);
80+
return jsg::JsRef(js, object);
7481
}
7582
}
7683
}
@@ -82,14 +89,15 @@ jsg::Optional<jsg::V8Ref<v8::Object>> CfProperty::getRef(jsg::Lock& js) {
8289
kj::Maybe<kj::String> CfProperty::serialize(jsg::Lock& js) {
8390
KJ_IF_MAYBE(cf, value) {
8491
KJ_SWITCH_ONEOF(*cf) {
85-
KJ_CASE_ONEOF(parsed, jsg::V8Ref<v8::Object>) {
86-
return js.serializeJson(parsed);
92+
KJ_CASE_ONEOF(parsed, jsg::JsRef<jsg::JsObject>) {
93+
return jsg::JsValue(parsed.getHandle(js)).toJson(js);
8794
}
8895
KJ_CASE_ONEOF(unparsed, kj::String) {
8996
if (!FeatureFlags::get(js).getNoCfBotManagementDefault()) {
9097
// we mess up with the value on this code path,
9198
// need to parse it, fix it and serialize back
92-
return js.serializeJson(KJ_ASSERT_NONNULL(getRef(js)));
99+
jsg::JsValue handle = KJ_ASSERT_NONNULL(getRef(js)).getHandle(js);
100+
return handle.toJson(js);
93101
}
94102

95103
return kj::str(unparsed);
@@ -103,9 +111,8 @@ kj::Maybe<kj::String> CfProperty::serialize(jsg::Lock& js) {
103111
CfProperty CfProperty::deepClone(jsg::Lock& js) {
104112
KJ_IF_MAYBE(cf, value) {
105113
KJ_SWITCH_ONEOF(*cf) {
106-
KJ_CASE_ONEOF(parsed, jsg::V8Ref<v8::Object>) {
107-
auto ref = parsed.deepClone(js);
108-
return CfProperty(kj::mv(ref));
114+
KJ_CASE_ONEOF(parsed, jsg::JsRef<jsg::JsObject>) {
115+
return CfProperty(jsg::JsRef(js, parsed.getHandle(js).jsonClone(js)));
109116
}
110117
KJ_CASE_ONEOF(unparsed, kj::String) {
111118
return CfProperty(unparsed.asPtr());
@@ -119,7 +126,7 @@ CfProperty CfProperty::deepClone(jsg::Lock& js) {
119126
void CfProperty::visitForGc(jsg::GcVisitor& visitor) {
120127
KJ_IF_MAYBE(cf, value) {
121128
KJ_SWITCH_ONEOF(*cf) {
122-
KJ_CASE_ONEOF(parsed, jsg::V8Ref<v8::Object>) {
129+
KJ_CASE_ONEOF(parsed, jsg::JsRef<jsg::JsObject>) {
123130
visitor.visit(parsed);
124131
}
125132
KJ_CASE_ONEOF_DEFAULT {}

‎src/workerd/api/cf-property.h‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@
1010

1111
namespace workerd::api {
1212

13+
// A holder for Cf header property value.
14+
// The string header is parsed on demand and the parsed value cached.
1315
class CfProperty {
14-
// A holder for Cf header property value.
15-
// The string header is parsed on demand and the parsed value cached.
1616

1717
public:
1818
KJ_DISALLOW_COPY(CfProperty);
@@ -22,34 +22,28 @@ class CfProperty {
2222
CfProperty(CfProperty&&) = default;
2323
CfProperty& operator=(CfProperty&&) = default;
2424

25-
explicit CfProperty(kj::Maybe<kj::StringPtr> unparsed) {
26-
KJ_IF_MAYBE(str, unparsed) {
27-
value = kj::str(*str);
28-
}
29-
}
25+
explicit CfProperty(kj::Maybe<kj::StringPtr> unparsed);
3026

31-
explicit CfProperty(kj::Maybe<jsg::V8Ref<v8::Object>>&& parsed) {
32-
KJ_IF_MAYBE(v, parsed) {
33-
value = kj::mv(*v);
34-
}
35-
}
27+
explicit CfProperty(jsg::Lock& js, const jsg::JsObject& object);
28+
29+
explicit CfProperty(kj::Maybe<jsg::JsRef<jsg::JsObject>>&& parsed);
3630

37-
jsg::Optional<v8::Local<v8::Object>> get(jsg::Lock& js);
3831
// Get parsed value
32+
jsg::Optional<jsg::JsObject> get(jsg::Lock& js);
3933

40-
jsg::Optional<jsg::V8Ref<v8::Object>> getRef(jsg::Lock& js);
4134
// Get parsed value as a global ref
35+
jsg::Optional<jsg::JsRef<jsg::JsObject>> getRef(jsg::Lock& js);
4236

43-
kj::Maybe<kj::String> serialize(jsg::Lock& js);
4437
// Serialize to string
38+
kj::Maybe<kj::String> serialize(jsg::Lock& js);
4539

46-
CfProperty deepClone(jsg::Lock& js);
4740
// Clone by deep cloning parsed v8 object (if any).
41+
CfProperty deepClone(jsg::Lock& js);
4842

4943
void visitForGc(jsg::GcVisitor& visitor);
5044

5145
private:
52-
kj::Maybe<kj::OneOf<kj::String, jsg::V8Ref<v8::Object>>> value;
46+
kj::Maybe<kj::OneOf<kj::String, jsg::JsRef<jsg::JsObject>>> value;
5347
};
5448

5549

‎src/workerd/api/http.c++‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -848,7 +848,10 @@ jsg::Ref<Request> Request::constructor(
848848
}
849849

850850
KJ_IF_MAYBE(newCf, initDict.cf) {
851-
cf = CfProperty(newCf->deepClone(js));
851+
// TODO(cleanup): When initDict.cf is updated to use jsg::JsRef instead
852+
// of jsg::V8Ref, we can clean this up a bit further.
853+
auto cloned = newCf->deepClone(js);
854+
cf = CfProperty(js, jsg::JsObject(cloned.getHandle(js)));
852855
}
853856

854857
KJ_IF_MAYBE(b, kj::mv(initDict.body).orDefault(nullptr)) {
@@ -940,7 +943,7 @@ kj::Maybe<jsg::Ref<AbortSignal>> Request::getSignal() {
940943
return signal.map([](jsg::Ref<AbortSignal>& s) {return s.addRef();});
941944
}
942945

943-
jsg::Optional<v8::Local<v8::Object>> Request::getCf(jsg::Lock& js) {
946+
jsg::Optional<jsg::JsObject> Request::getCf(jsg::Lock& js) {
944947
return cf.get(js);
945948
}
946949

@@ -1042,7 +1045,10 @@ jsg::Ref<Response> Response::constructor(
10421045
}
10431046

10441047
KJ_IF_MAYBE(newCf, initDict.cf) {
1045-
cf = CfProperty(newCf->deepClone(js));
1048+
// TODO(cleanup): When initDict.cf is updated to use jsg::JsRef instead
1049+
// of jsg::V8Ref, we can clean this up a bit further.
1050+
auto cloned = newCf->deepClone(js);
1051+
cf = CfProperty(js, jsg::JsObject(cloned.getHandle(js)));
10461052
}
10471053

10481054
KJ_IF_MAYBE(ws, initDict.webSocket) {
@@ -1379,9 +1385,8 @@ kj::Maybe<jsg::Ref<WebSocket>> Response::getWebSocket(jsg::Lock& js) {
13791385
});
13801386
}
13811387

1382-
jsg::Optional<v8::Local<v8::Object>> Response::getCf(
1383-
const v8::PropertyCallbackInfo<v8::Value>& info) {
1384-
return cf.get(jsg::Lock::from(info.GetIsolate()));
1388+
jsg::Optional<jsg::JsObject> Response::getCf(jsg::Lock& js) {
1389+
return cf.get(js);
13851390
}
13861391

13871392
// =======================================================================================

‎src/workerd/api/http.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -713,7 +713,7 @@ class Request: public Body {
713713

714714
jsg::Ref<AbortSignal> getThisSignal(jsg::Lock& js);
715715

716-
jsg::Optional<v8::Local<v8::Object>> getCf(jsg::Lock& js);
716+
jsg::Optional<jsg::JsObject> getCf(jsg::Lock& js);
717717
// Returns the `cf` field containing Cloudflare feature flags.
718718

719719
bool getKeepalive() { return false; }
@@ -939,7 +939,7 @@ class Response: public Body {
939939

940940
kj::Maybe<jsg::Ref<WebSocket>> getWebSocket(jsg::Lock& js);
941941

942-
jsg::Optional<v8::Local<v8::Object>> getCf(const v8::PropertyCallbackInfo<v8::Value>& info);
942+
jsg::Optional<jsg::JsObject> getCf(jsg::Lock& js);
943943
// Returns the `cf` field containing Cloudflare feature flags.
944944

945945
// v8::Local<v8::Value> getType(jsg::Lock& js) { return js.v8Undefined(); }

‎src/workerd/jsg/jsg.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2133,6 +2133,7 @@ class Lock {
21332133
ContextScope enterContextScope(v8::Local<v8::Context> context);
21342134

21352135
// ====================================================================================
2136+
JsObject global() KJ_WARN_UNUSED_RESULT;
21362137
JsValue undefined() KJ_WARN_UNUSED_RESULT;
21372138
JsValue null() KJ_WARN_UNUSED_RESULT;
21382139
JsBoolean boolean(bool val) KJ_WARN_UNUSED_RESULT;

‎src/workerd/jsg/jsvalue.c++‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,16 @@ kj::String JsObject::getConstructorName() {
9595
return kj::str(inner->GetConstructorName());
9696
}
9797

98+
void JsObject::recursivelyFreeze(Lock& js) {
99+
jsg::recursivelyFreeze(js.v8Context(), inner);
100+
}
101+
102+
JsObject JsObject::jsonClone(Lock& js) {
103+
auto tmp = JsValue(inner).toJson(js);
104+
auto obj = KJ_ASSERT_NONNULL(JsValue::fromJson(js, tmp).tryCast<jsg::JsObject>());
105+
return JsObject(obj);
106+
}
107+
98108
bool JsValue::isTruthy(Lock& js) const {
99109
KJ_ASSERT(!inner.IsEmpty());
100110
return inner->BooleanValue(js.v8Isolate);
@@ -214,6 +224,10 @@ JsDate::operator kj::Date() const {
214224
return kj::UNIX_EPOCH + (int64_t(inner->ValueOf()) * kj::MILLISECONDS);
215225
}
216226

227+
JsObject Lock::global() {
228+
return JsObject(v8Context()->Global());
229+
}
230+
217231
JsValue Lock::undefined() {
218232
return JsValue(v8::Undefined(v8Isolate));
219233
}

‎src/workerd/jsg/jsvalue.h‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,9 @@ class JsObject final : public JsBase<v8::Object, JsObject> {
293293
using JsBase<v8::Object, JsObject>::JsBase;
294294

295295
explicit JsObject(v8::Local<v8::Map> inner);
296+
297+
void recursivelyFreeze(Lock&);
298+
JsObject jsonClone(Lock&);
296299
};
297300

298301
template <typename T>
@@ -371,7 +374,8 @@ class JsRef final {
371374
JsRef(JsRef<T>& other) = delete;
372375
JsRef(JsRef<T>&& other) = default;
373376
template <typename U>
374-
JsRef(Value&& v8Value) : value(kj::mv(v8Value)) {}
377+
JsRef(Lock& js, V8Ref<U>&& v8Value)
378+
: value(js.v8Isolate, v8Value.getHandle(js).template As<v8::Value>()) {}
375379
JsRef& operator=(JsRef<T>& other) = delete;
376380
JsRef& operator=(JsRef<T>&& other) = default;
377381

0 commit comments

Comments
 (0)