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

Wrong stack trace in errors shown in combination of for..of loops and JsSetException #6314

Open
goozie81 opened this issue Oct 28, 2019 · 6 comments
Labels

Comments

@goozie81
Copy link

When throwing an error using JsSetException inside a for..of loop error.stack does point to the last line of the for loop instead of the line which has caused the real exception.

for..in however does work properly
throwing inside JavaScript works properly even inside for..of loop
try..catch and rethrow works properly

Example "for..of":

1 function crash() {
2 	Sys.CauseException();
3 }
4 
5 for (let x of [1,2,3]) {
6 	crash();
7 }

error.stack:

Error: Error invoking CLR method CauseException()
   at Global code (test.js:6:2)

Example "for..in":

1 function crash() {
2 	Sys.CauseException();
3 }
4 
5 for (let x in [1,2,3]) {
6 	crash();
7 }

error.stack:

Error: Error invoking CLR method CauseException()
   at crash (test.js:2:2)
   at Global code (test.js:6:2)

Example "rethrow":

1 function crash() {
2 	try {
3 		Sys.CauseException();
4 	} catch(error) } { throw error; }
5 }
6 
7 for (let x in [1,2,3]) {
8 	crash();
9 }

error.stack:

Error: Error invoking CLR method CauseException()
   at crash (test.js:4:19)
   at Global code (test.js:8:2)`

throwing the error in C#:

var error = JavaScriptValue.CreateError(JavaScriptValue.FromString("Error invoking CLR method CauseException()"));
JavaScriptContext.SetException(error);
return JavaScriptValue.Invalid;
@fatcerberus
Copy link
Contributor

See #3685.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 31, 2019

Yeah the problem is the internal workings of for...of which also cause #3685 - though the fix may not be the same.

In the bytecode emitter for of loops are re-written as follows:

Input JS:

for (let x of [1,2,3]) {
  crash();
}

Re-written version (roughly - may have missed a condition somewhere):

const iterator = [1,2,3][Symbol.iterator]();
const next = iterator.next;
let shouldCallReturn = false;
try {
  while(true)
  {
    shouldCallReturn = false;
    const result = next.call(iterator);
    if (! (result instanceof Object))
      throw new TypeError("Iterator should return an object");
    if (result.done)
      break;
    let x = result.value;
    shouldCallReturn = true;
    crash();
  }
} catch (e) {
  if (shouldCallReturn) {
    try { iterator.return(); shouldCallReturn = false; } catch (e) {}
  }
  throw e;
} finally {
  if (shouldCallReturn) {
    if (typeof iterator.return === "function") {
      const result = iterator.return();
      if (! (result instanceof Object))
        throw new TypeError("Iterator should return an object");
  }
}

Yes that is what the spec wants for a for...of implemented using simpler operations - chakracore does it this way as it's easier for the JIT to optimise than using a more complex operation. The problem is that the exception handling gets a little confused - as it thinks there's a try/catch/finally block when the original code didn't contain it.

@goozie81
Copy link
Author

goozie81 commented Nov 1, 2019

@rhuanjl
I am curious why running a for..in loop does produce the correct stack trace.
Does the byte-code of for..in not have any internal try/catch?

@zenparsing
Copy link
Contributor

@goozie81 That's right. The bytecode for for-in looks something like this:

  Line   1: for (var i in obj);
  Col    1: ^
    0009   ProfiledLdRootFld    R2 = root.obj #0 <0>
    000f   ProfiledInitForInEnumerator R2  uint:0  <0>
    0014   ProfiledLoopStart    uint:0
    0016   ProfiledLoopBodyStart uint:0


  Line   1: var i in obj);
  Col    6: ^
    0018   BrOnEmpty            x:0026 (   9)  R2  uint:0
    001d   ProfiledStRootFld    root.i = R2 #1 <1>
    0023   Br                   x:0016 ( -16)
    0026   ProfiledLoopEnd      uint:0

@rhuanjl
Copy link
Collaborator

rhuanjl commented Nov 1, 2019

Thinking about this - the only issue here is that the stack trace is wrong - so just need some way to capture the stack trace from the initial throw rather than re-generating it after calling .return I figure that that should not be too hard.

@fatcerberus
Copy link
Contributor

fatcerberus commented Nov 1, 2019

Note however that will fix this issue but won’t fix #3685.

@rhuanjl rhuanjl added the Bug label Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants