Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: headers refactoring and state transition invariants #80

Merged
merged 4 commits into from
Jul 24, 2024
Merged

fix: headers refactoring and state transition invariants #80

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f867a7e
fix: headers unused code and state transition invariants
guybedford Jul 22, 2024
a97d1da
more invariants
guybedford Jul 22, 2024
04dbf3b
Update builtins/web/fetch/headers.cpp
guybedford Jul 24, 2024
1d40a92
Merge branch 'main' into invariant-tweaks
guybedford Jul 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 20 additions & 31 deletions builtins/web/fetch/headers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,18 +283,12 @@ static bool switch_mode(JSContext* cx, HandleObject self, const Headers::Mode mo
}

if (current_mode == Headers::Mode::Uninitialized) {
if (mode == Headers::Mode::ContentOnly) {
RootedObject map(cx, JS::NewMapObject(cx));
if (!map) {
return false;
}
SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Entries), ObjectValue(*map));
} else {
MOZ_ASSERT(mode == Headers::Mode::HostOnly);
auto handle = new host_api::HttpHeaders(Headers::guard(self));
SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Handle), PrivateValue(handle));
MOZ_ASSERT(mode == Headers::Mode::ContentOnly);
RootedObject map(cx, JS::NewMapObject(cx));
if (!map) {
return false;
}

SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Entries), ObjectValue(*map));
SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Mode), JS::Int32Value(static_cast<int32_t>(mode)));
return true;
}
Expand Down Expand Up @@ -403,8 +397,6 @@ static bool switch_mode(JSContext* cx, HandleObject self, const Headers::Mode mo
auto handle = get_handle(self);
delete handle;
SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Handle), PrivateValue(nullptr));
SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Mode),
JS::Int32Value(static_cast<int32_t>(Headers::Mode::CachedInContent)));
}

SetReservedSlot(self, static_cast<size_t>(Headers::Slots::Mode),
Expand Down Expand Up @@ -519,14 +511,6 @@ bool Headers::append_header_value(JSContext *cx, JS::HandleObject self, JS::Hand
return true;
}

void init_from_handle(JSObject* self, host_api::HttpHeadersReadOnly* handle) {
MOZ_ASSERT(Headers::is_instance(self));
MOZ_ASSERT(Headers::mode(self) == Headers::Mode::Uninitialized);
SetReservedSlot(self, static_cast<uint32_t>(Headers::Slots::Mode),
JS::Int32Value(static_cast<int32_t>(Headers::Mode::HostOnly)));
SetReservedSlot(self, static_cast<uint32_t>(Headers::Slots::Handle), PrivateValue(handle));
}

JSObject *Headers::create(JSContext *cx, host_api::HttpHeadersGuard guard) {
JSObject* self = JS_NewObjectWithGivenProto(cx, &class_, proto_obj);
if (!self) {
Expand All @@ -546,7 +530,10 @@ JSObject *Headers::create(JSContext *cx, host_api::HttpHeadersReadOnly *handle,
return nullptr;
}

init_from_handle(self, handle);
MOZ_ASSERT(Headers::mode(self) == Headers::Mode::Uninitialized);
SetReservedSlot(self, static_cast<uint32_t>(Headers::Slots::Mode),
JS::Int32Value(static_cast<int32_t>(Headers::Mode::HostOnly)));
SetReservedSlot(self, static_cast<uint32_t>(Headers::Slots::Handle), PrivateValue(handle));
return self;
}

Expand All @@ -555,24 +542,28 @@ JSObject *Headers::create(JSContext *cx, HandleValue init_headers, host_api::Htt
if (!self) {
return nullptr;
}
return init_entries(cx, self, init_headers);
if (!init_entries(cx, self, init_headers)) {
return nullptr;
}
MOZ_ASSERT(mode(self) == Headers::Mode::ContentOnly || mode(self) == Headers::Mode::Uninitialized);
tschneidereit marked this conversation as resolved.
Show resolved Hide resolved
return self;
}

JSObject *Headers::init_entries(JSContext *cx, HandleObject self, HandleValue initv) {
bool Headers::init_entries(JSContext *cx, HandleObject self, HandleValue initv) {
// TODO: check if initv is a Headers instance and clone its handle if so.
// TODO: But note: forbidden headers have to be applied correctly.
bool consumed = false;
if (!core::maybe_consume_sequence_or_record<append_header_value>(cx, initv, self,
&consumed, "Headers")) {
return nullptr;
return false;
}

if (!consumed) {
api::throw_error(cx, api::Errors::InvalidSequence, "Headers", "");
return nullptr;
return false;
}

return self;
return true;
}

bool Headers::get(JSContext *cx, unsigned argc, JS::Value *vp) {
Expand Down Expand Up @@ -879,12 +870,10 @@ bool Headers::constructor(JSContext *cx, unsigned argc, JS::Value *vp) {
}
SetReservedSlot(headersInstance, static_cast<uint32_t>(Slots::Guard),
JS::Int32Value(static_cast<int32_t>(host_api::HttpHeadersGuard::None)));
JS::RootedObject headers(cx, init_entries(cx, headersInstance, headersInit));
if (!headers) {
if (!init_entries(cx, headersInstance, headersInit)) {
return false;
}

args.rval().setObject(*headers);
args.rval().setObject(*headersInstance);
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion builtins/web/fetch/headers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class Headers final : public BuiltinImpl<Headers> {
static JSObject *create(JSContext *cx, host_api::HttpHeadersReadOnly *handle,
host_api::HttpHeadersGuard guard);

static JSObject *init_entries(JSContext *cx, HandleObject self, HandleValue init_headers);
static bool init_entries(JSContext *cx, HandleObject self, HandleValue init_headers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make a decision at some point about how we version starlingmonkey and how we communicate public function signatures changes such as this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From js-compute-runtime's perspective, every StarlingMonkey commit is potentially a breaking change. There simply are no API guarantees.

But since these are changes on the js-compute-runtime -> StarlingMonkey interface, they are all manageable in theory.

Problems come into play if:

  1. A StarlingMonkey change causes the corresponding js-compute-runtime upgrade to be an inhibitative amount of work.
  2. A StarlingMonkey change causes a breaking change in js-compute-runtimes's public guest API usage for JS users.

I hope our implicit agreement is effectively that we could revert a change in the case of (1) or (2), but perhaps we should discuss this more with @tschneidereit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this as a sentiment for now, yes. Eventually I would like to get to a more stable API, but I don't think the one we have right now is sufficiently well designed to declare it stable as-is.


/// Returns a Map object containing the headers.
///
Expand Down
Loading