servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

Safe, straightforward API for capturing JS stacks #381

Closed mrowqa closed 6 years ago

mrowqa commented 6 years ago

PR for https://github.com/servo/servo/issues/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 Reviewable

mrowqa commented 6 years ago

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.

mrowqa commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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

jdm commented 6 years ago

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 commented 6 years ago

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

jdm commented 6 years ago

@bors-servo: r+

bors-servo commented 6 years ago

:pushpin: Commit c04d888 has been approved by jdm

bors-servo commented 6 years ago

:hourglass: Testing commit c04d8881132613d8eb9e0418b844ee1f8a7ca9b0 with merge ba52ca890eab15f30a035e007bdd24c7c728455d...

bors-servo commented 6 years ago

:sunny: Test successful - status-appveyor, status-travis Approved by: jdm Pushing ba52ca890eab15f30a035e007bdd24c7c728455d to master...