Exposing a new JSRT method to get additional information about exceptions by MSLaguana · Pull Request #2936 · chakra-core/ChakraCore

MSLaguana · 2017-05-08T21:15:10Z

This information includes the source file and line/column position of where the exception
came from, as well as the actual source content of the line.

The motivation for this method is for node to print it out on uncaught exceptions.

agarwal-sandeep · 2017-05-08T21:50:24Z

+/// <returns>
+///     The code <c>JsNoError</c> if the operation succeeded, a failure code otherwise.
+/// </returns>
+CHAKRA_API


JsGetAndClearExceptionWithMetadata [](start = 0, length = 34)

@liminzhu Should this be in ChakraCommon.h instead of core only?

At the moment this is still experimental and not yet officially supported, which is why I put it in ChakraCore.h

obastemur · 2017-05-09T17:00:55Z

    return JsNoError;
 }

+CHAKRA_API JsGetAndClearExceptionWithMetadata(_Out_ JsValueRef *metadata)


Apart from the return part, this implementation looks like an exact match to JsGetAndClearException implementation below. In order not to duplicate, could it be better to define a common function and make these two public methods using that?

They do share a common prelude, but they do slightly different things for TTD, and I need to reference a few things from the prelude in the latter part of this function like the scriptContext and the recordedException. We could split the prelude out into a common function but it would be very specific.

did you consider using ContextAPIWrapper, it does bunch of checks already which you are performing in this function?

In reply to: 115546477 [](ancestors = 115546477)

obastemur · 2017-05-09T17:13:39Z

Is it possible to add a test under test/native-tests using this new interface? or using it from CH and a test touching it somehow? ATM CI doesn't test this new interface and if we break it later, we won't know.

MSLaguana · 2017-05-09T17:30:01Z

I've extended one of the tests in JsRTApiTest.cpp to cover the new method.

jianchun · 2017-05-09T17:56:30Z

@MSLaguana A thought of API design. Instead of this, how about keep using the existing JsGetAndClearException, but add a new API like JsGetExceptionMetaData(exception, ...) in which to populate the info you needed? That'll avoid deprecating API and duplicating code.

MSLaguana · 2017-05-09T17:59:43Z

How do you see that flow working? The additional information that we need is accessed through the JavascriptExceptionObject which is cleared and forgotten in JsGetAndClearException. It isn't a javascript value so I don't believe that I can attach it to the exception object, which is a javascript value (and may be any javascript value since it is whatever was thrown).

jianchun · 2017-05-09T18:37:18Z

How do you see that flow working? The additional information that we need is accessed through the JavascriptExceptionObject which is cleared and forgotten in JsGetAndClearException. It isn't a javascript value so I don't believe that I can attach it to the exception object, which is a javascript value (and may be any javascript value since it is whatever was thrown).

Indeed that is a problem. I can only think of an ugly workaround that may have problems (use an InternalProperty to store the exceptionRecord in the thrownObject. Give up, or do ToObject on thrownObject if it is a tagged number.)

Your approach seems fine.

liminzhu · 2017-05-10T18:08:59Z

What's the difference in behavior between JsGetAndClearExceptionWithMetadata and JsGetAndClearException? Is the former exactly hte same as latter except for extra metadata info on the returned object?

liminzhu · 2017-05-10T18:10:25Z

+///     Returns metadata relating to the exception that caused the runtime of the current context
+///     to be in the exception state and resets the exception state for that runtime. The metadata
+///     includes a reference to the exception itself.
+/// </summary>


Can you lay out the returned metadata object in <remarks>? I've always hated that I've no clue what's in exception of JsGetAndClearException without looking into source code.

akroshg · 2017-05-10T18:28:27Z

+        }
+        startCharOffset = cache->GetCharacterOffsetForLine(line, &startByteOffset);
+
+        if (line + 1 >= cache->GetLineCount())


can you use unit32math helper ?

akroshg · 2017-05-10T18:29:16Z

+            // The offsets above point to the start of the following line,
+            // while we need to find the end of the current line.
+            // To do so, just step back over the preceeding newline character(s)
+            if (functionSource[endByteOffset - 1] == _u('\n'))


could this be 0 ? assertorfailfast in beginning?

This must not be 0 since it is the offset for at least the second line. However, the next check could potentially be 0, so I've added a check there.

akroshg · 2017-05-10T18:31:42Z

+
+        Js::Utf8SourceInfo* sourceInfo = functionBody->GetUtf8SourceInfo();
+        sourceInfo->EnsureLineOffsetCache();
+        JsUtil::LineOffsetCache<Recycler> *cache = sourceInfo->GetLineOffsetCache();


Did you consider to move and encapsulate this to functionbody?

akroshg · 2017-05-10T18:33:25Z

+
+        if (line >= cache->GetLineCount())
+        {
+            return false;


if we return false here, all the setproperty we have done above is wasted, isn't it? make sense to move this to top of the function.

akroshg · 2017-05-10T18:40:27Z

+        if (functionBody == nullptr)
+        {
+            // This is probably a parse error. We can get the error location metadata from the thrown object.
+            Js::JavascriptExceptionMetadata::PopulateMetadataFromCompileException(exceptionMetadata, exception, scriptContext);


do you have the test case for this syntax error part in the jsRTApiTest?

akroshg · 2017-05-10T18:42:54Z

        REQUIRE(JsHasException(&value) == JsNoError);
        CHECK(value == true);
+
+        REQUIRE(JsGetAndClearExceptionWithMetadata(&exceptionMetadata) == JsNoError);


would be nice to verify this API in the case where there is no exception thrown?

Done, which required some fixes in the JSRT functions.

MSLaguana · 2017-05-10T22:03:58Z

@liminzhu JsGetAndClearException returns the thrown object from an exception while also clearing the current exception, and exposes no other information. JsGetAndClearExceptionWithMetadata has the same side effect of clearing the current exception, but returns a javascript object which has the thrown object as a property (the exception property) as well as other information: line number, column number, length, line of source code, and script name.

liminzhu · 2017-05-12T00:06:20Z

@MSLaguana gotcha. Is there any reason to use the old API when the new one's added? From an API design perspective, how do you feel about using and old API instead and piling new properties on the returned err object?

MSLaguana · 2017-05-12T14:38:24Z

If you only care about the thrown object, then it's slightly cheaper to invoke the original method, but once this is accepted then I'll be changing node-chakracore to use only the new method since it always wants the additional information.

I would have preferred to re-use the previous method, but that's not possible. The object returned in the original method is the thrown javascript object, which can be any javascript object including a number, so it may not be something with properties (e.g. it may be a number).

liminzhu · 2017-05-12T17:20:36Z

@MSLaguana makes sense. Thanks for the explanation.

akroshg · 2017-05-15T17:48:19Z

+                else
+                {
+                    // This should not be possible
+                    return false;


assert here, just to catch those issues?

akroshg · 2017-05-15T17:49:18Z

+
+        LPCUTF8 functionSource = sourceInfo->GetSource(_u("Jsrt::JsExperimentalGetAndClearExceptionWithMetadata"));
+
+        charcount_t startByteOffset;


nit: initialize locals.

akroshg · 2017-05-15T17:49:54Z

+        charcount_t endCharOffset;
+
+
+        startCharOffset = cache->GetCharacterOffsetForLine(line, &startByteOffset);


did you consider moving this whole thing to function body and encapsulate there?

akroshg · 2017-05-15T17:52:39Z

kfarnung

🚢

…ions This information includes the source file and line/column position of where the exception came from, as well as the actual source content of the line.

… information about exceptions Merge pull request #2936 from MSLaguana:exposeExceptionMetadataR20 This information includes the source file and line/column position of where the exception came from, as well as the actual source content of the line. The motivation for this method is for node to print it out on uncaught exceptions.

…et additional information about exceptions Merge pull request #2936 from MSLaguana:exposeExceptionMetadataR20 This information includes the source file and line/column position of where the exception came from, as well as the actual source content of the line. The motivation for this method is for node to print it out on uncaught exceptions.

liminzhu · 2017-05-16T17:46:54Z

Added API reference in wiki. Please update them if needed in the future.

https://github.com/Microsoft/ChakraCore/wiki/JsGetAndClearExceptionWithMetadata

msftclas added the cla-not-required label May 8, 2017

kfarnung requested a review from mrkmarron May 8, 2017 21:49

agarwal-sandeep reviewed May 8, 2017

View reviewed changes

obastemur reviewed May 9, 2017

View reviewed changes

liminzhu reviewed May 10, 2017

View reviewed changes

akroshg reviewed May 10, 2017

View reviewed changes

akroshg reviewed May 15, 2017

View reviewed changes

kfarnung approved these changes May 15, 2017

View reviewed changes

Exposing a new JSRT method to get additional information about except…

84d61ad

…ions This information includes the source file and line/column position of where the exception came from, as well as the actual source content of the line.

MSLaguana force-pushed the exposeExceptionMetadataR20 branch from 6b1731f to 84d61ad Compare May 15, 2017 18:52

digitalinfinity approved these changes May 15, 2017

View reviewed changes

chakrabot merged commit 84d61ad into chakra-core:release/2.0 May 15, 2017

MSLaguana deleted the exposeExceptionMetadataR20 branch May 16, 2017 00:05


		LPCUTF8 functionSource = sourceInfo->GetSource(_u("Jsrt::JsExperimentalGetAndClearExceptionWithMetadata"));

		charcount_t startByteOffset;

		charcount_t endCharOffset;


		startCharOffset = cache->GetCharacterOffsetForLine(line, &startByteOffset);

Conversation

MSLaguana commented May 8, 2017

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!

obastemur commented May 9, 2017

Uh oh!

MSLaguana commented May 9, 2017

Uh oh!

jianchun commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSLaguana commented May 9, 2017

Uh oh!

jianchun commented May 9, 2017

Uh oh!

liminzhu commented May 10, 2017 • edited 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!

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!

MSLaguana commented May 10, 2017

Uh oh!

liminzhu commented May 12, 2017

Uh oh!

MSLaguana commented May 12, 2017

Uh oh!

liminzhu commented May 12, 2017

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!

akroshg commented May 15, 2017

Uh oh!

kfarnung left a comment

Choose a reason for hiding this comment

Uh oh!

liminzhu commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

jianchun commented May 9, 2017 •

edited

Loading

liminzhu commented May 10, 2017 •

edited

Loading