LibWeb: Make ShadowRealm importValue work #2556
Conversation
An environment settings object will return a copy to the URL. From a quick glance, we could probably make an environment settings object return a reference to one, but let's just change this code to make a copy since its not safe to rely on that.
This was removed from the ShadowRealm HTML spec integration PR after my suggestion as it is not used anywhere, and I don't believe it would ever need to be used in the future or by other specs.
Fixes a crash for module loading for a shadow realm.
| Optional<EnvironmentSettingsObject&> settings_object; | ||
| Optional<URL::URL const&> base_url; | ||
| GC::Ptr<EnvironmentSettingsObject> settings_object; | ||
| Optional<URL::URL> base_url; |
There was a problem hiding this comment.
This makes me wonder why we use Optional<T&> at all
There was a problem hiding this comment.
🤔 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?
There was a problem hiding this comment.
template<typename U = T>
requires(CanBePlacedInOptional<U&>)
ALWAYS_INLINE Optional(U const&& value) = delete;?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Fixes two different crashes, and some minor catchup to spec changes in the open HTML spec PR.