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

LibWeb: Make ShadowRealm importValue work #2556

Merged
merged 3 commits into from
Nov 24, 2024
Merged

LibWeb: Make ShadowRealm importValue work #2556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c04a4b
LibWeb: Don't store reference to temporary in resolve_module_specifier
shannonbooth Nov 24, 2024
e962237
LibWeb: Remove underlying realm from SyntheticRealmSettings
shannonbooth Nov 24, 2024
075fbc6
LibWeb: Get Page from principal realm during resource loading
shannonbooth Nov 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
15 changes: 6 additions & 9 deletions Libraries/LibWeb/Bindings/MainThreadVM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,29 +583,26 @@ ErrorOr<void> initialize_main_thread_vm(HTML::EventLoop::Type type)
// 5. Set settings's principal realm to O's associated realm
.principal_realm = object.shape().realm(),

// 6. Set settings's underlying realm to realm.
.underlying_realm = realm,

// 7. Set settings's module map to a new module map, initially empty.
// 6. Set settings's module map to a new module map, initially empty.
.module_map = realm.create<HTML::ModuleMap>(),
};

// 8. Set realm.[[HostDefined]] to settings.
// 7. Set realm.[[HostDefined]] to settings.
realm.set_host_defined(make<Bindings::SyntheticHostDefined>(move(settings), realm.create<Bindings::Intrinsics>(realm)));

// 9. Set realm.[[GlobalObject]] to globalObject.
// 8. Set realm.[[GlobalObject]] to globalObject.
realm.set_global_object(global_object);

// 10. Set realm.[[GlobalEnv]] to NewGlobalEnvironment(globalObject, globalObject).
// 9. Set realm.[[GlobalEnv]] to NewGlobalEnvironment(globalObject, globalObject).
realm.set_global_environment(realm.heap().allocate<JS::GlobalEnvironment>(global_object, global_object));

// 11. Perform ? SetDefaultGlobalBindings(realm).
// 10. Perform ? SetDefaultGlobalBindings(realm).
set_default_global_bindings(realm);

// NOTE: This needs to be done after initialization so that the realm has an intrinsics in its [[HostDefined]]
global_object->initialize_web_interfaces();

// 12. Return NormalCompletion(unused).
// 11. Return NormalCompletion(unused).
return {};
};

Expand Down
2 changes: 1 addition & 1 deletion Libraries/LibWeb/Fetch/Fetching/Fetching.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2240,7 +2240,7 @@ WebIDL::ExceptionOr<GC::Ref<PendingResponse>> nonstandard_resource_loader_file_o

auto request = fetch_params.request();

auto& page = Bindings::principal_host_defined_page(realm);
ADKaster marked this conversation as resolved.
Show resolved Hide resolved
auto& page = Bindings::principal_host_defined_page(HTML::principal_realm(realm));

// NOTE: Using LoadRequest::create_for_url_on_page here will unconditionally add cookies as long as there's a page available.
// However, it is up to http_network_or_cache_fetch to determine if cookies should be added to the request.
Expand Down
4 changes: 2 additions & 2 deletions Libraries/LibWeb/HTML/Scripting/Fetching.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ ByteString module_type_from_module_request(JS::ModuleRequest const& module_reque
WebIDL::ExceptionOr<URL::URL> resolve_module_specifier(Optional<Script&> referring_script, ByteString const& specifier)
{
// 1. Let settingsObject and baseURL be null.
Optional<EnvironmentSettingsObject&> settings_object;
Optional<URL::URL const&> base_url;
GC::Ptr<EnvironmentSettingsObject> settings_object;
Optional<URL::URL> base_url;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes me wonder why we use Optional<T&> at all

Copy link
Member

Choose a reason for hiding this comment

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

🤔 Perhaps we should try to make it harder to assign an xvalue or rvalue to an optional T&? @alimpfard any thoughts on how to check for that in AK?

Copy link
Contributor

Choose a reason for hiding this comment

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

    template<typename U = T>
    requires(CanBePlacedInOptional<U&>)
    ALWAYS_INLINE Optional(U const&& value) = delete;

?

Copy link
Member

Choose a reason for hiding this comment

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

const rvalue is a nonsense qualifier, but otherwise yeah, that looks like it would do the trick. + some test coverage, that would make a nice PR

Copy link
Contributor

Choose a reason for hiding this comment

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

You have to go out of your way to make a const&&, but if you manage to, it would pick the U const& over U&&. Anyway, that code doesn't compile right now, an "incompatible operand types" error in LibWeb.

I also notice there is already existing code to try and prevent these kinds of assignments, but I guess it isn't working correctly:

https://github.com/LadybirdBrowser/ladybird/blob/master/AK/Optional.h#L449-L457


// 2. If referringScript is not null, then:
if (referring_script.has_value()) {
Expand Down
1 change: 0 additions & 1 deletion Libraries/LibWeb/HTML/Scripting/SyntheticRealmSettings.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ void SyntheticRealmSettings::visit_edges(JS::Cell::Visitor& visitor)
{
execution_context->visit_edges(visitor);
visitor.visit(principal_realm);
visitor.visit(underlying_realm);
visitor.visit(module_map);
}

Expand Down
4 changes: 0 additions & 4 deletions Libraries/LibWeb/HTML/Scripting/SyntheticRealmSettings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,6 @@ struct SyntheticRealmSettings {
// The principal realm which this synthetic realm exists within.
GC::Ref<JS::Realm> principal_realm;

// An underlying realm
// The synthetic realm which this settings object represents.
GC::Ref<JS::Realm> underlying_realm;

// A module map
// A module map that is used when importing JavaScript modules.
GC::Ref<ModuleMap> module_map;
Expand Down
1 change: 1 addition & 0 deletions Tests/LibWeb/Text/data/external-module.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = "Well hello shadows";
1 change: 1 addition & 0 deletions Tests/LibWeb/Text/expected/HTML/ShadowRealm-importValue.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Well hello shadows
10 changes: 10 additions & 0 deletions Tests/LibWeb/Text/input/HTML/ShadowRealm-importValue.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<script src="../include.js"></script>
<script>
asyncTest(async done => {
const shadowRealm = new ShadowRealm();
const promise = shadowRealm.importValue("../../data/external-module.mjs", "foo");
const value = await promise;
println(value);
done();
});
</script>
Loading