Move re-throw of exception out of catch #5497

IrinaYatsenko · 2018-07-20T21:19:37Z

Fixes OS.18246154

Re-throw from catch doesn't unwind the stack (only resets the active frame pointer) so, if there are nested try/catch blocks that refrow, the stack keeps growing, which is unhelpful if the initial exception is trying to prevent Stack Overflow. To avoid this, move re-throw outside of the catch (so the stack fully unwinds on each handling site).

Took the opportunity to add more test coverage for typeof and fix exception handling deviations from the spec (and V8).

LouisLaf · 2018-07-20T22:12:39Z

lib/Runtime/Language/JavascriptOperators.h

    } \
    catch (const JavascriptException& err) \
    { \
-        JavascriptExceptionObject* exceptionObject = err.GetAndClear(); \


It still seem wrong though to not propagate the stack overflow in this case. The rest of the change looks fine. #Closed

maybe we should propagate instead of swallow, exceptions other than the script exceptions we explicitly swallow. #Resolved

Propagating it now and added a test for it.

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

per http://es-discourse.com/t/why-typeof-is-no-longer-safe/15 it seems that the general expectation for "typeof" to not throw so I'm going to keep it as is (I couldn't find it in the spec though)

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

From my reading of the spec, I think that we should throw. The spec says that first you evaluate the expression, then you set val to ? GetValue(val). GetValue checks if val is an "abrupt completion", which includes a throw, and returns it if so, and the ? expression syntax means that you should do the same; if the expression returns an abrupt completion, then return that abrupt completion.

If I've parsed that out right, then typeof(expressionThatThrows) should have the exception propagate.

@zenparsing does that sound correct? #Resolved

If typeof operator isn't supposed to be safe, why do we have BEGIN_TYPEOF_ERROR_HANDLER and END_TYPEOF_ERROR_HANDLER macros at all? Are there any exceptions that must be swallowed?

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

It looks to me like some reference errors (or more specifically, some circumstances which would result in a reference error) should be swallowed. There is some subtlety, since if x has not been declared or assigned to, then typeof(x) should return undefined, but typeof((() => x)()) should throw ReferenceError: x is not defined #Resolved

Jimmy, could you please take a look at the tests I've added for typeof? Anything missing? Currently only the test for throwing getter is failing.

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

MSLaguana · 2018-07-21T20:28:14Z

lib/Runtime/Language/JavascriptOperators.h

    CONTEXT                 ep##c;                  \
    EXCEPTION_POINTERS      ep = {&ep##er, &ep##c};

+// Re-frow from catch unwinds the active frame but doesn't clear the stack


Small typo here #Closed

yes :) thanks!

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

MSLaguana · 2018-07-21T20:32:46Z

lib/Runtime/Language/JavascriptOperators.h

 #endif

 #define BEGIN_TYPEOF_ERROR_HANDLER(scriptContext)  \
+    JavascriptExceptionObject* exceptionObject = nullptr; \


Perhaps add another block here to scope the exceptionObject to these macros? I think you'd just need a { here, and a } in END_TYPEOF_ERROR_HANDLER #Closed

MSLaguana · 2018-07-26T17:41:03Z

test/Basics/typeof.js

-WScript.Echo("typeof function : " + typeof (f));
-WScript.Echo("typeof function returning function : " + typeof (f()));
+            function test_let() { var res = typeof x; let x = 42; return res; }
+            assert.throws(test_let, ReferenceError, "typeof of 'let' variable in its dead zone", "Use before declaration");


Perhaps also add a case for typeof x.y when x is undefined #Closed

this one throws (in V8 as well) -- expected?

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

Yes, it should throw. #Closed

IrinaYatsenko · 2018-07-26T22:54:38Z

@LouisLaf @rajatd @MSLaguana -- I believe I've finally arrived to the correct exception handling behaviour for typeof. Could you please take a look and sign off?

MSLaguana

Looks good to me!

LouisLaf

rajatd

…f catch to avoid OS stack overflow.

Merge pull request #5497 from irinayat-MS:StackOverflow Fixes OS.18246154 Re-throw from catch doesn't unwind the stack (only resets the active frame pointer) so, if there are nested try/catch blocks that refrow, the stack keeps growing, which is unhelpful if the initial exception is trying to prevent Stack Overflow. To avoid this, move re-throw outside of the catch (so the stack fully unwinds on each handling site). Took the opportunity to add more test coverage for typeof and fix exception handling deviations from the spec (and V8).

…ut of catch Merge pull request #5497 from irinayat-MS:StackOverflow Fixes OS.18246154 Re-throw from catch doesn't unwind the stack (only resets the active frame pointer) so, if there are nested try/catch blocks that refrow, the stack keeps growing, which is unhelpful if the initial exception is trying to prevent Stack Overflow. To avoid this, move re-throw outside of the catch (so the stack fully unwinds on each handling site). Took the opportunity to add more test coverage for typeof and fix exception handling deviations from the spec (and V8).

IrinaYatsenko requested a review from LouisLaf July 20, 2018 21:20

LouisLaf reviewed Jul 20, 2018

View reviewed changes

MSLaguana reviewed Jul 21, 2018

View reviewed changes

boingoing approved these changes Jul 23, 2018

View reviewed changes

IrinaYatsenko force-pushed the StackOverflow branch from c510f65 to 1263bc2 Compare July 24, 2018 22:51

IrinaYatsenko changed the base branch from master to release/1.10 July 24, 2018 22:52

IrinaYatsenko requested a review from akroshg July 24, 2018 23:47

IrinaYatsenko mentioned this pull request Jul 24, 2018

OS 18333466 : Recursion in the generator next call. #5519

Merged

IrinaYatsenko force-pushed the StackOverflow branch from 1263bc2 to 34cc65d Compare July 26, 2018 17:32

MSLaguana reviewed Jul 26, 2018

View reviewed changes

IrinaYatsenko force-pushed the StackOverflow branch from 34cc65d to 5413f08 Compare July 26, 2018 22:50

MSLaguana approved these changes Jul 26, 2018

View reviewed changes

LouisLaf approved these changes Jul 27, 2018

View reviewed changes

rajatd approved these changes Jul 27, 2018

View reviewed changes

Make exception handling in typeof spec-complient. Move re-throw out o…

dacef92

…f catch to avoid OS stack overflow.

IrinaYatsenko force-pushed the StackOverflow branch from 5413f08 to dacef92 Compare July 27, 2018 15:57

chakrabot merged commit dacef92 into chakra-core:release/1.10 Jul 27, 2018

Move re-throw of exception out of catch #5497

Move re-throw of exception out of catch #5497

Uh oh!

Conversation

IrinaYatsenko commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LouisLaf Jul 20, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajatd Jul 20, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko Jul 24, 2018

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko Jul 24, 2018 • 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!

MSLaguana Jul 24, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko Jul 25, 2018

Choose a reason for hiding this comment

Uh oh!

MSLaguana Jul 25, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

MSLaguana Jul 21, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko Jul 23, 2018

Choose a reason for hiding this comment

Uh oh!

MSLaguana Jul 21, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MSLaguana Jul 26, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko Jul 26, 2018

Choose a reason for hiding this comment

Uh oh!

MSLaguana Jul 26, 2018 • edited by IrinaYatsenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IrinaYatsenko commented Jul 26, 2018

Uh oh!

MSLaguana left a comment

Choose a reason for hiding this comment

Uh oh!

LouisLaf left a comment

Choose a reason for hiding this comment

Uh oh!

rajatd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

IrinaYatsenko commented Jul 20, 2018 •

edited

Loading

LouisLaf Jul 20, 2018 •

edited by IrinaYatsenko

Loading

rajatd Jul 20, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Jul 24, 2018 •

edited

Loading

MSLaguana Jul 24, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana Jul 25, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana Jul 21, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana Jul 21, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana Jul 26, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana Jul 26, 2018 •

edited by IrinaYatsenko

Loading