Safe, straightforward API for capturing JS stacks #381

mrowqa · 2017-11-24T23:29:42Z

Can someone take a look if it goes in the right direction and also look through my comments? Maybe, I have left some "stupid questions", but I wanted to post my changes and get feedback before weekend - I work from Europe, so here is middle of the night.

CC: @jdm

This change is

…stacks

emilio · 2017-11-25T16:21:58Z

src/rust.rs

+        // do we want to do the actual logic of conversion in this method or maybe in constructor and just return cached result?
+        unsafe {
+            let stack_handle = self.stack.handle();
+            let nullptr = 0 as *const ::std::os::raw::c_char;


nit: rust has ptr::null() and is_null, so you should use them instead probably.

mrowqa · 2017-11-27T09:38:45Z

Thank you, @emilio.

I met with @Xanewok and the rest of guys and we came up with the idea of capture_stack!(cx) macro. I have also left some other comments waiting for answers.

jdm

Good work!

jdm · 2017-11-27T21:27:52Z

src/rust.rs

+}
+
+impl<'a> CapturedJSStack<'a> {
+    pub fn new(cx: *mut JSContext, mut guard: RootedGuard<'a, *mut JSObject>, max_frame_count: u32) -> Option<Self> { // or maybe another name, for example "capture"?


This needs to be unsafe, because it accepts a raw pointer argument. I think new is a fine name. However, let's make max_frame_count an Option value and use unwrap_or(0) to make it easier to use.

jdm · 2017-11-27T21:28:49Z

src/rust.rs

+
+impl<'a> CapturedJSStack<'a> {
+    pub fn new(cx: *mut JSContext, mut guard: RootedGuard<'a, *mut JSObject>, max_frame_count: u32) -> Option<Self> { // or maybe another name, for example "capture"?
+        let obj_handle = guard.handle_mut(); // if nullptr, return None


if obj_handle.get().is_null()

jdm · 2017-11-27T21:32:46Z

src/rust.rs

+        // TODO where errors can occur? some example of non trivial error handling?
+        // TODO how to write tests for this code? Including the problem of obtaining some stack trace
+        // do we want to do the actual logic of conversion in this method or maybe in constructor and just return cached result?
+        unsafe { // since almost everything here is unsafe, is it okay to wrap it like this?


jdm · 2017-11-27T21:33:34Z

src/rust.rs

+            }
+
+            // wanted somehow to just get the utf-16 string and use String::from_utf16_lossy
+            // but JS_GetTwoByteStringCharsAndLength requires some object of strange type (AutoCheckCannotGC)


rust-mozjs/src/conversions.rs

Lines 515 to 519 in 2b49303

let mut length = 0;

let chars = JS_GetTwoByteStringCharsAndLength(cx, ptr::null(), jsstr, &mut length);

assert!(!chars.is_null());

let char_vec = slice::from_raw_parts(chars, length as usize);

Ok(String::from_utf16_lossy(char_vec)).map(ConversionResult::Success)

Are you sure about passing null pointer to the JS_GetTwoByteStringCharsAndLength? In C++ it requires a reference: https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_Reference/JS_GetLatin1StringCharsAndLength. Maybe it doesn't have any internal fields, but isn't it undefined behavior?

I read the comment before definition (https://dxr.mozilla.org/mozilla-central/source/js/public/GCAPI.h?q=AutoCheckCannotGC&redirect_type=direct#852-876), but I am still not sure when we can use it. Does it mean that we can use it when we have rooted all the objects whose handles we are passing to the function?

If you look at where the nogc parameter is actually used in the function body, you'll see that it's passed as a parameter to twoByteChars, and that function doesn't do anything with it, so I'm quite confident that there's no UB going on here.

In fact, the comment before the definition of AutoCheckCannotGC states that this is only used for static rooting hazard analysis, so I don't see how it can go wrong when you send in a null pointer.

@KiChjang thank you for the explanation. I wasn't sure.

jdm · 2017-11-27T21:35:22Z

src/rust.rs

+            }
+
+            let mut size = 0 as usize;
+            while *str_contents.offset(size as isize) != 0 { // what is some better way of calculating length on raw pointers?


For future reference, CStr::from_ptr is useful here.

jdm · 2017-11-27T21:36:38Z

src/rust.rs

+        // do we want to do the actual logic of conversion in this method or maybe in constructor and just return cached result?
+        unsafe { // since almost everything here is unsafe, is it okay to wrap it like this?
+            let stack_handle = self.stack.handle();
+            rooted!(in(self.cx) let mut js_string = JS_NewStringCopyZ(self.cx, ptr::null()));


It should be possible to just use rooted!(in(self.cx) let mut js_string);.

jdm · 2017-11-27T21:38:08Z

src/rust.rs

+        })
+    }
+
+    pub fn as_string(&self, indent: usize) -> Option<String> {


Use an Option for indent, and unwrap_or(0).

jdm · 2017-11-27T21:39:07Z

src/rust.rs

+#[macro_export]
+macro_rules! capture_stack {
+    ($cx:expr, $max_frame_count:expr) => {
+        rooted!(in($cx) let mut __obj = JS_NewPlainObject($cx));


We should be able to do rooted!(in($cx) let mut __obj); instead.

Right now, we can't. Please take a look at the definition of the macro: https://github.com/servo/rust-mozjs/blob/master/src/rust.rs#L454-L464.
Should I expand it and if yes, would you prefer to do it in this PR or separate one? Another problem is that JS_NewPlainObject returns *mut JSObject and JS_NewCopyStringZ, which I used somewhere else, returns *mut JSString. So, what should be the type of the new binding created within the macro?

Oh, my mistake. It should be possible to use a null pointer instead, though.

So, if some function expects a mutable handle for an output argument, we can just pass a handle to a null pointer? Did I get it right?

Yes. The mutable handle allows the function to initialize it with a meaningful value.

jdm · 2017-11-27T21:41:10Z

src/rust.rs

+    }
+
+    pub fn as_string(&self, indent: usize) -> Option<String> {
+        // TODO where errors can occur? some example of non trivial error handling?


We should verify that IsSavedFrame returns true.

Isn't it guaranted by CaptureCurrentStack after returning true? Is it bad to assume that it will not change since we own the RootedGuard to it?

Yes, but if a consumer does not use the macros and overwrites the value in the Rooted<*mut JSObject> before calling as_string, it won't work. This is probably an instance where we would get a false value from the code in as_string.

jdm · 2017-11-27T21:43:16Z

src/rust.rs

+
+    pub fn as_string(&self, indent: usize) -> Option<String> {
+        // TODO where errors can occur? some example of non trivial error handling?
+        // TODO how to write tests for this code? Including the problem of obtaining some stack trace


We can create an automated test that evaluates some JS that creates several JS stack frames (by defining and invoking functions), then invokes a function that calls into Rust and creates a stack trace. https://github.com/servo/rust-mozjs/blob/master/tests/callback.rs is a good model for this.

mrowqa · 2017-11-29T00:04:59Z

Now it should be almost complete. When I use JS_GetTwoByteStringCharsAndLength in as_string(), the method returns some trash instead of actual stack trace. Probably some problem with text encoding. I left this code commented out. Version with JS_EncodeStringToUTF8 works.

mrowqa · 2017-11-29T23:18:01Z

@jdm I have just removed the commented out code with JS_GetTwoByteStringCharsAndLength. To be honest, I am really confused what it does. I thought JS Engine should keep internally strings encoded in UTF-16 and this function looked like getting a pointer to the internal buffer since it doesn't allocate anything and return const char16_t*. I have printed out the memory and it contains the expected stack, but it looks like it is encoded in UTF-8.

So, since I am really confused what is going on, I have left the version with JS_EncodeStringToUTF8 since it works as expected. The drawback of this is that we have this one extra JS_Free (and associated memory allocation). Moreover, it just cuts out the higher byte of character instead of converting it to, for instance, '?'.

We can merge this, if it is okay with you. And I am willing to learn more about what is going on internally, what these functions are supposed to do and so on :)

mrowqa · 2017-11-30T08:28:07Z

@jdm Travis failed to have installed nightly toolchain, but the code actually works and passes the tests.

jdm · 2017-12-01T06:06:16Z

With respect to the unexpected output of JS_GetTwoByteStringCharsAndLength, that sounds like what the code at https://github.com/Mrowqa/rust-mozjs/blob/e654bb6c238b659e85ac087adf6568eda20e8648/src/conversions.rs#L510-L513 is intended to address. Perhaps we can try extracting https://github.com/Mrowqa/rust-mozjs/blob/e654bb6c238b659e85ac087adf6568eda20e8648/src/conversions.rs#L511-L519 into a method that we can reuse here?

bors-servo · 2017-12-01T06:37:42Z

☔ The latest upstream changes (presumably #382) made this pull request unmergeable. Please resolve the merge conflicts.

…stacks

…to capturing-js-stacks

jdm · 2017-12-01T20:36:10Z

@bors-servo: r+

bors-servo · 2017-12-01T20:36:11Z

📌 Commit c04d888 has been approved by jdm

bors-servo · 2017-12-01T20:36:14Z

⌛ Testing commit c04d888 with merge ba52ca8...

@jdm

Safe, straightforward API for capturing JS stacks PR for servo/servo#14987 Can someone take a look if it goes in the right direction and also look through my comments? Maybe, I have left some "stupid questions", but I wanted to post my changes and get feedback before weekend - I work from Europe, so here is middle of the night. CC: @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/381)

bors-servo · 2017-12-02T01:24:15Z

☀️ Test successful - status-appveyor, status-travis
Approved by: jdm
Pushing ba52ca8 to master...

Started implementation of safe, straightforward API for capturing JS …

a5043ab

…stacks

emilio reviewed Nov 25, 2017

View reviewed changes

Defined capture_stack! macro and did some minor changes

ffddd90

jdm suggested changes Nov 27, 2017

View reviewed changes

jdm reviewed Nov 27, 2017

View reviewed changes

Added test and did changes according to review

0190272

Removed commented out code for getting "two byte" string

e654bb6

Xanewok mentioned this pull request Nov 30, 2017

Reimplement CustomAutoRooter hierarchy #382

Merged

mrowqa and others added 8 commits December 1, 2017 16:54

Started implementation of safe, straightforward API for capturing JS …

5613622

…stacks

Defined capture_stack! macro and did some minor changes

652c412

Added test and did changes according to review

b7bd578

Removed commented out code for getting "two byte" string

4e0c8cf

Extracted part of FromJSValConvertible for String

392a9b3

Merge branch 'capturing-js-stacks' of github.com:Mrowqa/rust-mozjs in…

b818ed4

…to capturing-js-stacks

Merge branch 'master' into capturing-js-stacks

ed5e37a

Merge branch 'master' into capturing-js-stacks

c04d888

bors-servo merged commit c04d888 into servo:master Dec 2, 2017

	let mut length = 0;
	let chars = JS_GetTwoByteStringCharsAndLength(cx, ptr::null(), jsstr, &mut length);
	assert!(!chars.is_null());
	let char_vec = slice::from_raw_parts(chars, length as usize);
	Ok(String::from_utf16_lossy(char_vec)).map(ConversionResult::Success)

Safe, straightforward API for capturing JS stacks #381

Safe, straightforward API for capturing JS stacks #381

Uh oh!

Conversation

mrowqa commented Nov 24, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrowqa commented Nov 27, 2017

Uh oh!

jdm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrowqa Nov 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrowqa commented Nov 29, 2017

Uh oh!

mrowqa commented Nov 29, 2017

Uh oh!

mrowqa commented Nov 30, 2017

Uh oh!

jdm commented Dec 1, 2017

Uh oh!

bors-servo commented Dec 1, 2017

Uh oh!

jdm commented Dec 1, 2017

Uh oh!

bors-servo commented Dec 1, 2017

Uh oh!

bors-servo commented Dec 1, 2017

Uh oh!

bors-servo commented Dec 2, 2017

Uh oh!

Uh oh!

mrowqa commented Nov 24, 2017 •

edited by larsbergstrom

Loading

mrowqa Nov 28, 2017 •

edited

Loading