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.

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

Add a basic example #165

Merged
merged 1 commit into from Jul 2, 2015
Merged

Add a basic example #165

merged 1 commit into from Jul 2, 2015

Conversation

@zofrex
Copy link
Contributor

zofrex commented Jul 1, 2015

I am sure this still needs work, but it's a starting point for a basic example - #134. Credit to jdm, Ms2ger, and mbrubeck for helping me get this far!

Things that are missing compared to the SpiderMonkey documentation:

  • JS_InitStandardClasses
  • Error handling (see #164)
@zofrex
Copy link
Contributor Author

zofrex commented Jul 1, 2015

This example works and doesn't crash, but with SM assertions on, it fails:

$ cargo run --features debugmozjs --example callback
Assertion failure: cx->runtime()->requestDepth || cx->runtime()->isHeapBusy(), at /Users/zofrex/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/jscntxt.cpp:1144
An unknown error occurred
@michaelwu
Copy link
Contributor

michaelwu commented Jul 1, 2015

Add in a JSAutoRequest

@zofrex
Copy link
Contributor Author

zofrex commented Jul 1, 2015

Thanks! Getting a different one now, can't figure this one out...

Assertion failure: clasp->trace == JS_GlobalObjectTraceHook, at /Users/zofrex/.cargo/git/checkouts/mozjs-06d7f04b6dbb8a8e/master/mozjs/js/src/vm/GlobalObject.cpp:243

Going to take a break and revisit this tomorrow.

@michaelwu
Copy link
Contributor

michaelwu commented Jul 1, 2015

It wants JS_GlobalObjectTraceHook in trace, but you have trace set to None. Note that you'll need to use the mangled version of that.

@Ms2ger
Copy link
Collaborator

Ms2ger commented Jul 1, 2015

Don't forget the MPL2 header.

@zofrex
Copy link
Contributor Author

zofrex commented Jul 1, 2015

Thanks! Bit horrible that you can't use the JS_GlobalObjectTraceHook wrapper :(

Almost there, it's now failing an assert at the end of evaluate_script in rust.rs:

Assertion failure: mStatementDone, at ../../dist/include/mozilla/GuardObjects.h:100

Looks like it's complaining about a guard object used as temporary, but I can't find one...

@Ms2ger
Copy link
Collaborator

Ms2ger commented Jul 1, 2015

Rooted::new(context, global).handle() looks highly suspicious

@jdm
Copy link
Member

jdm commented Jul 1, 2015

https://github.com/servo/rust-mozjs/pull/165/files#diff-490a245ae6cc96c180d8cb9a9a8eaaa6R50 looks wrong to me - the RootedObject will only be a temporary that will live as long as that statement, and the Handle will point to invalid memory.

@michaelwu
Copy link
Contributor

michaelwu commented Jul 1, 2015

You're making a handle in the puts functions. In general, you can't make manually make handles unless you know exactly what you're doing.

I don't think I can help with the guard object issue without more information to figure out what object the guard object is embedded in.

@zofrex
Copy link
Contributor Author

zofrex commented Jul 1, 2015

I believe I've fixed both the rooting / handle issues. Michael - I don't know how to figure out which object the guard object is embedded in. I tried adding a breakpoint on guard object creation but that was far too noisy to be useful.

@michaelwu
Copy link
Contributor

michaelwu commented Jul 1, 2015

A stack on that assertion would help.

@zofrex
Copy link
Contributor Author

zofrex commented Jul 1, 2015

@michaelwu
Copy link
Contributor

michaelwu commented Jul 1, 2015

This might be an issue with the C++ part of the glue being built without debug/debugmozjs enabled. Try clobbering target/ and building from scratch.

@zofrex
Copy link
Contributor Author

zofrex commented Jul 1, 2015

That works! (And also explains why this code worked without crashing on my other machine...)

Alright, the code compiles and runs with no errors, even with debugmozjs enabled. What's next?

let global_root = Rooted::new(context, global);
let global = global_root.handle();
let _ac = JSAutoCompartment::new(context, global.get());
JS_DefineFunction(context, global, CString::new("puts").unwrap().as_ptr(), Some(puts), 1, 0);

This comment has been minimized.

@jdm

jdm Jul 1, 2015

Member

Instead of CString, this could probably be b"puts\0".as_ptr()

let args = CallArgs::from_vp(vp, argc);

if args.argc_ != 1 {
JS_ReportError(context, CString::new("puts() requires exactly 1 argument").unwrap().as_ptr());

This comment has been minimized.

@jdm

jdm Jul 1, 2015

Member

Same as above.

@zofrex
Copy link
Contributor Author

zofrex commented Jul 2, 2015

It needs a as *const i8 or as *const libc::c_char if I do it the b"something.as_ptr() way. I have no opinion on which is less ugly.

@jdm
Copy link
Member

jdm commented Jul 2, 2015

I'd go with c_char.

@zofrex zofrex force-pushed the zofrex:add-example branch from 6775401 to 4044555 Jul 2, 2015
@jdm
Copy link
Member

jdm commented Jul 2, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2015

📌 Commit 4044555 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2015

Testing commit 4044555 with merge b600fee...

bors-servo pushed a commit that referenced this pull request Jul 2, 2015
bors-servo
Add a basic example

I am sure this still needs work, but it's a starting point for a basic example - #134. Credit to jdm, Ms2ger, and mbrubeck for helping me get this far!

Things that are missing compared to the SpiderMonkey documentation:

* JS_InitStandardClasses
* Error handling (see #164)
@bors-servo
Copy link
Contributor

bors-servo commented Jul 2, 2015

💔 Test failed - cargo-mac

Ms2ger added a commit that referenced this pull request Jul 2, 2015
Add a basic example; r=jdm
@Ms2ger Ms2ger merged commit a76ddc6 into servo:master Jul 2, 2015
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor AppVeyor build failed
Details
homu Test failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.