-
Notifications
You must be signed in to change notification settings - Fork 385
feat(console): Improve console namespace compatibility #1258
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.
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
base: main
Are you sure you want to change the base?
Conversation
richarddavison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor change. Thanks for this once!
| let console = Object::new(ctx.clone())?; | ||
| // NOTE: Console must be created from an empty object with no prototype. | ||
| // https://console.spec.whatwg.org/#console-namespace | ||
| let console = ctx.eval::<Object, &str>("Object.create({})")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be done in qjs directly.
qjs::JS_NewObjectProto add a comment that this should exist as a high level API in rquickjs as well:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried the following implementation, but WPT started failing again. Am I using it wrong?
// let console = ctx.eval::<Object, &str>("Object.create({})")?;
let console = unsafe {
let proto = "{}".into_js(&ctx)?.as_raw();
let js_value = qjs::JS_NewObjectProto(ctx.as_raw().as_ptr(), proto);
Value::from_raw(ctx.clone(), js_value)
.into_object()
.or_throw(&ctx)?
};console > should pass console-is-a-namespace.any.js tests
Error: [The prototype chain must be correct] not an object
console > should pass console-namespace-object-class-string.any.js tests
Error: [@@toStringTag exists on the namespace object with the appropriate descriptor] not a function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the problem @nabetti1720
let proto = "{}".into_js(&ctx)?.as_raw();This is a string "{}" It should be
let photo = Object::new(ctx.clone())?;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richarddavison I tried to fix it as advised, but now I get a new segmentation fault.
let proto = Object::new(ctx.clone())?.as_raw(); // need Object -> JSValuecargo run -- test -d bundle/js/__tests__/wpt 2> wpt_errors.tmp
/bin/sh: line 1: 48807 Segmentation fault: 11 cargo run -- test -d bundle/js/__tests__/wpt 2> wpt_errors.tmp
The same issue occurs when you run the following command on the built binary instead of WPT.
% llrt -e "console.log('foo');"
zsh: segmentation fault llrt -e "console.log('foo');"
% llrt -v
zsh: segmentation fault llrt -v
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a memory issue. When we drop the proto variable it also free's the raw values pointer. So a fix would be to do:
let proto = Object::new(ctx.clone())?
let raw = proto.as_raw();
mem::forget(proto);And then use raw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let console = unsafe {
let proto = Object::new(ctx.clone())?;
let raw = proto.as_raw();
std::mem::forget(proto);
let js_value = qjs::JS_NewObjectProto(ctx.as_raw().as_ptr(), raw);
Value::from_raw(ctx.clone(), js_value)
.into_object()
.or_throw(&ctx)?
};I tried implementing it like this. WPT is now working again, but the following error occurs when the process ends.
Assertion failed: (list_empty(&rt->gc_obj_list)), function JS_FreeRuntime, file quickjs.c, line 2205.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to contribute to a higher level interface in rquickjs, can you open an issue for me @nabetti1720
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sytten I have created the following issue in the rquickjs repository, but I wonder if the intent of the problem is clear?
DelSkayn/rquickjs#572
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tbh I need to brush up on prototype. I didnt even know the base object had a prototype. I think it should be easy enough to create a new method.
| let console = Object::new(ctx.clone())?; | ||
| // NOTE: Console must be created from an empty object with no prototype. | ||
| // https://console.spec.whatwg.org/#console-namespace | ||
| let console = ctx.eval::<Object, &str>("Object.create({})")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a memory issue. When we drop the proto variable it also free's the raw values pointer. So a fix would be to do:
let proto = Object::new(ctx.clone())?
let raw = proto.as_raw();
mem::forget(proto);And then use raw
a3186b6 to
56830a3
Compare
56830a3 to
cd2e7db
Compare
|
F.Y.I blocked by: DelSkayn/rquickjs#572 |
Description of changes
The console implementation has been tweaked to be more web standards compliant.
Checklist
tests/unitand/or in Rust for my feature if neededmake fixto format JS and apply Clippy auto fixesmake checktypes/directoryBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.